HIVE-29702: Reduce-side merge join can produce NULL values or fail with IllegalStateException under Tez container reuse#6586
Open
ryukobayashi wants to merge 3 commits into
Open
Conversation
…n failures on Tez container reuse
…g the same merge join
|
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.



What changes were proposed in this pull request?
This PR fixes two related defects in Hive's reduce-side merge join (SMB / DummyStore +
CommonMergeJoinOperatorpipeline) on Tez:TezDummyStoreOperator#closeOpdid not remove itself from its child'sparentOperators, nor clear its ownchildOperators, when closing. When Hive's per-queryObjectCachehands a subsequent task the sameDummyStoreOperator/CommonMergeJoinOperatorinstances a previous task (sharing the same JVM/container) already used, the stale child reference causesReduceRecordProcessor#getJoinParentOpto walk into the wrong operator subtree and throwIllegalStateException: Was expecting dummy store operator but found: ....ReduceRecordProcessor#close()also closed the mainreducerbefore themergeWorkList(DummyStore chain). SinceDummyStoreOperator#closeOpis what removes the stale parent reference from theCommonMergeJoinOperatorside, closing the main reducer first could preventCommonMergeJoinOperator#allInitializedParentsAreClosed()from detecting all parents as closed in the correct order to flush the final join group.GenTezUtils#createReduceWorksets eachReduceWork'snumReduceTaskspurely from that branch's ownReduceSinkOperator, with no cross-check against siblingReduceSinkOperators that feed the same downstreamCommonMergeJoinOperator.SetReducerParallelismassigns eachReduceSinkOperator's reducer count/traits independently, so two siblings feeding the same merge join can end up with differentnumReducers(and different partitioning behavior, since theUNIFORMtrait also affectsReduceWork#setUniformDistribution), with no reconciliation between them.A new
normalizeMergeJoinReducershelper walks from aReduceSinkOperatordownstream to find aCommonMergeJoinOperator, then walks upstream from each of the merge join's parents to find their originatingReduceSinkOperators, and normalizes all of them to the same (maximum)numReducers, clearingAUTOPARALLEL/UNIFORMtraits so Tez's dynamic auto-parallelism cannot independently shrink one side's task count at runtime.Why are the changes needed?
IllegalStateException) when Tez reuses a container across multiple tasks of the same reducer vertex.Does this PR introduce any user-facing change?
No
How was this patch tested?
TestTezDummyStoreOperator(unit test) for (1): verifiescloseOpcorrectly removes stale parent/child references. Fails against the pre-fix code (3/5 assertions), passes after the fix.tez_dummystore_reduce_side_reuse.q(TestMiniTezCliDriver) for (1): forces Tez to reuse a container across multiple tasks of the same reducer vertex. Reproduces the exactIllegalStateExceptionagainst the pre-fix code; passes cleanly after the fix.TestGenTezUtilsMergeJoinNumReducers(unit test) for (2): drivesGenTezUtils#createReduceWorkdirectly for two syntheticReduceSinkOperators (one feeding the merge join through aGroupByOperator, the other through aDummyStoreOperator) with differentnumReducers. Fails against the pre-fix code (the two resultingReduceWorks keep their original, mismatchednumReduceTasks), passes after the fix..qtest was added for (2): the mismatch only manifests through Tez's runtime dynamic-parallelism shrinking (ShuffleVertexManager) at realistic data volumes, which could not be reproduced deterministically at qtest scale without either breaking the reduce-side-merge-join plan shape or triggering unrelated flush-ordering issues in synthetic query shapes tried. The unit test above exercises the exact defect directly and deterministically instead.