Skip to content

Hive 29574 merge join poc#6456

Open
illiabarbashov-sketch wants to merge 1 commit into
apache:masterfrom
illiabarbashov-sketch:HIVE-29574_merge_join_poc
Open

Hive 29574 merge join poc#6456
illiabarbashov-sketch wants to merge 1 commit into
apache:masterfrom
illiabarbashov-sketch:HIVE-29574_merge_join_poc

Conversation

@illiabarbashov-sketch

@illiabarbashov-sketch illiabarbashov-sketch commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

  1. New Hive configurations: hive.merge.join.skew.threshold and hive.merge.join.skew.abort
  2. A new SkewedMergeJoinMonitor class to manage those configurations and log skew join event or abort it.
  3. Unit tests to cover positive and negative cases
  4. Query tests to cover clientpositive and clientnegative test cases

Why are the changes needed?

  1. This feature adds observability to merge join operator and flags the skewed keys
  2. Clients have problem with Skewed Merge Join when the job is stuck and there are no progress and no information about the reasons behind this issue. This feature adds a configuration to abort the stuck job if the threshold is hit.

Does this PR introduce any user-facing change?

No

How was this patch tested?

  • Unit tests
  • Query tests with clientpositive and clientnegative cases

@sonarqubecloud

sonarqubecloud Bot commented May 20, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
52 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

abstractdog
abstractdog requested changes May 26, 2026
View reviewed changes

@abstractdog abstractdog left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice patch @illiabarbashov-sketch so far, let some comments

Comment thread ql/src/java/org/apache/hadoop/hive/ql/exec/tez/monitoring/SkewedMergeJoinMonitor.java Outdated
Comment thread ql/src/test/queries/clientnegative/mergejoin_skew_abort.q Outdated
Comment thread ql/src/test/queries/clientpositive/mergejoin_skew_warn.q Outdated
Comment thread ql/src/test/queries/clientpositive/mergejoin_skew_warn.q Outdated
Comment thread ql/src/test/queries/clientnegative/mergejoin_skew_abort.q Outdated
Comment thread ql/src/test/queries/clientpositive/mergejoin_skew_warn.q Outdated
Comment thread ql/src/test/queries/clientpositive/mergejoin_skew_warn.q Outdated
Comment thread ql/src/test/queries/clientnegative/mergejoin_skew_abort.q Outdated
Comment thread common/src/java/org/apache/hadoop/hive/conf/HiveConf.java Outdated
Comment thread common/src/java/org/apache/hadoop/hive/conf/HiveConf.java Outdated
@illiabarbashov-sketch illiabarbashov-sketch force-pushed the HIVE-29574_merge_join_poc branch from d9bc666 to f97785a Compare May 29, 2026 10:29
@abstractdog abstractdog self-requested a review June 8, 2026 11:42
abstractdog
abstractdog requested changes Jun 8, 2026
View reviewed changes

@abstractdog abstractdog left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left minor comments, also the last run has a related failure:
https://ci.hive.apache.org/blue/organizations/jenkins/hive-precommit/detail/PR-6456/12/tests/

other than that, this is quite close! 🤞

Comment thread ...st/org/apache/hadoop/hive/ql/optimizer/metainfo/query/TestJoinOperationMetadataResolver.java Outdated
Comment on lines +21 to +22
import org.apache.hadoop.hive.ql.exec.*;
import org.apache.hadoop.hive.ql.plan.*;

@abstractdog abstractdog Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we tend to extract wildcard imports to one-by-one

Comment thread ...st/org/apache/hadoop/hive/ql/optimizer/metainfo/query/TestJoinOperationMetadataResolver.java Outdated
Comment thread ...st/org/apache/hadoop/hive/ql/optimizer/metainfo/query/TestJoinOperationMetadataResolver.java Outdated
Comment thread ...st/org/apache/hadoop/hive/ql/optimizer/metainfo/query/TestJoinOperationMetadataResolver.java Outdated
Comment thread ...st/org/apache/hadoop/hive/ql/optimizer/metainfo/query/TestJoinOperationMetadataResolver.java Outdated
Comment thread ...st/org/apache/hadoop/hive/ql/optimizer/metainfo/query/TestJoinOperationMetadataResolver.java Outdated
Comment thread ...st/org/apache/hadoop/hive/ql/optimizer/metainfo/query/TestJoinOperationMetadataResolver.java Outdated
Comment thread ...st/org/apache/hadoop/hive/ql/optimizer/metainfo/query/TestJoinOperationMetadataResolver.java Outdated
ReduceSinkDesc rsConf = mock(ReduceSinkDesc.class);
when(rsConf.getKeyCols()).thenReturn(Collections.singletonList(keyExpr));

@SuppressWarnings("unchecked") ReduceSinkOperator rs = mock(ReduceSinkOperator.class);

@abstractdog abstractdog Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most style guides and formatters (including Google Java Format) will place the annotation on its own line for readability, I recommend doing the same here

Comment thread ql/src/java/org/apache/hadoop/hive/ql/exec/tez/monitoring/SkewedMergeJoinMonitor.java Outdated
Comment thread ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java Outdated
String[] descTableAliases = conf.getSkewJoinTableAliases();

for (int pos = 0; pos < maxAlias; pos++) {
joinSkewKeyColumns[pos] = (descKeyNames != null && pos < descKeyNames.length

@abstractdog abstractdog Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can still see the same pattern here, can you double-check, please?

      joinSkewKeyColumns[pos]  = (descKeyNames    != null && pos < descKeyNames.length
          && descKeyNames[pos] != null)    ? descKeyNames[pos]    : "unknown";

@abstractdog

abstractdog commented Jun 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

there are some warnings in sonarqube report:
https://sonarcloud.io/project/issues?sinceLeakPeriod=true&issueStatuses=OPEN%2CCONFIRMED&pullRequest=6456&id=apache_hive&open=AZ6On0YSJoVnC2vLVdC3

some of them can be fixed easily, like Line is longer than 120 characters (found 152). , '{' is followed by whitespace., or Using the '.*' form of import should be avoided, try to fix them where it makes sense to you, sorry that I haven't caught them earlier

@illiabarbashov-sketch illiabarbashov-sketch force-pushed the HIVE-29574_merge_join_poc branch from f8348ed to 09d4440 Compare June 11, 2026 13:01
abstractdog
abstractdog requested changes Jun 25, 2026
View reviewed changes

@abstractdog abstractdog left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments @illiabarbashov-sketch , otherwise this looks good to me, almost ready :)

Comment thread ...st/org/apache/hadoop/hive/ql/optimizer/metainfo/query/TestJoinOperationMetadataResolver.java
private JoinOperationMetadataResolver resolver;

@BeforeEach
void setUp() {

@abstractdog abstractdog Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it's strange for me that these unit tests are not public, I got used to see it, can you please fix

@abstractdog abstractdog Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I just got to know: it's fine as is in case of Junit5 and Sonar complains

Comment thread ql/src/test/results/clientnegative/mergejoin_skew_abort.q.out Outdated
Caused by: org.apache.hadoop.hive.ql.metadata.HiveException: Data skew detected in merge join: 2 rows accumulated for join column(s) [testcolkey_a] in table alias [a]. Consider reviewing data distribution.
#### A masked pattern was here ####
]], Vertex did not succeed due to OWN_TASK_FAILURE, failedTasks:1 killedTasks:0, Vertex vertex_#ID# [Reducer 2] killed/failed due to:OWN_TASK_FAILURE]
DAG did not succeed due to VERTEX_FAILURE. failedVertices:1 killedVertices:0

@abstractdog abstractdog Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is going to be masked after HIVE-29656, so needs to be changed here

@sonarqubecloud

sonarqubecloud Bot commented Jun 29, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abstractdog abstractdog Awaiting requested review from abstractdog

Assignees

No one assigned

Labels

tests unstable

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@illiabarbashov-sketch @abstractdog @asf-ci-hive