apache / polaris

The interoperable, open source catalog for Apache Iceberg
http://polaris.io/
Apache License 2.0
1.02k stars 99 forks source link

Introduce Immutables library #282

Closed adutra closed 3 hours ago

adutra commented 6 days ago

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.

ashvina commented 5 days ago

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 commented 5 days ago

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.

flyrain commented 1 day ago

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 commented 1 day ago

@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 commented 1 day ago

@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 commented 1 day ago

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.

eric-maynard commented 4 hours ago

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 commented 3 hours ago

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 commented 3 hours ago

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