Skip to content

Conversation

@asolimando
Copy link
Member

What changes were proposed in this pull request?

Improving the modularity of the *ColumnStatsMerger classes by introducing a common parent class and moving there all the shared code that is not depending on the data type.

Why are the changes needed?

Those classes host some complex merging logic that is copy-pasted into several classes, a problem that is bound to worsen as we add new (and more complex) statistics.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing and introduced unit-tests relevant for the change:
mvn test -Dtest.groups=org.apache.hadoop.hive.metastore.annotation.MetastoreUnitTest -Dtest='*ColumnStats*Test.java' -pl standalone-metastore/metastore-server

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
No Duplication information No Duplication information

@akshat0395
Copy link
Contributor

LGTM, +1

@asolimando
Copy link
Member Author

@ayushtkn, if you have spare cycles maybe you can help get this in since it's been approved already? Thanks!

Copy link
Member

@ayushtkn ayushtkn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Will give a day before committing in case anyone else has comments

@ayushtkn ayushtkn merged commit 35b1518 into apache:master Feb 4, 2023
ayushtkn added a commit that referenced this pull request Feb 4, 2023
…classes (#3997). (Alessandro Solimando, reviewed by Ayush Saxena)"

This reverts commit 35b1518.

Reverting to correct the commit message
ayushtkn pushed a commit that referenced this pull request Feb 4, 2023
…3997). (Alessandro Solimando, reviewed by Ayush Saxena, Akshat Mathur)
@ayushtkn
Copy link
Member

ayushtkn commented Feb 4, 2023

And I messed up while committing :-(
Forgot to mention Akshat's name while committing, Have reverted this and committed it again with the correct message. I don't have anything more than sorry here, will be more careful next time

@asolimando asolimando deleted the master-HIVE-27000-improve_column_stats_mergers branch February 4, 2023 21:26
@asolimando
Copy link
Member Author

And I messed up while committing :-( Forgot to mention Akshat's name while committing, Have reverted this and committed it again with the correct message. I don't have anything more than sorry here, will be more careful next time

We never make mistakes only if we do nothing at all, you are very very active so it happens. On that note, thanks a lot for always helping out with reviews and merges, much appreciated Ayush!

yeahyung pushed a commit to yeahyung/hive that referenced this pull request Jul 20, 2023
yeahyung pushed a commit to yeahyung/hive that referenced this pull request Jul 20, 2023
…classes (apache#3997). (Alessandro Solimando, reviewed by Ayush Saxena)"

This reverts commit 35b1518.

Reverting to correct the commit message
yeahyung pushed a commit to yeahyung/hive that referenced this pull request Jul 20, 2023
…pache#3997). (Alessandro Solimando, reviewed by Ayush Saxena, Akshat Mathur)
tarak271 pushed a commit to tarak271/hive-1 that referenced this pull request Dec 19, 2023
tarak271 pushed a commit to tarak271/hive-1 that referenced this pull request Dec 19, 2023
…classes (apache#3997). (Alessandro Solimando, reviewed by Ayush Saxena)"

This reverts commit 35b1518.

Reverting to correct the commit message
tarak271 pushed a commit to tarak271/hive-1 that referenced this pull request Dec 19, 2023
…pache#3997). (Alessandro Solimando, reviewed by Ayush Saxena, Akshat Mathur)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants