Fix Removing Partition Builder

[konjac-h]

Description

Ensuring that the partition maintains its original type (e.g., child class) and associated information.

Motivation and Context

The issue here is that the Partition class is a base class for other child-class Partition, and when using the Partition.builder() method to create a new instance of Partition, the resulting object will be of type Partition rather than child-class Partition. This means that any methods or properties specific to child-class Partition will not be available on the new object created by the builder.

Impact

Test Plan

Unit Test in TestMemcacheHiveMetastore.java

Contributor checklist

• Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
• PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
• Documented new properties (with its default value), SQL syntax, functions, or other functionality.
• If release notes are required, they follow the release notes guidelines.
• Adequate tests were added if applicable.
• CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

== NO RELEASE NOTE ==

[konjac-h]

This PR doesn't set the lastDataCommitTime. I would love to initiate the discussion whether we need to update the corresponding attribute. If so, we might need a setter method instead of purely builder class (or if anyone else has better suggestions on this)

[NikhilCollooru]

There is a test failure related to your code changes

Error:  Tests run: 2977, Failures: 1, Errors: 0, Skipped: 90, Time elapsed: 2,749.705 s <<< FAILURE! - in TestSuite
Error:  com.facebook.presto.hive.TestHiveCommitHandleOutput.testCommitOutputForPartitions  Time elapsed: 0.038 s  <<< FAILURE!
java.lang.AssertionError: expected [1715649749] but found [0]
	at org.testng.Assert.fail(Assert.java:110)
	at org.testng.Assert.failNotEquals(Assert.java:1413)
	at org.testng.Assert.assertEqualsImpl(Assert.java:149)
	at org.testng.Assert.assertEquals(Assert.java:131)
	at org.testng.Assert.assertEquals(Assert.java:655)
	at org.testng.Assert.assertEquals(Assert.java:665)
	at com.facebook.presto.hive.TestHiveCommitHandleOutput.testCommitOutputForPartitions(TestHiveCommitHandleOutput.java:204)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:135)
	at org.testng.internal.invokers.TestInvoker.invokeMethod(TestInvoker.java:673)
	at org.testng.internal.invokers.TestInvoker.invokeTestMethod(TestInvoker.java:220)
	at org.testng.internal.invokers.MethodRunner.runInSequence(MethodRunner.java:50)
	at org.testng.internal.invokers.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:945)
	at org.testng.internal.invokers.TestInvoker.invokeTestMethods(TestInvoker.java:193)
	at org.testng.internal.invokers.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
	at org.testng.internal.invokers.TestMethodWorker.run(TestMethodWorker.java:128)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:750)

[elharo]

Tangential note, but setter methods instead of or in addition to builders are really useful in test code like this. I've been seeing a need for these in other test classes. They can be marked @VisibleForTesting if that helps. Also, they help avoid CVEs like https://nvd.nist.gov/vuln/detail/CVE-2022-3171 where builders turn into a DoS attack.

Immutability is a convenient property to have but only when objects are truly immutable. When the things the objects represent do change over time, classes should be mutable to reflect that. E.g. if a file's last modified time isn't fixed, then the File object shouldn't be immutable either.

[konjac-h]

@elharo thanks it's good to know about it. Nikhil and I have synced offline and we came out with a hack way to deal with this issue instead of changing the open source code. However, I will def look into what you mentioned about @VisibleForTesting. I also agree that setter methods are pretty useful for some test cases. I will modify the PR correspondingly afterward. Thanks