[native] Add static mapping of native query configs #21266
Merged
Conversation
This file contains 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
mbasmanova
reviewed
Oct 28, 2023
presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java
Outdated
Show resolved
Hide resolved
presto-native-execution/presto_cpp/main/QueryContextManager.cpp
Outdated
Show resolved
Hide resolved
presto-native-execution/presto_cpp/main/QueryContextManager.cpp
Outdated
Show resolved
Hide resolved
presto-native-execution/presto_cpp/main/QueryContextManager.cpp
Outdated
Show resolved
Hide resolved
presto-native-execution/presto_cpp/main/QueryContextManager.cpp
Outdated
Show resolved
Hide resolved
presto-native-execution/presto_cpp/main/QueryContextManager.cpp
Outdated
Show resolved
Hide resolved
|
nit but as a reminder: In my observation, in the title of the commit message, it is more likely to use |
amitkdutta
changed the title
kewang1024
reviewed
Oct 30, 2023
presto-native-execution/presto_cpp/main/QueryContextManager.cpp
Outdated
Show resolved
Hide resolved
bikramSingh91
force-pushed
the
query_configs
branch
from
2002f85 to
5181dea
Compare
@kewang1024 |
kewang1024
reviewed
Nov 2, 2023
Comment on lines
75
to
85
| {"native_execution_process_reuse_enabled", | ||
| QueryConfig::kAggregationSpillMemoryThreshold}, |
There was a problem hiding this comment.
Looks like this pair doesn't match?
There was a problem hiding this comment.
Thanks for catching this, forgot to remove it since it doesnt map to anything in velox. Will remove it in the next iteration of this PR
bikramSingh91
force-pushed
the
query_configs
branch
from
5181dea to
5f1f55d
Compare
mbasmanova
approved these changes
Nov 3, 2023
| return name.substr(kNativePrefix.length()); | ||
| } | ||
| return name; | ||
| // Utility function to translate a config name in presto to its equivalent in |
bikramSingh91
force-pushed
the
query_configs
branch
3 times, most recently
from
a5bcc5e to
6df067b
Compare
This change updates the mapping of native only query configs from a prefix-based system to a hardcoded, static mapping. This is to decouple naming conventions that can be different between presto and velox. For instance, Velox permits the use of dots, whereas Presto employs dots to indicate the catalog to which a property should be linked.
bikramSingh91
force-pushed
the
query_configs
branch
from
6df067b to
9e11fc0
Compare
|
A failure in testQueryScanExceeded in pre-commits which looks unrelated |
xiaoxmeng
approved these changes
Nov 9, 2023
26 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This change updates the mapping of native only query configs from a
prefix-based system to a hardcoded, static mapping. This is to
decouple naming conventions that can be different between presto
and velox. For instance, Velox permits the use of dots, whereas Presto
employs dots to indicate the catalog to which a property should
be linked.