Mutable objects used for immutable values #772

[fabio-rizzo-01]

Updated Polaris entity objects to use builders instead of normal constructors and removed all the set methods to guarantee object immutability.
All the other changes are refactoring based on those changes.

[snazy]

I like the removal of all the setters!
However, since these types now look pretty much how those would look like when using immutables, mind going down that route? That would also remove all the hand-written Builders.

[pingtimeout]

I agree with Robert. I think this PR does not fully embrace the immutable pattern.

• The class fields are not final, which would clearly establish the intent of those objects being immutable.
• Adding the Builder subclasses adds a lot of code that could be automatically generated
• (nit) It could even be possible to annotate the classes with @Immutable (from jcip) to enable an extra verification step in the IDE

It would be good to explore the immutables library to address some of these points.

Note that this PR contains a removal of some utility copy-constructors (PolarisEntityCore from PolarisBaseEntity), which might not be related?

[singhpk234]

+1 to PolarisImmutables

[fabio-rizzo-01]

I like the removal of all the setters! However, since these types now look pretty much how those would look like when using immutables, mind going down that route? That would also remove all the hand-written Builders.

From the bug description I thought the point was to make those object not mutable, i didn't think there was a library already there hence why I created the builders. can you guys point me in the right direction thx ? @singhpk234

[eric-maynard]

@fabio-rizzo-01 I think the ask is that you use this interface.

Besides this, I think we should try to minimize the changes that callers need to make as much as possible.

[fabio-rizzo-01]

@fabio-rizzo-01 I think the ask is that you use this interface.

Besides this, I think we should try to minimize the changes that callers need to make as much as possible.

Looking at that interface and seeing where it is used, it seems to me that using it would require even more changes that what I already did.

Taking PolarisEntityCore as an example, to convert that to an immutable object I'd have to create an interface (something like PolarisEntityCoreInt) that uses PolarisImmutable annotation, that would generate a class called Immutable PolarisEntityCoreInt. So all the code that is using PolarisEntityCore object it needs to be updated to use the new immutable object.

Is this what you guys are suggesting ? @snazy @singhpk234

[eric-maynard]

Personally I'm fine with you not using @PolarisImmutable -- I still haven't figured out why we need that or why my IDE so often doesn't seem to like it:

But I do think this PR makes quite a few changes, such as those to TransactionalMetaStoreManagerImpl.java, which could be reduced or elimated. For example, does the method setGrantRecordsVersion really need to go away? If it does, can we implement a cloning method like withGrantRecordsVersion?

[fabio-rizzo-01]

Personally I'm fine with you not using @PolarisImmutable -- I still haven't figured out why we need that or why my IDE so often doesn't seem to like it:

But I do think this PR makes quite a few changes, such as those to TransactionalMetaStoreManagerImpl.java, which could be reduced or elimated. For example, does the method setGrantRecordsVersion really need to go away? If it does, can we implement a cloning method like withGrantRecordsVersion?

The builder in the PolarisBaseEntity has a constructor that allows you to pass an entity into it, that is what I'm using inTransactionalMetaStoreManagerImpl.java to update properties and the grant record version.

Like here https://github.com/apache/polaris/pull/1596/files#diff-5327c1f118c61e497b4fb5b3200002388a3a4bcde3b44d91403d202bac6a0cccR293

[snazy]

Taking PolarisEntityCore as an example, to convert that to an immutable object I'd have to create an interface (something like PolarisEntityCoreInt) that uses PolarisImmutable annotation, that would generate a class called Immutable PolarisEntityCoreInt. So all the code that is using PolarisEntityCore object it needs to be updated to use the new immutable object.

Is this what you guys are suggesting ? @snazy @singhpk234

Yes.

[fabio-rizzo-01]

Taking PolarisEntityCore as an example, to convert that to an immutable object I'd have to create an interface (something like PolarisEntityCoreInt) that uses PolarisImmutable annotation, that would generate a class called Immutable PolarisEntityCoreInt. So all the code that is using PolarisEntityCore object it needs to be updated to use the new immutable object.
Is this what you guys are suggesting ? @snazy @singhpk234

Yes.

Ok. In doing so every entity object in Polaris will need to change because every entity (catalog, namespace, etc.) extends PolarisEntity that extends PolarisBaseEntity. Correct?
I just want to be sure before I start rewriting the code that I'm not going down the wrong path.

[eric-maynard]

@fabio-rizzo-01 the example you gave is a good one; my point is you could reduce that change to only this line:

    securableEntity.setGrantRecordsVersion(securableEntity.getGrantRecordsVersion() + 1);

Which could become like:

securableEntity = securableEntity.withGrantRecordsVersion(securableEntity.getGrantRecordsVersion() + 1)

(Or we could make securableEntity final and make a new variable, but the code change required to 'update' grantRecordsVersion remains smaller)

[fabio-rizzo-01]

@fabio-rizzo-01 the example you gave is a good one; my point is you could reduce that change to only this line:

    securableEntity.setGrantRecordsVersion(securableEntity.getGrantRecordsVersion() + 1);

Which could become like:

securableEntity = securableEntity.withGrantRecordsVersion(securableEntity.getGrantRecordsVersion() + 1)

(Or we could make securableEntity final and make a new variable, but the code change required to 'update' grantRecordsVersion remains smaller)

that makes sense, I'll wait for the others to answer my previous question before I'll make the changes.

[flyrain]

Thanks @fabio-rizzo-01 for doing this! +1 on using annotation PolarisImmutable.

[fabio-rizzo-01]

Thanks @fabio-rizzo-01 for doing this! +1 on using annotation PolarisImmutable.

I understand the push to use PolarisImmutable, but I've identified some issues with its implementation:

Our object hierarchy consists of Core -> Base -> Entity -> Catalog, with each class extending the previous one. Applying PolarisImmutable to Core and Base would result in two new immutable classes, both of which are final. This prevents further extension of the immutable Base class, disrupting our current hierarchy.

While there are workarounds, they would necessitate significant code changes, which I believe should be minimized in this part of the repository.

@snazy @singhpk234 @flyrain