HIVE-25822: Unexpected result rows in case of outer join contains conditions only affecting one side #2891

kgyrtkirk · 2021-12-18T19:32:12Z

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

zabetak · 2021-12-19T11:48:18Z

common/src/java/org/apache/hadoop/hive/conf/HiveConf.java

+    HIVE_JOIN_SHORTCUT_EXTENSIONROWS("hive.join.shortcut.extensionrows", true,
+        "Enables to shortcut processing of known filtered rows in merge joins."),


If my understanding is correct if this is set to false the outer join produces incorrect results. I don't think it is a good idea to have a property which determines the correctness of the results.

idea was to add this toggle to make it possible to check if this feature is causing any perf regression - and make it possible to validate that quickly

I see your point but I still think that it would be better to avoid toggles when it comes to correctness issues. I guess it is not the first time we are doing this so I am not gonna push back on this but it shouldn't be the norm. I am leaving the final judgement up to you.

If it goes in I would add some extra information that it is for "internal use only" and changing the value can lead to incorrect results.

Agree that this config is needed for performance testing. At the same time, agree that it should be marked for internal use. I see there's precedent ..there are a few config options with 'Internal use only' in the description.

zabetak · 2021-12-19T11:52:13Z

ql/src/test/results/clientpositive/llap/join_1to1.q.out

-NULL	NULL	NULL	50	10050	66
-NULL	NULL	NULL	50	10050	66
-NULL	NULL	NULL	50	10050	88


Indeed these rows shouldn't be there. It's a pity that people update q.out files without paying attention if the resulting changes are correct. We could have found this bug much earlier if people verified the results against another DBMS as well.

yes; overwrite without checking tends to happen - that's why I see no value in the tests which use the src table - which content is not defined at all - one way around these problems is to use assert_true ...

I am not a big fan of the the src and similar tables but if people validated the results against another DBMS I can leave with it.

zabetak · 2021-12-19T11:58:54Z

ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java

+   * Decides if the actual row must be an extension row.
+   *
+   * Extension rows are those which are not part of the inner-join.
+   * May not correctly identify all extension rows - but will remove trivially filtered ones.


May not correctly... It is not clear what this means. Possibly an example could help clarify this better.
What happens in the case where it does not identify an extension row? Wrong results?

May not identify all extension rows

it could not identify non-trivial ones like that no rows could satisfy the l.id=r.id and l.id+2*r.id=1 condition...
guess what...running:

select * from t_xy l full outer join t_y r on (l.id=r.id and l.s='y' and l.id+2*r.id=1);

resulted in incorrect results as well...

this will leave me no choice but to remove that skip crap and replace it with something decent....

Since you have examples that do not work, maybe it would be better to write "The current implementation, does not correctly identify all..." and put a reference to a follow up JIRA highlighting that there is an implementation limitation.

zabetak

LGTM, I left a few comments but most of them are minor. It would good though if someone more familiar with this code can do another pass.

zabetak · 2021-12-20T09:09:44Z

common/src/java/org/apache/hadoop/hive/conf/HiveConf.java

+    HIVE_JOIN_SHORTCUT_EXTENSIONROWS("hive.join.shortcut.extensionrows", true,
+        "Enables to shortcut processing of known filtered rows in merge joins."),


I see your point but I still think that it would be better to avoid toggles when it comes to correctness issues. I guess it is not the first time we are doing this so I am not gonna push back on this but it shouldn't be the norm. I am leaving the final judgement up to you.

If it goes in I would add some extra information that it is for "internal use only" and changing the value can lead to incorrect results.

zabetak · 2021-12-20T09:13:52Z

ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java


  // A field because we cannot multi-inherit.
  transient InterruptibleProcessing interruptChecker;
+  transient boolean shortcutExtensionRows;


Minor: should we use unmatched everywhere we use extension?

"Unmatched rows of an outer join" sounds more natural than "Extension rows of an outer join".

zabetak · 2021-12-20T09:21:13Z

ql/src/test/results/clientpositive/llap/join_1to1.q.out

-NULL	NULL	NULL	50	10050	66
-NULL	NULL	NULL	50	10050	66
-NULL	NULL	NULL	50	10050	88


I am not a big fan of the the src and similar tables but if people validated the results against another DBMS I can leave with it.

ql/src/test/results/clientpositive/llap/join_1to1.q.out

zabetak · 2021-12-20T09:42:59Z

ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java

+   * Decides if the actual row must be an extension row.
+   *
+   * Extension rows are those which are not part of the inner-join.
+   * May not correctly identify all extension rows - but will remove trivially filtered ones.


Since you have examples that do not work, maybe it would be better to write "The current implementation, does not correctly identify all..." and put a reference to a follow up JIRA highlighting that there is an implementation limitation.

This reverts commit 8aeb731.

This reverts commit 7f33728.

amansinha100 · 2021-12-20T16:05:23Z

ql/src/java/org/apache/hadoop/hive/ql/exec/CommonMergeJoinOperator.java

+    if (shortcutExtensionRows && isOuterJoinExtensionRow(tag, value)) {
+      int type = condn[0].getType();
+      if (tag == 0 && (type == JoinDesc.LEFT_OUTER_JOIN || type == JoinDesc.FULL_OUTER_JOIN)) {
+        emitExtensionRow(tag, value);


I applied this patch and added a log message to emitExtensionRow() and ran queries with full , left and right outer join (with the extra single table predicate in ON clause) but only see the log msg for full outer join, not for left/right. It is unclear which situations would lead to extension rows for left/right outer join.

it have stopped for me for other outer joins ; however I had to disable auto.join.converion to have that - without that the join was converted into a mapjoin
I will make some further tests for different types of joins to see if we don't have any issues

…ditions only affecting one side (apache#2891) (Zoltan Haindrich reviewed by Stamatis Zampetakis, Aman Sinha)

kgyrtkirk added 4 commits December 18, 2021 19:15

patch

3614080

organize

4f5cfcf

add q.out

2daabdf

update

bd12f32

kgyrtkirk added tests pending tests unstable and removed tests pending labels Dec 18, 2021

kgyrtkirk added 2 commits December 18, 2021 22:43

row order changes

741a6a3

full-outer; most likely bad

b68703b

kgyrtkirk added tests pending tests passed and removed tests unstable tests pending labels Dec 19, 2021

kgyrtkirk requested review from abstractdog and kasakrisz December 19, 2021 06:43

zabetak reviewed Dec 19, 2021

View reviewed changes

zabetak approved these changes Dec 20, 2021

View reviewed changes

kgyrtkirk added 7 commits December 20, 2021 10:50

add

8aeb731

Revert "add"

7f33728

This reverts commit 8aeb731.

Revert "Revert "add""

be274a8

This reverts commit 7f33728.

add load

a9539fc

undo change

692a447

updates

1d8c760

updates

567c74d

kgyrtkirk added tests pending tests failed and removed tests passed tests pending tests failed labels Dec 20, 2021

amansinha100 reviewed Dec 20, 2021

View reviewed changes

add back feature toggle

ce0cdf9

kgyrtkirk added tests failed tests pending tests passed and removed tests pending tests failed labels Dec 20, 2021

kgyrtkirk merged commit 621bfd1 into apache:master Dec 21, 2021

dengzhhu653 pushed a commit to dengzhhu653/hive that referenced this pull request Dec 15, 2022

HIVE-25822: Unexpected result rows in case of outer join contains con…

5e09ec7

…ditions only affecting one side (apache#2891) (Zoltan Haindrich reviewed by Stamatis Zampetakis, Aman Sinha)

		HIVE_JOIN_SHORTCUT_EXTENSIONROWS("hive.join.shortcut.extensionrows", true,
		"Enables to shortcut processing of known filtered rows in merge joins."),

HIVE-25822: Unexpected result rows in case of outer join contains conditions only affecting one side #2891

HIVE-25822: Unexpected result rows in case of outer join contains conditions only affecting one side #2891

Uh oh!

Conversation

kgyrtkirk commented Dec 18, 2021

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zabetak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants