hive HIVE-28046: Using serdeConstants fields instead of string literals in hive-exec module

HIVE-28046: Using serdeConstants fields instead of string literals in hive-exec module

Open mlorek opened this issue 1 year ago • 6 comments

Using serdeConstants fields instead of string literals in hive-exec module

Why are the changes needed? to improve the use of existing constants

Does this PR introduce any user-facing change? No

Is the change a dependency upgrade? No

How was this patch tested? using existing tests

Feb 07 '24 10:02 mlorek

Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

4 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Feb 07 '24 13:02 sonarqubecloud[bot]

@abstractdog can you take a look?

Feb 07 '24 19:02 mlorek

cc @ayushtkn

Feb 09 '24 15:02 mlorek

@zabetak amy idea who can review this PR?

Feb 21 '24 08:02 mlorek

At first glance it looks reasonable. I will take a better look tomorrow.

One thing that I am not sure if it's beneficial is using the serdeConstants in tests. I assume that we don't want to change the values of such constants cause we risk breaking compatibility. By hardcoding the string values in tests we can guard against such changes.

looks good to me

I don't think we're about to change these constants, and I cannot think of a dev scenario where we risk compatibility due to the fact that we're using the constants in tests

Feb 22 '24 11:02 abstractdog

I don't think we're about to change these constants, and I cannot think of a dev scenario where we risk compatibility due to the fact that we're using the constants in tests

If a developer changes serdeConstants#INT_TYPE_NAME from "int" to "integer" then if the tests are using the constant they will all pass without issues hiding the fact that the type name change and this may be a breaking change for end-users. Doing a holistic replace in tests would require very careful review without a significant gain.

Also note that when we attempted a replace literals with constants in ql/src/test I got 84 files changed, 745 insertions(+), 745 deletions(-) which is much more than what is currently is covered by this PR.