[CALCITE-3926] CannotPlanException when an empty LogicalValues requires a certain collation by rubenada · Pull Request #1918 · apache/calcite

rubenada · 2020-04-15T17:21:38Z

rubenada · 2020-04-15T17:37:26Z

Maybe the same fix should be applied to PruneEmptyRules.SORT_FETCH_ZERO_INSTANCE ...

hsyuan · 2020-04-15T17:54:49Z

core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java

+        emptyValues = emptyValues.copy(
+            emptyValues.getTraitSet().replace(((Sort) single).getCollation()),
+            Collections.emptyList());
+      }


Not only sort, but all the operators can have the same issue.
We should update emptyValues to have the same traits with single. Just collation is not enough, should be all the traits.

Maybe we should put the trait logic in RelBuilder#empty ?

we can try. But if only empty() considers the trait logic, the other methods don't, that might look odd.

Agree with @hsyuan . This trait replacement is a one-off thing just for PruneEmptyRule. I am not convinced it should be available in RelBuilder.

Done. Trait propagation applied to all RemoveEmptySingleRule instances (AGGREGATE, FILTER, PROJECT, SORT) + SORT_FETCH_ZERO_INSTANCE.
What about the rest of PruneEmptyRules (MINUS, UNION, INTERSECT, JOIN_LEFT, JOIN_RIGHT)?

I'm confused, EmptyValues has no data, so what's the meaning of collation and distribution on it ?

Collation is required in order to solve this bug, since we are removing the Sort:
Sort + EmptyValues => EmptyValues
if the resulting empty values does not have the sort's collation, we get a CannotPlanException. See https://issues.apache.org/jira/browse/CALCITE-3926 for more details.
Therefore, in order to fix this problem it seems clear that we need to propagate the collation into the EmptyValues. Another discussion would be what about other traits, like distribution....

Therefore, in order to fix this problem it seems clear that we need to propagate the collation into the EmptyValues.

Not really from my side, to me, it seems that the traits check should be tweaked for EmptyValues, it should satisfy all the required traits, the EmptyValues itself does nothing wrong.

Thanks an interesting approach @danny0405 . I guess we could tweak EmptyValues to satisfy any collation... will work on that

Unfortunately, I cannot find a clean way of tweaking EmptyValues' collation to satisfy any collation. Since propagating all traits into EmptyValues did not work either, I am going back to the original solution of propagating just collation into EmptyValues if we are removing a Sort from the plan.

core/src/test/java/org/apache/calcite/test/RelBuilderTest.java

hsyuan · 2020-04-15T18:40:24Z

core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableValues.java

+            .replaceIf(RelDistributionTraitDef.INSTANCE,
+                () -> RelMdDistribution.values(rowType, tuples));
+    return new EnumerableValues(cluster, rowType, tuples, traitSet);
+  }


Instead of doing this, can we just modify copy(traitset, inputs) method to just return new EnumerableValues(getCluster(), rowType, tuples, traitSet);?

core/src/test/java/org/apache/calcite/test/RelBuilderTest.java

hsyuan · 2020-04-16T07:52:03Z

core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java

-            Collections.emptyList());
-      }
-      call.transformTo(emptyValues);
+      RelNode result = emptyValues.copy(single.getTraitSet(), Collections.emptyList());


If you change PruneEmptyRule's autoPruneOld to false, you might be able to see another error.

That's rigth, in that case I get:

java.lang.AssertionError at org.apache.calcite.rel.logical.LogicalValues.copy(LogicalValues.java:95)

Which refers to the following line:

@Override public RelNode copy(RelTraitSet traitSet, List<RelNode> inputs) { assert traitSet.containsIfApplicable(Convention.NONE); // <-- ...

Actually, we can solve the problem with a different approach, and it would take just one line of code, which is returning false in PruneEmptyRule's autoPruneOld (or just remove the method to not override the default SubstitutionRule's behavior):

protected abstract static class PruneEmptyRule extends RelOptRule implements SubstitutionRule { ... @Override public boolean autoPruneOld() { return false; // this solves the problem! } }

It will just hide the issue of the rule. We still get a sort in the plan.

core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableValues.java

rubenada · 2020-04-28T12:48:33Z

@danny0405 @xndai @hsyuan Do you think we could consider merging the proposed fix? It solves this specific problem by just propagating the Sort's collation into the empty values.

danny0405 · 2020-04-30T09:10:57Z

core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableValues.java


+  /** Creates an EnumerableValues. */
+  public static EnumerableValues create(Values input) {
+    final RelOptCluster cluster = input.getCluster();


How about we modify the other create to support an explicit traitSet param ?

I think the copy method should be handling that scenario

danny0405 · 2020-04-30T09:14:08Z

core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java

+            RelNode emptyValues = call.builder().push(sort).empty().build();
+            emptyValues = emptyValues.copy(
+                emptyValues.getTraitSet().replace(sort.getCollation()),
+                Collections.emptyList());


Why only copy the Collation trait ?

The (non propagation of) Collation trait is the root cause of this issue. So propagating just the Collation seems the easiest way to fix this bug (but arguably maybe not the "most correct" one).
Previously, I tried to propagate other traits as well, but it did not work as expected:
Propagating all traits does not seem a good solution. [...] there are some tests failing because of the following scenario: we have an enumerable single rel + logical empty values, so we replace the whole thing with an empty values (which in our case is a LogicalValues, as returned by RelBuilder#empty). If we try to propagate all traits it will fail, because we would be trying to propagate the EnumerableConvention towards a LogicalValues. So what should be our strategy here? Should we propagate all traits except the convention? Am I missing something in the trait propagation process?

Thanks @rubenada , i have gave a fix in [1] based on your code, hope it helps ~

[1] https://github.com/danny0405/calcite/tree/rubenada-CALCITE-3926

Thanks @danny0405 , I have added a comment in there.

danny0405 · 2020-04-30T09:46:54Z

Thanks @rubenada , i have gave a fix in [1] based on your code, hope it helps ~

[1] https://github.com/danny0405/calcite/tree/rubenada-CALCITE-3926

hsyuan · 2020-05-05T14:19:44Z

I am afraid 2970 won't finish soon. @rubenada Can you take a look at my patch and bring useful ideas you think back to your patch? https://github.com/hsyuan/calcite/commit/5365272fdd69fd4ca4d3356e7c640b17e243c6d3

I don't like the fix, which looks hacky, but it should work. The more thorough fix is logical operator should not have traits and drop RelCompositeTrait completely. They are useless, and have caused so many bugs.

rubenada · 2020-05-05T15:06:23Z

Thanks @hsyuan , I think your patch should work fine.
I have applied your approach in the current PR, please take a look when you have some time.

hsyuan · 2020-05-05T18:49:55Z

LGTM

…es a certain collation

rubenada · 2020-05-06T08:11:58Z

Thanks @hsyuan. I have rebased and I will merge as soon as the checks pass.

rubenada added the slow-tests-needed label Apr 15, 2020

hsyuan reviewed Apr 15, 2020

View reviewed changes

hsyuan reviewed Apr 16, 2020

View reviewed changes

core/src/test/java/org/apache/calcite/test/RelBuilderTest.java Show resolved Hide resolved

hsyuan reviewed Apr 16, 2020

View reviewed changes

danny0405 reviewed Apr 17, 2020

View reviewed changes

core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableValues.java Show resolved Hide resolved

danny0405 reviewed Apr 30, 2020

View reviewed changes

[CALCITE-3926] CannotPlanException when an empty LogicalValues requir…

e7b7c7e

…es a certain collation

rubenada force-pushed the CALCITE-3926 branch from cebcaa5 to e7b7c7e Compare May 6, 2020 07:28

rubenada added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label May 6, 2020

rubenada merged commit e3fe745 into apache:master May 7, 2020

Conversation

rubenada commented Apr 15, 2020

Uh oh!

rubenada commented Apr 15, 2020

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!

rubenada Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danny0405 Apr 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rubenada Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rubenada commented Apr 28, 2020

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!

danny0405 commented Apr 30, 2020

Uh oh!

hsyuan commented May 5, 2020

Uh oh!

rubenada commented May 5, 2020

Uh oh!

hsyuan commented May 5, 2020

Uh oh!

rubenada commented May 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rubenada Apr 17, 2020 •

edited

Loading

danny0405 Apr 18, 2020 •

edited

Loading

rubenada Apr 16, 2020 •

edited

Loading