You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/02/26 09:40:00 UTC
[GitHub] [calcite] rubenada opened a new pull request #1833: [CALCITE-3828]
MergeJoin throws NPE in case of null keys
rubenada opened a new pull request #1833: [CALCITE-3828] MergeJoin throws NPE in case of null keys
URL: https://github.com/apache/calcite/pull/1833
Jira ticket: [CALCITE-3828](https://issues.apache.org/jira/browse/CALCITE-3828)
EnumerableDefaults#MergeJoinEnumerator throws a NPE in case of null keys.
MergeJoin algorithm assumes that inputs sorted in ascending order, with nulls last (see EnumerableMergeJoinRule), so when reaching a null key on the left / right side means that we are done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1833:
[CALCITE-3828] MergeJoin throws NPE in case of null keys
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1833: [CALCITE-3828] MergeJoin throws NPE in case of null keys
URL: https://github.com/apache/calcite/pull/1833#discussion_r384457610
##########
File path: linq4j/src/main/java/org/apache/calcite/linq4j/EnumerableDefaults.java
##########
@@ -3829,6 +3829,12 @@ private boolean advance() {
TInner right = rightEnumerator.current();
TKey rightKey = innerKeySelector.apply(right);
for (;;) {
+ // mergeJoin assumes inputs sorted in ascending order with nulls last,
Review comment:
The nulls order is actually defined by the `RelFieldCollation`, did we set it anywhere ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1833:
[CALCITE-3828] MergeJoin throws NPE in case of null keys
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1833: [CALCITE-3828] MergeJoin throws NPE in case of null keys
URL: https://github.com/apache/calcite/pull/1833#discussion_r384881686
##########
File path: linq4j/src/main/java/org/apache/calcite/linq4j/EnumerableDefaults.java
##########
@@ -3829,6 +3829,12 @@ private boolean advance() {
TInner right = rightEnumerator.current();
TKey rightKey = innerKeySelector.apply(right);
for (;;) {
+ // mergeJoin assumes inputs sorted in ascending order with nulls last,
Review comment:
Reasonable, can you add this comments into the EnumerableDefaults code block.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] rubenada commented on a change in pull request #1833:
[CALCITE-3828] MergeJoin throws NPE in case of null keys
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #1833: [CALCITE-3828] MergeJoin throws NPE in case of null keys
URL: https://github.com/apache/calcite/pull/1833#discussion_r384459011
##########
File path: linq4j/src/main/java/org/apache/calcite/linq4j/EnumerableDefaults.java
##########
@@ -3829,6 +3829,12 @@ private boolean advance() {
TInner right = rightEnumerator.current();
TKey rightKey = innerKeySelector.apply(right);
for (;;) {
+ // mergeJoin assumes inputs sorted in ascending order with nulls last,
Review comment:
It's set by EnumerableMergeJoinRule:
https://github.com/apache/calcite/blob/e427180b6a55445fe246b00c60259a95f96bdbf2/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableMergeJoinRule.java#L72
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] rubenada commented on a change in pull request #1833:
[CALCITE-3828] MergeJoin throws NPE in case of null keys
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #1833: [CALCITE-3828] MergeJoin throws NPE in case of null keys
URL: https://github.com/apache/calcite/pull/1833#discussion_r384456714
##########
File path: linq4j/src/main/java/org/apache/calcite/linq4j/EnumerableDefaults.java
##########
@@ -3829,6 +3829,12 @@ private boolean advance() {
TInner right = rightEnumerator.current();
TKey rightKey = innerKeySelector.apply(right);
for (;;) {
+ // mergeJoin assumes inputs sorted in ascending order with nulls last,
Review comment:
Comments have been added.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1833:
[CALCITE-3828] MergeJoin throws NPE in case of null keys
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1833: [CALCITE-3828] MergeJoin throws NPE in case of null keys
URL: https://github.com/apache/calcite/pull/1833#discussion_r384445110
##########
File path: linq4j/src/main/java/org/apache/calcite/linq4j/EnumerableDefaults.java
##########
@@ -3829,6 +3829,12 @@ private boolean advance() {
TInner right = rightEnumerator.current();
TKey rightKey = innerKeySelector.apply(right);
for (;;) {
+ // mergeJoin assumes inputs sorted in ascending order with nulls last,
Review comment:
Is it documented anywhere?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] chunweilei commented on a change in pull request #1833:
[CALCITE-3828] MergeJoin throws NPE in case of null keys
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #1833: [CALCITE-3828] MergeJoin throws NPE in case of null keys
URL: https://github.com/apache/calcite/pull/1833#discussion_r384433566
##########
File path: linq4j/src/main/java/org/apache/calcite/linq4j/EnumerableDefaults.java
##########
@@ -3829,6 +3829,12 @@ private boolean advance() {
TInner right = rightEnumerator.current();
TKey rightKey = innerKeySelector.apply(right);
for (;;) {
+ // mergeJoin assumes inputs sorted in ascending order with nulls last,
+ // if we reach a null key, we are done
+ if (leftKey == null || rightKey == null) {
+ done = true;
Review comment:
```we are done``` -> ```we are done.``` ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] rubenada merged pull request #1833: [CALCITE-3828]
MergeJoin throws NPE in case of null keys
Posted by GitBox <gi...@apache.org>.
rubenada merged pull request #1833: [CALCITE-3828] MergeJoin throws NPE in case of null keys
URL: https://github.com/apache/calcite/pull/1833
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] rubenada commented on a change in pull request #1833:
[CALCITE-3828] MergeJoin throws NPE in case of null keys
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #1833: [CALCITE-3828] MergeJoin throws NPE in case of null keys
URL: https://github.com/apache/calcite/pull/1833#discussion_r384459011
##########
File path: linq4j/src/main/java/org/apache/calcite/linq4j/EnumerableDefaults.java
##########
@@ -3829,6 +3829,12 @@ private boolean advance() {
TInner right = rightEnumerator.current();
TKey rightKey = innerKeySelector.apply(right);
for (;;) {
+ // mergeJoin assumes inputs sorted in ascending order with nulls last,
Review comment:
It's set by EnumerableMergeJoinRule:
https://github.com/apache/calcite/blob/e427180b6a55445fe246b00c60259a95f96bdbf2/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableMergeJoinRule.java#L72
which forces EnumerableMergeJoin's inputs to be sorted by key ascending, nulls last.
So, this sort collation is assumed when we arrive to EnumerableDefaults#MergeJoinEnumerator (note that there is already some IllegalStateException in there if inputs are not in ascending order)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] rubenada commented on a change in pull request #1833:
[CALCITE-3828] MergeJoin throws NPE in case of null keys
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #1833: [CALCITE-3828] MergeJoin throws NPE in case of null keys
URL: https://github.com/apache/calcite/pull/1833#discussion_r384452576
##########
File path: linq4j/src/main/java/org/apache/calcite/linq4j/EnumerableDefaults.java
##########
@@ -3829,6 +3829,12 @@ private boolean advance() {
TInner right = rightEnumerator.current();
TKey rightKey = innerKeySelector.apply(right);
for (;;) {
+ // mergeJoin assumes inputs sorted in ascending order with nulls last,
Review comment:
I'll add a comment about it in the javadoc of EnumerableDefaults#mergeJoin and EnumerableDefaults#MergeJoinEnumerator
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] rubenada commented on a change in pull request #1833:
[CALCITE-3828] MergeJoin throws NPE in case of null keys
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #1833: [CALCITE-3828] MergeJoin throws NPE in case of null keys
URL: https://github.com/apache/calcite/pull/1833#discussion_r384975567
##########
File path: linq4j/src/main/java/org/apache/calcite/linq4j/EnumerableDefaults.java
##########
@@ -3829,6 +3829,12 @@ private boolean advance() {
TInner right = rightEnumerator.current();
TKey rightKey = innerKeySelector.apply(right);
for (;;) {
+ // mergeJoin assumes inputs sorted in ascending order with nulls last,
Review comment:
@danny0405 , comment has been added in the javadoc of EnumerableDefaults#mergeJoin and EnumerableDefaults#MergeJoinEnumerator
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services