Fix explicit tableOptions hint handling in PinotImplicitTableHintRule#19003
Open
yashmayya wants to merge 1 commit into
Open
Fix explicit tableOptions hint handling in PinotImplicitTableHintRule#19003yashmayya wants to merge 1 commit into
yashmayya wants to merge 1 commit into
Conversation
…plicit partition overrides
Jackie-Jiang
left a comment
Contributor
There was a problem hiding this comment.
I feel the main issue is that TableOptions do not maintain all available options. When is_replicated is set, we shouldn't override partition related options
| */ | ||
| private static ImmutableTableOptions overridePartitionKey(ImmutableTableOptions base, TableScan tableScan, | ||
| Map<String, String> kvOptions) { | ||
| String partitionKey = kvOptions.get(kvOptions.get(PinotHintOptions.TableHintOptions.PARTITION_KEY)); |
| */ | ||
| private static ImmutableTableOptions overridePartitionFunction(ImmutableTableOptions base, | ||
| TableScan tableScan, Map<String, String> kvOptions) { | ||
| String partitionFunction = kvOptions.get(kvOptions.get(PinotHintOptions.TableHintOptions.PARTITION_FUNCTION)); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19003 +/- ##
============================================
+ Coverage 64.79% 65.26% +0.46%
- Complexity 1318 1405 +87
============================================
Files 3393 3419 +26
Lines 211566 214704 +3138
Branches 33285 33932 +647
============================================
+ Hits 137091 140129 +3038
+ Misses 63398 63275 -123
- Partials 11077 11300 +223
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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 fixes two bugs in
PinotImplicitTableHintRule(the rule that inferstableOptionspartition hints wheninferPartitionHintis enabled via thepinot.broker.multistage.infer.partition.hintbroker config or theinferPartitionHintquery option):1. Explicit non-partition hint options (e.g.
is_replicated) were dropped during inferencewithNewTableOptions()rebuilt thetableOptionshint with only the partition options (partition_key/partition_function/partition_size/partition_parallelism), silently discarding any other explicitly supplied options — notablyis_replicated='true'.As a result, a replicated local join like:
fails during worker assignment with
Found multiple local exchanges in the childrenwhen partition hint inference is enabled, because the replicated leaf loses itsis_replicatedflag and itsSINGLETONsend is counted as a second local exchange inWorkerManager.The rebuilt hint's kv-options are now seeded from the explicit hint so that options not modeled by
TableOptionsare carried over, and the merged (inferred + explicitly overridden) partition options overwrite the explicitly supplied ones.2. Explicit
partition_key/partition_functionoverrides of inferred options were never appliedoverridePartitionKey()andoverridePartitionFunction()did a double map lookup —kvOptions.get(kvOptions.get(PARTITION_KEY))— which looks up the option value as a key and therefore always resolves tonull, so partial explicit overrides of the inferred partition key/function were silently ignored (the documented contract is "any explicit hint will override the implicit hint").Note on behavior: this is a user-visible behavior change for deployments running with partition hint inference enabled — a partial
tableOptionshint carrying an incorrectpartition_key/partition_functionwas previously ignored (the query planned with the inferred values), and will now be honored and fail planning with a validation error (e.g.Partition key: col1 does not match partition column: col2) until the hint is corrected or removed.Tests
Planner regression tests (all fail without the fix):
JoinPlans.json: the replicated local join above withSET inferPartitionHint=true, plus a variant where the hint also carriespartition_parallelism, both asserting the plan and successful worker assignment.PinotHintablePlans.json: partial-hint override cases withSET inferPartitionHint=true— a wrongpartition_key/partition_functionnow fails planning with the mismatch error, and a matchingpartition_keyplans successfully.