rubenada commented on a change in pull request #2194:
URL: https://github.com/apache/calcite/pull/2194#discussion_r501505321
##########
File path: core/src/main/java/org/apache/calcite/rel/metadata/RelMdUtil.java
##########
@@ -872,23 +872,36 @@ public static boolean
checkInputForCollationAndLimit(RelMetadataQuery mq,
RelNode input, RelCollation collation, RexNode offset, RexNode fetch) {
// Check if the input is already sorted
boolean alreadySorted = collation.getFieldCollations().isEmpty();
- for (RelCollation inputCollation : mq.collations(input)) {
- if (inputCollation.satisfies(collation)) {
- alreadySorted = true;
- break;
+ if (!alreadySorted) {
+ final ImmutableList<RelCollation> collations = mq.collations(input);
+ if (collations != null) {
Review comment:
Yes, totally. I'll change it
##########
File path: core/src/test/java/org/apache/calcite/test/RelMetadataTest.java
##########
@@ -3132,6 +3133,35 @@ private void checkNodeTypeCount(String sql, Map<Class<?
extends RelNode>, Intege
is("=($0, $1)"));
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-4315";>[CALCITE-4315]
+ * NPE in RelMdUtil#checkInputForCollationAndLimit</a>. */
+ @Test void testCheckInputForCollationAndLimit() {
+ final Project rel = (Project) convertSql("select * from emp, dept");
+ final Join join = (Join) rel.getInput();
+ final RelOptTable empTable = join.getInput(0).getTable();
+ final RelOptTable deptTable = join.getInput(1).getTable();
+ Frameworks.withPlanner((cluster, relOptSchema, rootSchema) -> {
+ checkInputForCollationAndLimit(cluster, empTable, deptTable);
+ return null;
+ });
+ }
+
+ private void checkInputForCollationAndLimit(RelOptCluster cluster,
RelOptTable empTable,
+ RelOptTable deptTable) {
+ final RexBuilder rexBuilder = cluster.getRexBuilder();
+ final RelMetadataQuery mq = cluster.getMetadataQuery();
+ final List<RelHint> hints = ImmutableList.of();
+ final LogicalTableScan empScan = LogicalTableScan.create(cluster,
empTable, hints);
+ final LogicalTableScan deptScan = LogicalTableScan.create(cluster,
deptTable, hints);
+ final LogicalJoin join =
+ LogicalJoin.create(empScan, deptScan, ImmutableList.of(),
+ rexBuilder.makeLiteral(true), ImmutableSet.of(),
JoinRelType.INNER);
+ assertTrue(
+ RelMdUtil.checkInputForCollationAndLimit(mq, join,
join.getTraitSet().getCollation(),
+ null, null));
Review comment:
`true` is expected because we are checking the `join` against its own
collation (so of course it is already sorted by that collation) and no limit
(fetch parameter is `null`). In any case, checking the result of the method is
not really relevant, the original problem was a NPE inside
`checkInputForCollationAndLimit`, so for this test we could perfectly remove
the `assertTrue(...)`, and just the call `checkInputForCollationAndLimit`.
##########
File path: core/src/test/java/org/apache/calcite/test/RelMetadataTest.java
##########
@@ -3132,6 +3133,35 @@ private void checkNodeTypeCount(String sql, Map<Class<?
extends RelNode>, Intege
is("=($0, $1)"));
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-4315";>[CALCITE-4315]
+ * NPE in RelMdUtil#checkInputForCollationAndLimit</a>. */
+ @Test void testCheckInputForCollationAndLimit() {
+ final Project rel = (Project) convertSql("select * from emp, dept");
+ final Join join = (Join) rel.getInput();
+ final RelOptTable empTable = join.getInput(0).getTable();
+ final RelOptTable deptTable = join.getInput(1).getTable();
+ Frameworks.withPlanner((cluster, relOptSchema, rootSchema) -> {
+ checkInputForCollationAndLimit(cluster, empTable, deptTable);
+ return null;
+ });
+ }
+
+ private void checkInputForCollationAndLimit(RelOptCluster cluster,
RelOptTable empTable,
+ RelOptTable deptTable) {
+ final RexBuilder rexBuilder = cluster.getRexBuilder();
+ final RelMetadataQuery mq = cluster.getMetadataQuery();
+ final List<RelHint> hints = ImmutableList.of();
+ final LogicalTableScan empScan = LogicalTableScan.create(cluster,
empTable, hints);
+ final LogicalTableScan deptScan = LogicalTableScan.create(cluster,
deptTable, hints);
+ final LogicalJoin join =
+ LogicalJoin.create(empScan, deptScan, ImmutableList.of(),
+ rexBuilder.makeLiteral(true), ImmutableSet.of(),
JoinRelType.INNER);
+ assertTrue(
+ RelMdUtil.checkInputForCollationAndLimit(mq, join,
join.getTraitSet().getCollation(),
+ null, null));
Review comment:
`true` is expected because we are checking the `join` against its own
collation (so of course it is already sorted by that collation) and no limit
(fetch parameter is `null`). In any case, checking the result of the method is
not really relevant, the original problem was a NPE inside
`checkInputForCollationAndLimit`, so for this test I can perfectly remove the
`assertTrue(...)`, and just the call `checkInputForCollationAndLimit`.
##########
File path: core/src/main/java/org/apache/calcite/rel/metadata/RelMdUtil.java
##########
@@ -872,23 +872,36 @@ public static boolean
checkInputForCollationAndLimit(RelMetadataQuery mq,
RelNode input, RelCollation collation, RexNode offset, RexNode fetch) {
// Check if the input is already sorted
boolean alreadySorted = collation.getFieldCollations().isEmpty();
- for (RelCollation inputCollation : mq.collations(input)) {
- if (inputCollation.satisfies(collation)) {
- alreadySorted = true;
- break;
+ if (!alreadySorted) {
+ final ImmutableList<RelCollation> collations = mq.collations(input);
+ if (collations != null) {
Review comment:
Changed
##########
File path: core/src/test/java/org/apache/calcite/test/RelMetadataTest.java
##########
@@ -3132,6 +3133,35 @@ private void checkNodeTypeCount(String sql, Map<Class<?
extends RelNode>, Intege
is("=($0, $1)"));
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-4315";>[CALCITE-4315]
+ * NPE in RelMdUtil#checkInputForCollationAndLimit</a>. */
+ @Test void testCheckInputForCollationAndLimit() {
+ final Project rel = (Project) convertSql("select * from emp, dept");
+ final Join join = (Join) rel.getInput();
+ final RelOptTable empTable = join.getInput(0).getTable();
+ final RelOptTable deptTable = join.getInput(1).getTable();
+ Frameworks.withPlanner((cluster, relOptSchema, rootSchema) -> {
+ checkInputForCollationAndLimit(cluster, empTable, deptTable);
+ return null;
+ });
+ }
+
+ private void checkInputForCollationAndLimit(RelOptCluster cluster,
RelOptTable empTable,
+ RelOptTable deptTable) {
+ final RexBuilder rexBuilder = cluster.getRexBuilder();
+ final RelMetadataQuery mq = cluster.getMetadataQuery();
+ final List<RelHint> hints = ImmutableList.of();
+ final LogicalTableScan empScan = LogicalTableScan.create(cluster,
empTable, hints);
+ final LogicalTableScan deptScan = LogicalTableScan.create(cluster,
deptTable, hints);
+ final LogicalJoin join =
+ LogicalJoin.create(empScan, deptScan, ImmutableList.of(),
+ rexBuilder.makeLiteral(true), ImmutableSet.of(),
JoinRelType.INNER);
+ assertTrue(
+ RelMdUtil.checkInputForCollationAndLimit(mq, join,
join.getTraitSet().getCollation(),
+ null, null));
Review comment:
The test without assert runs fine (I have just pushed a version in that
way). It just executes a method that previously used to failed with a NPE, and
now with the fix it runs successfully.
But I am ok with both (with or without assert). If you prefer, I can
re-apply the assert with an explanation in the assert message.
##########
File path: core/src/test/java/org/apache/calcite/test/RelMetadataTest.java
##########
@@ -3132,6 +3133,35 @@ private void checkNodeTypeCount(String sql, Map<Class<?
extends RelNode>, Intege
is("=($0, $1)"));
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-4315";>[CALCITE-4315]
+ * NPE in RelMdUtil#checkInputForCollationAndLimit</a>. */
+ @Test void testCheckInputForCollationAndLimit() {
+ final Project rel = (Project) convertSql("select * from emp, dept");
+ final Join join = (Join) rel.getInput();
+ final RelOptTable empTable = join.getInput(0).getTable();
+ final RelOptTable deptTable = join.getInput(1).getTable();
+ Frameworks.withPlanner((cluster, relOptSchema, rootSchema) -> {
+ checkInputForCollationAndLimit(cluster, empTable, deptTable);
+ return null;
+ });
+ }
+
+ private void checkInputForCollationAndLimit(RelOptCluster cluster,
RelOptTable empTable,
+ RelOptTable deptTable) {
+ final RexBuilder rexBuilder = cluster.getRexBuilder();
+ final RelMetadataQuery mq = cluster.getMetadataQuery();
+ final List<RelHint> hints = ImmutableList.of();
+ final LogicalTableScan empScan = LogicalTableScan.create(cluster,
empTable, hints);
+ final LogicalTableScan deptScan = LogicalTableScan.create(cluster,
deptTable, hints);
+ final LogicalJoin join =
+ LogicalJoin.create(empScan, deptScan, ImmutableList.of(),
+ rexBuilder.makeLiteral(true), ImmutableSet.of(),
JoinRelType.INNER);
+ assertTrue(
+ RelMdUtil.checkInputForCollationAndLimit(mq, join,
join.getTraitSet().getCollation(),
+ null, null));
Review comment:
re-added assert, with message
----------------------------------------------------------------
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:
[email protected]
