You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "soumyakanti3578 (via GitHub)" <gi...@apache.org> on 2025/11/04 04:47:34 UTC
[PR] HIVE-26653: Wrong results when (map) joining multiple tables on parti… [hive]
soumyakanti3578 opened a new pull request, #6165:
URL: https://github.com/apache/hive/pull/6165
…tion column
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
In INNER joins, when we only project the small table's joining key, we can run into a situation when the hashmap's value is empty. Then, if we serialize the empty value, we will get `NULL`s. Instead we should just copy the `key` into the vectorized batch.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
Explained in detail: https://issues.apache.org/jira/browse/HIVE-26653
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
Added unit tests in `TestMapJoinOperator.java`. The original issue is not reproducible anymore because of an unrelated patch, as explained in the Jira.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-26653: Wrong results when (map) joining multiple tables on parti… [hive]
Posted by "soumyakanti3578 (via GitHub)" <gi...@apache.org>.
soumyakanti3578 closed pull request #6165: HIVE-26653: Wrong results when (map) joining multiple tables on parti…
URL: https://github.com/apache/hive/pull/6165
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-26653: Wrong results when (map) joining multiple tables on parti… [hive]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #6165:
URL: https://github.com/apache/hive/pull/6165#discussion_r2549975690
##########
ql/src/test/org/apache/hadoop/hive/ql/exec/vector/mapjoin/MapJoinTestData.java:
##########
@@ -297,14 +297,25 @@ public static void generateVariationData(MapJoinTestData testData,
private static RowTestObjects generateRandomSmallTableValueRow(MapJoinTestDescription testDesc, Random random) {
+ final ValueOption valueOption = testDesc.getSmallTableGenerationParameters().getValueOption();
final int columnCount = testDesc.smallTableValueTypeInfos.length;
PrimitiveTypeInfo[] primitiveTypeInfos = new PrimitiveTypeInfo[columnCount];
for (int i = 0; i < columnCount; i++) {
primitiveTypeInfos[i] = (PrimitiveTypeInfo) testDesc.smallTableValueTypeInfos[i];
}
- Object[] smallTableValueRow =
- VectorRandomRowSource.randomWritablePrimitiveRow(
- columnCount, random, primitiveTypeInfos);
+ Object[] smallTableValueRow;
+
+ // Generate empty value row if specified.
+ if (valueOption == ValueOption.EMPTY_VALUE) {
+ smallTableValueRow = new Object[columnCount];
+ for (int c = 0; c < columnCount; c++) {
+ smallTableValueRow[c] =
+ VectorizedBatchUtil.getPrimitiveWritable(primitiveTypeInfos[c].getPrimitiveCategory());
Review Comment:
Which real use-case is this code trying to simulate? Is this equivalent to having `null` values in the data or something else. Basically, I am trying to understand under what circumstances we have these values in the table.
From a quick look, it seems that we are using special values (e.g., `new Text(ArrayUtils.EMPTY_BYTE_ARRAY)`) but not really nulls. If that's the case then I don't see why we need to handle this separately from `VectorRandomRowSource.randomWritablePrimitiveRow`. Wouldn't it make more sense to tune the random generator to occasioanlly generate this "special" values if they can really appear in practice?
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/mapjoin/VectorMapJoinGenerateResultOperator.java:
##########
@@ -226,6 +223,29 @@ protected int generateHashMapResultSingleValue(VectorizedRowBatch batch,
return numSel;
}
+ private void updateBatchWithSmallTableValue(
+ VectorizedRowBatch batch, VectorMapJoinHashMapResult hashMapResult, int batchIndex, ByteSegmentRef byteSegmentRef)
+ throws HiveException {
+
+ // Check if the small table value is empty.
+ boolean isSmallTableValueEmpty = byteSegmentRef.getLength() == 0;
Review Comment:
The fact that we need to check the actual data in order to decide how to evaluate the join (or rather the creation of the resulting row) is somewhat suspicious and a bit brittle. Ideally, the compiler should be able to determine exactly how the operator should behave via the query plan. Can we exploit (or add) information in the query plan in order to drive the copy decision below?
##########
ql/src/test/org/apache/hadoop/hive/ql/exec/vector/mapjoin/TestMapJoinOperator.java:
##########
@@ -1512,8 +1605,17 @@ private RowTestObjectsMultiSet createExpectedTestRowMultiSet(MapJoinTestDescript
Object[] valueRow = valueList.get(v).getRow();
final int smallTableRetainValueColumnNumsLength =
testDesc.smallTableRetainValueColumnNums.length;
+
+ // When EMPTY_VALUE is specified, the small table value columns are not
+ // actually stored in the small table HashMap. So we have to simulate that here.
+ // The expected output should have values from the big table key columns.
+ final boolean isEmptyValue =
+ testDesc.smallTableGenerationParameters.getValueOption() == ValueOption.EMPTY_VALUE &&
+ testDesc.smallTableRetainValueColumnNums.length > 0 &&
+ testDesc.smallTableRetainValueColumnNums.length == testDesc.bigTableKeyColumnNums.length;
Review Comment:
This simulation is problematic cause it makes the solution and the test code somewhat identical. We're implementing a copy logic in two places (prod & test) so the tests will trivially pass as it is right now and immediately fail if the implementation changes in the future.
##########
ql/src/test/org/apache/hadoop/hive/ql/exec/vector/mapjoin/MapJoinTestDescription.java:
##########
@@ -44,7 +45,8 @@ public static class SmallTableGenerationParameters {
public static enum ValueOption {
NO_RESTRICTION,
ONLY_ONE,
- NO_REGULAR_SMALL_KEYS
+ NO_REGULAR_SMALL_KEYS,
+ EMPTY_VALUE, // Generate empty value entries.
Review Comment:
By going over the code, I get the impression that the `ValueOption` enumeration is about the generation of the keys of the small table **not** the values. Mixing the two creates confusion and makes the code harder to understand.
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/mapjoin/VectorMapJoinCommonOperator.java:
##########
@@ -596,6 +603,16 @@ protected void initializeOp(Configuration hconf) throws HiveException {
outerSmallTableKeyVectorCopy.init(outerSmallTableKeyMapping);
}
+ // Checks if the plan is projecting values from the SmallTable
+ // AND if the number of projected values matches the number of join keys.
+ if (smallTableValueMapping.getCount() > 0 &&
+ smallTableValueMapping.getCount() == bigTableKeyColumnMap.length) {
Review Comment:
I don't understand the reasoning/intuition behind this check. Why do we care about values and keys being of the same length?
##########
ql/src/test/org/apache/hadoop/hive/ql/exec/vector/mapjoin/TestMapJoinOperator.java:
##########
@@ -1343,6 +1349,93 @@ public void testString2() throws Exception {
} while (!hiveConfVariationsDone);
}
+ /**
+ * Test case for INNER vector map join with only small table key projection - string type.
+ *
+ * @throws Exception Exception
+ */
+ @Test
+ public void testSmallTableKeyOnlyProjectionWithEmptyValueString() throws Exception {
Review Comment:
Adding tests in this class are useful but it may not be the best option in every situation. These tests depend on random generation of input/output and they are good for covering general behavior of the joins operators but for edge cases and very specific bugs having fixed input & output and join configuration would be much easier to reason about.
For showcasing the bug in this PR (if there is one), it would really help to have a dedicated test case possibly in another class and have well-defined and minimal input/output and join settings. Then we can discuss if we also need these randomized tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-26653: Wrong results when (map) joining multiple tables on parti… [hive]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #6165:
URL: https://github.com/apache/hive/pull/6165#issuecomment-3484026938
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6165) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6165&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6165&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=6165&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6165&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=6165&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=6165)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-26653: Wrong results when (map) joining multiple tables on parti… [hive]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #6165:
URL: https://github.com/apache/hive/pull/6165#discussion_r2550031108
##########
ql/src/test/org/apache/hadoop/hive/ql/exec/vector/mapjoin/TestMapJoinOperator.java:
##########
@@ -1343,6 +1349,93 @@ public void testString2() throws Exception {
} while (!hiveConfVariationsDone);
}
+ /**
+ * Test case for INNER vector map join with only small table key projection - string type.
+ *
+ * @throws Exception Exception
+ */
+ @Test
+ public void testSmallTableKeyOnlyProjectionWithEmptyValueString() throws Exception {
Review Comment:
Adding tests in this class are useful but it may not be the best option in every situation. These tests depend on random generation of input/output and they are good for covering general behavior of the joins operators but for edge cases and very specific bugs having fixed input & output and join configuration would be much easier to reason about.
For showcasing the bug in this PR (if there is one), it would really help to have a dedicated test case possibly in another class and have well-defined and minimal input/output and join settings. Then we can discuss if we also need these randomized tests. The bug implies a problem in a binary join operator so we should be able to demonstrate the issue by correctly picking the schema/data for the left and right side of the join having a few rows on each side.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-26653: Wrong results when (map) joining multiple tables on parti… [hive]
Posted by "soumyakanti3578 (via GitHub)" <gi...@apache.org>.
soumyakanti3578 commented on PR #6165:
URL: https://github.com/apache/hive/pull/6165#issuecomment-3658839059
Closing PR based on discussions here and in the Jira: [HIVE-26653](https://issues.apache.org/jira/browse/HIVE-26653)
Thanks @zabetak for reviewing and highlighting potential issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org