Hive: Refactor HMS table parameter setting to be able to reuse

[zratkai]

HIVE-28801 Iceberg: Refactor HMS table parameter setting to be able to reuse

[gaborkaszab]

Hi @zratkai ,

Could you give me a bit more details for the motivation of this PR? You mention code reusal in the description, but I saw that it's simply a refactor from Iceberg's perspective, and I also checked the Hive PR popping up here, but that doesn't add any code reusal either. So I guess, the motivation is rather code refactor.

In general, I think that having a util class that helps producing table properties relevant for HMS from some inputs makes sense. However, with the current PR I have the impressions that the work was just partially made. I saw some remaining functionality in HiveOperationsBase, and also in HiveViewOperations that also take care of HMS related table properties. I think with a refactor like this we should go all-in. Otherwise, it won't be very intuitive what responsibilities this new converter class has and what is left that are done outside the class.

So in general the idea would make sense, but I'm not comfortable with the implementation TBH.

[zratkai]

@gaborkaszab here is the origin of the PR:
apache/hive#5628 (comment)

[zratkai]

@pvary could you please review?

[gaborkaszab]

I'm fine with the PR in general. I added some comments for better clarity on this code for future readers.

[wypoon]

Some small suggestions and test fixes.

[zratkai]

@pvary could you please review? I addressed all your comments.

[pvary]

Left my final comments.
LGTM +1

Thanks @zratkai for your persistence!

[gaborkaszab]

With Peter's suggestions, the PR looks fine for me. Thanks for this work, @zratkai !

[zratkai]

@pvary thanks for the review! Fixed your comments.

[wypoon]

I suggested this before and I'll suggest it again:

I think that it would be valuable to preserve a small part of the javadoc for HiveTableOperations.translateToIcebergProp which is removed in this PR, and add it as a comment before this static field:
"Provides key translation where necessary between Iceberg and HMS props. This translation is needed because some properties control the same behaviour but are named differently in Iceberg and Hive."
Then the inner comment about gc.enabled and external.table.purge is not needed, as it would be clear.

At the moment, there is only one property that needs translation, and the inner comment explains why we currently do this translation. It is conceivable that in future, more properties may need translation. Having a comment explaining the reason for the Map is better than an inner comment explaining why gc.enabled needs translation.

[zratkai]

This description was for the translateToIcebergProp() method, which is not used, so it is deleted. This is why this is not needed.

[wypoon]

Hi @zratkai. You are missing the point. The point is that
"Provides key translation where necessary between Iceberg and HMS props. This translation is needed because some properties control the same behaviour but are named differently in Iceberg and Hive."
is an appropriate, relevant, and informative description of the Map here. What is the purpose of the Map? It is exactly this.

You may think, "I just want to move some chunks of code around; why am I being made to make all these other changes?"
To my mind, a refactor is an opportunity to improve the usability and the readability of code, not just a rearrangement of code. And I'm not making you do anything. I understand someone may not accept all suggestions offered. However, I offer you my time and attention, and make only suggestions that I believe are constructive, and take the time to explain my reasoning.

[pvary]

Updated the PR based on @wypoon's suggestion

[pvary]

Merged to main.
Thanks @zratkai for the PR and the persistence to enhance it! Please next time be more respectful for reviewers. They often spend their own personal time on reviewing changes.
Thanks @wypoon, @gaborkaszab for the reviews!