HIVE-28480: Disable SMB on partition hash generator mismatch across join branches…

[himanshu-mishra]

… in previous RS

What changes were proposed in this pull request?

As SMB replaces last RS op from the joining branches and the JOIN op with MERGEJOIN, we need to ensure the RS before these RS, in both branches, are partitioning using same hash generator.

Hash code generator differs based on ReducerTraits.UNIFORM i.e. ReduceSinkOperator#computeMurmurHash() or ReduceSinkOperator#computeHashCode(), leading to different hash code for same value.

Skip SMB join in such cases.

Why are the changes needed?

It fixes bug that results in incorrect join result. Example query and behavior explanation is added in https://issues.apache.org/jira/browse/HIVE-28480

Does this PR introduce any user-facing change?

No

Is the change a dependency upgrade?

No

How was this patch tested?

Tested via auto_sortmerge_join_18.q added in this PR

[ngsg]

The patch seems reasonable. LGTM

[okumin]

This looks fine to me

[okumin]

I confirmed this test case reproduces the issue explained in the JIRA ticket

[okumin]

Can I assume we want to find ReduceSinkOperator-1 of Operators -> ReduceSinkOperator-1 -> Operators -> ReduceSinkOperator-2(parentOp) ->JoinOperator?

[himanshu-mishra]

Yes. The ReduceSinkOperator-2 -> JoinOperator get replaced by MERGEJOIN operator for SMB. We want to ensure the RS before these in both join branches are using same partitioning logic.

[okumin]

That makes sense. Thanks

[himanshu-mishra]

@okumin if this looks good, please merge the PR

[okumin]

@himanshu-mishra Sorry, I don't have the right to merge a PR as I am not a committer. You need approval of any committer who is ideally an expert of SMB join.

[himanshu-mishra]

@kasakrisz can you please review or add reviewer

[kasakrisz]

This is a good candidate to fix this issue, but have you checked why the reducer traits are different?

[kasakrisz]

There are several utility methods to find operators in branches of a plan in ql.exec.OperatorUtils

OperatorUtils.find*Operator*()

Is it possible to use one of them to find parent RS operators?

[himanshu-mishra]

There isn't a utility method there that returns first ancestor operator encountered from given set of classes. Here, we are only interested in ancestor RS if that is the source of data for current RS, if TS, JOIN are encountered, they are the source, and we do not need verifying them.

[kasakrisz]

a few lines earlier the same while loop exists

hive/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ConvertJoinMapJoin.java

Lines 853 to 859 in b13f529

	while (op != null && !(op instanceof TableScanOperator || op instanceof ReduceSinkOperator
	|| op instanceof CommonJoinOperator)) {
	// If op has parents it is guaranteed to be 1.
	List<Operator<?>> parents = op.getParentOperators();
	Preconditions.checkState(parents.size() == 0 || parents.size() == 1);
	op = parents.size() == 1 ? parents.get(0) : null;
	}

Could you please extract these to a utility method and move it to OperatorUtils

[himanshu-mishra]

Thanks for reviewing. I'll incorporate it.

[himanshu-mishra]

This is a good candidate to fix this issue, but have you checked why the reducer traits are different?

Consider the sample query

SELECT * FROM (
    SELECT k, COUNT(DISTINCT v), SUM(v)
    FROM t_asj_18 GROUP BY k
) a LEFT JOIN (
    SELECT k, COUNT(v)
    FROM t_asj_18 GROUP BY k
) b ON a.k = b.k; 

In this query the aggregation k, count(distinct v).. group by k leads to group by and hence following RS operator's keys to be k, v while keeping partition column k, as we are aggregating on k. In such cases where key columns and partition columns differ, the UNIFORM trait is removed refer commit.
Note that the group by keys and partition columns differ only with distinct aggregate, removing that would lead to both being a thereby having UNIFORM trait.

[himanshu-mishra]

@kasakrisz let me know if there are pending concerns.

[kasakrisz]

@kasakrisz let me know if there are pending concerns.

Sorry, I was occupied in the last few days. I left only one comment.

[sonarqubecloud]

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[kasakrisz]

+1

[himanshu-mishra]

@kasakrisz as you have approved the changes, can you help with merging the PR.