HIVE-27238: Avoid Calcite Code generation for MetadataHandler on ever… #4212
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…y query In the current code base, a new CachingRelMetadataProvider is created on every query. This causes a cache miss within JaninoRelMetadataProvider, which stores the provider in the key. As a result, a new MetadataHandler is created multiple times in every query, which causes code generation and is a small hit on performance This commit caches HiveDefaultRelMetadataProviders globally which stores a RelMetadataProvider that can be reused. Ultimately, this class could most likely be removed as well, but this commit limits the scope of the change in a first pass.
|
Kudos, SonarCloud Quality Gate passed!
|
kasakrisz
reviewed
Apr 25, 2023
Comment on lines
+158
to
+163
| bldr.put(HiveConf.ConfVars.HIVE_EXECUTION_ENGINE, | ||
| conf.getVar(HiveConf.ConfVars.HIVE_EXECUTION_ENGINE)); | ||
| bldr.put(HiveConf.ConfVars.HIVE_CBO_EXTENDED_COST_MODEL, | ||
| conf.getBoolVar(HiveConf.ConfVars.HIVE_CBO_EXTENDED_COST_MODEL)); | ||
| bldr.put(HiveConf.ConfVars.MAPREDMAXSPLITSIZE, | ||
| conf.getLongVar(HiveConf.ConfVars.MAPREDMAXSPLITSIZE)); |
Contributor
There was a problem hiding this comment.
Are these settings can be changed without hs2 restart?
zabetak
reviewed
Apr 26, 2023
Member
zabetak
left a comment
zabetak
left a comment
There was a problem hiding this comment.
When a change claims to improve performance ideally it should be accompanied with a (micro) benchmark.
Moreover, adding a new global cache shared among queries is a significant and possibly risky change. Note that we can compile queries in parallel and I am not even sure if the generated metadata handler code or the HiveDefaultRelMetadataProvider is thread safe.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…y query
In the current code base, a new CachingRelMetadataProvider is created on every query. This causes a cache miss within JaninoRelMetadataProvider, which stores the provider in the key. As a result, a new MetadataHandler is created multiple times in every query, which causes code generation and is a small hit on performance
This commit caches HiveDefaultRelMetadataProviders globally which stores a RelMetadataProvider that can be reused. Ultimately, this class could most likely be removed as well, but this commit limits the scope of the change in a first pass.
What changes were proposed in this pull request?
Stated in commit message
Why are the changes needed?
This will help performance and possibly have a smaller memory footprint.
Does this PR introduce any user-facing change?
No
How was this patch tested?
This doesn't introduce new functionality, so current regression tests should be good. I ran this through the debugger and made sure the Calcite JaninoMetadataProvider.load3 method was not called after the first query was called with the same conf variables.