Introduce Immutables library #282

adutra · 2024-09-11T20:19:38Z

This PR introduces the Immutables library:

https://immutables.github.io/

This library is widely used in Iceberg and many other projects. The ability to define immutable components in an easy way, without struggling with the nitty-gritty details (getters, builders, equals, hashCode, toString, etc.) is a nice productivity boost and improves code correctness by enforcing immutability by default.

This PR only adds the library and converts a few components that were good candidates for immutability. If the change is accepted, there are probably more components that could be progressively converted into immutables.

dimas-b

Looks like a nice improvement to me :)

ashvina · 2024-09-12T16:05:32Z

I agree, immutables and Lombok are valuable development tools, and I have used them in other projects. Thank you for the contribution.
IMO, the use of immutables is an internal detail, and immutables should be replaceable without breaking compatibility. It's not necessary for users to be aware of it. Perhaps it would be more appropriate to include the new annotations in the core module instead. WDYT?
Additionally, sharing high level guidelines with other developers on migrating existing code and documenting new annotations would be beneficial.

adutra · 2024-09-12T16:12:53Z

Perhaps it would be more appropriate to include the new annotations in the core module instead. WDYT?

Unfortunately, that's not feasible. The procedure for adding a custom annotation requires the annotation to be placed in a separate module, because it needs to be present on the annotation processor's classpath. It's explained here: https://immutables.github.io/style.html#custom-immutable-annotation.

snazy

This is overall a very nice improvement!

The only thing that worries me a little bit is the change in the PolarisStorageConfigurationInfo hierarchy. I think we should keep the original test - it might work when overriding validate() in StorageConfigurationOverride. I suspect, this change didn't break anything, but to be on the safe side wrt polymorphism can you add a test to validate that the JSON-serialized representation didn't change? Some test that passes for the state before this change and with this change. I think, then we're good here.

snazy · 2024-09-13T15:46:53Z

polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java

-    this.externalId = externalId;
-    validateMaxAllowedLocations(MAX_ALLOWED_LOCATIONS);
+  @Override
+  public abstract List<String> getAllowedLocations();


Nit: I think those overrides can go away.

I added these overrides just to avoid warnings like this one:

> Task :polaris-core:compileJava polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:35: warning: (immutables:subtype) Constructor parameters should be better defined on the same level of inheritance hierarchy, otherwise generated constructor API would be unstable: parameter list can change the order of arguments. It is better redeclare (override) each inherited attribute parameter in this abstract value type to avoid this warning. Or better have constructor parameters defined by only single supertype. public abstract class AwsStorageConfigurationInfo extends PolarisStorageConfigurationInfo { ^ 1 warning

Is there a better way to handle this?

I wonder if it's a good idea to set allParameters = true by default in @PolarisImmutable, wdyt? As soon as the interface extends other interfaces, the warning pops up unless all methods are overridden.

Yea - it's just a warning. But warnings are annoying.
It's okay to remove allParameters = true.

snazy · 2024-09-13T15:50:32Z

polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java

  }

-  T cast(Object value) {
-    return this.typ.cast(value);
+  @Value.Lazy


Suggested change

@Value.Lazy

@Value.Derived

Err, I meant - doesn't need to be stored.

Suggested change

@Value.Lazy

@Value.NonAttribute

Minor: this suggestion is NOT applied

Sorry I applied it to the wrong method 🤦‍♂️

snazy · 2024-09-13T15:55:33Z

...c/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java

@@ -94,26 +88,11 @@ public void testGetSubscopedCreds() {
        .containsEntry(PolarisCredentialProperty.AWS_SECRET_KEY, "secretKey");
  }

-  @ParameterizedTest
-  @ValueSource(strings = {AWS_PARTITION, "aws-cn", "aws-us-gov"})


Maybe just comment out the last two ones for now?
Or probably better: override validate() in StorageConfigurationOverride, which seems to be the only case where the locations are not validated.

You are right that I need to override validate() in StorageConfigurationOverride – the current code looks really fishy as it mixes validation on the override instance vs on the delegate instance.

But unfortunately the problem with ARN domains cannot be solved like this: the validateArn() method belongs to the AWS subtype and the subtype is being constructed directly in the test, with the forbidden domains. I will find something else.

snazy · 2024-09-13T16:35:02Z

polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfo.java

-  @JsonSubTypes.Type(value = AzureStorageConfigurationInfo.class),
-  @JsonSubTypes.Type(value = GcpStorageConfigurationInfo.class),
-  @JsonSubTypes.Type(value = FileStorageConfigurationInfo.class),
+  @JsonSubTypes.Type(value = ImmutableAwsStorageConfigurationInfo.class),


We're changing the types in this hierarchy quite a bit.
Can you add a test class (or tests in the individual sub-type test classes) to ensure that the serialized JSON format is still the same?

adutra · 2024-09-13T20:43:54Z

...-core/src/test/java/org/apache/polaris/core/storage/PolarisStorageConfigurationInfoTest.java

+  @MethodSource
+  void serialize(PolarisStorageConfigurationInfo instance, String typeName) {
+    String json = instance.serialize();
+    assertThat(json).contains("\"@type\":\"" + typeName + "\"");


@snazy I manually ran this test against main and verified that the serialized json is similar: the @type attribute is the same and the rest is similar in the sense that the new json from this branch generally contains more properties than the json from main (you cannot just add @JsonIgnore and expect the immutables library to honor it: if the property is part of the object, it will be serialized – besides, the equals() method would fail after deserialization if the property was ignored).

But since the ObjectMapper is configured with FAIL_ON_UNKNOWN_PROPERTIES I think we are good.

But since the ObjectMapper is configured with FAIL_ON_UNKNOWN_PROPERTIES I think we are good.

Hm - I see DEFAULT_MAPPER.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES); in PolarisStorageConfigurationInfo.

Sorry that's what I meant: FAIL_ON_UNKNOWN_PROPERTIES is disabled.

Also looked at the serialized JSONs from main and this branch - and the result's equal, except for AWS (see above).

snazy · 2024-09-16T07:48:50Z

polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java

-  public void setExternalId(String externalId) {
-    this.externalId = externalId;
+  @Value.Default
+  protected Set<String> getAllowedArnDomains() {


This seems to be a new attribute - not sure whether it's good to add it in this PR?

I think I need to revert the ConfigurationInfo changes then. I don't see any other way to keep the current behavior of rejecting China/USGov while still allowing it in tests.

Yea - I guess that's fine. Those ARN domains "break" in prod code anyways.

snazy

The change LGTM now!

I'll leave it unmerged for a bit for others to get a chance to look at it before merging.

flyrain · 2024-09-16T16:27:41Z

Unfortunately, that's not feasible. The procedure for adding a custom annotation requires the annotation to be placed in a separate module, because it needs to be present on the annotation processor's classpath. It's explained here:

Hi @adutra, does that mean the third-party projects have to depend on two jar files(polaris-immutables.jar and polaris-cores) if they want to reuse module polaris-core?

Is it possible to upgrade polaris-core jdk from 11 to 17, so that we can use a native solution like Record?

cc @collado-mike @dennishuo @eric-maynard

snazy · 2024-09-16T16:32:54Z

@flyrain immutables does not have a runtime dependency. Java records are different - immutables and records are not exclusive. BTW: Java records are still "plain/ordinary" Java objects - rather syntactic sugar. In terms of runtime performance and footprint both are rather equal - just that immutables offers much richer functionalities.

(What we're all waiting for are value objects that can be "properly flattened" ;) )

flyrain · 2024-09-16T16:52:30Z

@snazy sorry I wasn't clear. I was asking if we can use Java record instead of the immutables. This is only possible when we upgrade jdk, as record was introduced after jdk 11. I understand the immutables provides more functionalities, but if record is good enough, that'd be a cleaner solution.

flyrain · 2024-09-16T21:13:29Z

Immutables does not have a runtime dependency.

The annotation jar is not needed at runtime.

Looking a bit more. The polaris-immutables.jar is not needed at the runtime. It's great that any other downstream project doesn't have to know the module polaris-immutables exists, the complexity is only added in the Polaris project.

flyrain

LGTM overall with minor comments and questions.

flyrain · 2024-09-16T21:48:24Z

polaris-core/src/main/java/org/apache/polaris/core/PolarisCallContext.java


-  // meta store which is used to persist Polaris entity metadata
-  private final PolarisMetaStoreSession metaStore;
+  @Value.Parameter(order = 0)


Is the order needed here? Looks like the constructor isn't used anywhere.

It's required to trigger the generation of ImmutablePolarisCallContext.of(...) factory methods.

flyrain · 2024-09-16T21:53:58Z

polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java

  }

-  T cast(Object value) {
-    return this.typ.cast(value);
+  @Value.Lazy


Minor: this suggestion is NOT applied

flyrain · 2024-09-16T23:02:22Z

polaris-immutables/src/main/java/org/apache/polaris/immutables/PolarisImmutable.java

+ * {@code @Value.Immutable}</a> using {@code lazyhash=true, forceJacksonPropertyNames = false,
+ * clearBuilder = true, depluralize = true, and JavaBeans-style getters}.
+ */


I think we don't need to repeat the properties in the Java doc. Maybe we could add a bit reason why we choose these properties in the doc.

flyrain · 2024-09-16T23:16:54Z

...rvice/src/main/java/org/apache/polaris/service/context/PolarisCallContextCatalogFactory.java

@@ -82,8 +82,6 @@ public Catalog createCallContextCatalog(
            taskExecutor,
            fileIOFactory);

-    context.contextVariables().put(CallContext.REQUEST_PATH_CATALOG_INSTANCE_KEY, catalogInstance);


Curious how the catalogInstance is set in context if we remove this line.

The call context is re-created once this method exits:

polaris/polaris-service/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java

Lines 145 to 152 in d854a37

ImmutableMap.Builder<String, Object> contextVariablesBuilder = ImmutableMap.builder();

contextVariablesBuilder.putAll(callContext.contextVariables());

contextVariablesBuilder.put(CallContext.REQUEST_PATH_CATALOG_INSTANCE_KEY, baseCatalog);

this.callContext =

ImmutableCallContext.builder()

.from(callContext)

.contextVariables(contextVariablesBuilder.buildKeepingLast())

.build();

flyrain · 2024-09-17T17:30:48Z

polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java

-      }
-      return new PolarisConfiguration<>(key, description, defaultValue, catalogConfig);
-    }
+  default T cast(Object value) {


We will also need the annotation @Value.NonAttribute here, as it is an utility method instead of an attribute.

I don't think so, this method is not a setter nor a getter. If you look at the generated class, there is no cast method nor field.

You are right. There is no such field. In that case, we don't have to add the annotation to method default String catalogConfigOrThrow() either, right?

We do, because given its signature, it would be considered as an accessor method.

FYI, this doesn't matter since you close it, but my test shows it doesn't generate the method in Immutable class without it.

flyrain

LGTM with one minor comment

eric-maynard · 2024-09-17T19:47:05Z

polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java

-  T cast(Object value) {
-    return this.typ.cast(value);
+  @Value.NonAttribute
+  default String catalogConfigOrThrow() {


This PR seems to include a lot of generic refactoring outside of just adding a dependency.

eric-maynard · 2024-09-17T19:47:26Z

polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java

@@ -111,7 +76,7 @@ public static <T> Builder<T> builder() {
          .defaultValue(false)
          .build();

-  public static final PolarisConfiguration<Boolean> ALLOW_NAMESPACE_LOCATION_OVERLAP =
+  PolarisConfiguration<Boolean> ALLOW_NAMESPACE_LOCATION_OVERLAP =


Again, refactoring

eric-maynard · 2024-09-17T19:48:04Z

...e/src/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java

@@ -219,7 +220,7 @@ public void setStorageIntegrationProvider(PolarisStorageIntegrationProvider stor
  private void checkPolarisServiceBootstrappedForRealm(
      RealmContext realmContext, PolarisMetaStoreManager metaStoreManager) {
    PolarisCallContext polarisContext =
-        new PolarisCallContext(
+        PolarisCallContext.of(


This syntactic sugar seems to be the primary "advantage" we are gaining from adding this framework. Is this really better?

eric-maynard · 2024-09-17T19:48:55Z

polaris-core/src/main/java/org/apache/polaris/core/storage/FileStorageConfigurationInfo.java

+@JsonSerialize(as = ImmutableFileStorageConfigurationInfo.class)
+@JsonDeserialize(as = ImmutableFileStorageConfigurationInfo.class)


Do we actually need to add this to classes?

eric-maynard · 2024-09-17T19:49:30Z

polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java

-  // Technically, it should be ^arn:(aws|aws-cn|aws-us-gov):iam::\d{12}:role/.+$,
-  @JsonIgnore public static final String ROLE_ARN_PATTERN = "^arn:aws:iam::\\d{12}:role/.+$";
+  private static final String ROLE_ARN_PATTERN =
+      "^arn:(aws|aws-cn|aws-us-gov):iam::\\d{12}:role/.+$";


This is a material change

eric-maynard · 2024-09-17T19:51:21Z

This is a pretty large refactor that also includes a couple of nontrivial changes.

The ability to define immutable components in an easy way, without struggling with the nitty-gritty details (getters, builders, equals, hashCode, toString, etc.) is a nice productivity boost

In much of the code base, can't we just use record for this?

adutra · 2024-09-17T20:40:41Z

Ok at this point I don't see a huge appetite from the community for this change. Let's just wait until we can use records everywhere. Closing.

flyrain · 2024-09-17T21:14:55Z

I guess a followup task is to figure out if we can compile the polaris-core with newer jdk, like jdk17 or jdk21.

adutra requested review from jbonofre, ashvina, RussellSpitzer, snazy, vvcephei, takidau and jackye1995 as code owners September 11, 2024 20:19

dimas-b approved these changes Sep 11, 2024

View reviewed changes

adutra force-pushed the immutables branch from 2d02303 to 211d11e Compare September 12, 2024 10:25

adutra requested a review from flyrain as a code owner September 12, 2024 10:25

snazy reviewed Sep 13, 2024

View reviewed changes

adutra commented Sep 13, 2024

View reviewed changes

snazy reviewed Sep 16, 2024

View reviewed changes

adutra added 9 commits September 16, 2024 12:30

Introduce Java Immutables

d854a37

PolarisCallContext

705c20f

PolarisConfiguration

59f9fdd

PolarisStorageConfigurationInfo & hierarchy

89b7212

review

967cbe0

serialize test

cfe35c0

PolarisStorageConfigurationInfo hierarchy fixes + test

bc22de5

nit

305b8b2

revert ARN stuff

2f328c9

adutra force-pushed the immutables branch from 9722e44 to 2f328c9 Compare September 16, 2024 10:30

snazy previously approved these changes Sep 16, 2024

View reviewed changes

remove allParameters=true

fd58cc8

adutra dismissed snazy’s stale review via fd58cc8 September 16, 2024 11:21

snazy previously approved these changes Sep 16, 2024

View reviewed changes

flyrain reviewed Sep 16, 2024

View reviewed changes

more reviews

1f0cd1a

adutra dismissed snazy’s stale review via 1f0cd1a September 17, 2024 08:39

flyrain reviewed Sep 17, 2024

View reviewed changes

eric-maynard reviewed Sep 17, 2024

View reviewed changes

adutra closed this Sep 17, 2024

	ImmutableMap.Builder<String, Object> contextVariablesBuilder = ImmutableMap.builder();
	contextVariablesBuilder.putAll(callContext.contextVariables());
	contextVariablesBuilder.put(CallContext.REQUEST_PATH_CATALOG_INSTANCE_KEY, baseCatalog);
	this.callContext =
	ImmutableCallContext.builder()
	.from(callContext)
	.contextVariables(contextVariablesBuilder.buildKeepingLast())
	.build();

		@JsonSerialize(as = ImmutableFileStorageConfigurationInfo.class)
		@JsonDeserialize(as = ImmutableFileStorageConfigurationInfo.class)

Introduce Immutables library #282

Introduce Immutables library #282

Conversation

adutra commented Sep 11, 2024

dimas-b left a comment

Choose a reason for hiding this comment

ashvina commented Sep 12, 2024

adutra commented Sep 12, 2024

snazy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adutra Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adutra Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snazy left a comment

Choose a reason for hiding this comment

flyrain commented Sep 16, 2024

snazy commented Sep 16, 2024

flyrain commented Sep 16, 2024

flyrain commented Sep 16, 2024

flyrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flyrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-maynard commented Sep 17, 2024

adutra commented Sep 17, 2024

flyrain commented Sep 17, 2024

adutra Sep 17, 2024 •

edited

Loading

adutra Sep 13, 2024 •

edited

Loading