[GitHub] [ignite] alex-plekhanov commented on a change in pull request #9103: IGNITE-12747 Correlated queries support

[GitBox <gi...@apache.org>]

alex-plekhanov commented on a change in pull request #9103:
URL: https://github.com/apache/ignite/pull/9103#discussion_r635797674



##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ExpressionFactoryImpl.java
##########
@@ -32,6 +32,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Ordering;
 import com.google.common.primitives.Primitives;
+import com.sun.xml.internal.ws.spi.db.FieldGetter;

Review comment:
       This import is not required.

##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/FunctionsTest.java
##########
@@ -86,7 +86,9 @@ public void testRange() {
             "Increment can't be 0");
     }
 
-    /** */
+    /** Important! Don`t change query call sequence in this test. This also tests correctness of
+     * {@link org.apache.ignite.internal.processors.query.calcite.exec.exp.ExpressionFactoryImpl#SCALAR_CACHE} usage.

Review comment:
       If it tests the correctness of the SCALAR_CACHE there should be some asserts to check some invariants.

##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/FunctionsTest.java
##########
@@ -86,7 +86,9 @@ public void testRange() {
             "Increment can't be 0");
     }
 
-    /** */
+    /** Important! Don`t change query call sequence in this test. This also tests correctness of
+     * {@link org.apache.ignite.internal.processors.query.calcite.exec.exp.ExpressionFactoryImpl#SCALAR_CACHE} usage.
+     **/

Review comment:
       redundant `*`

##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/TableFunctionTest.java
##########
@@ -102,7 +102,7 @@ public void testRandomTableAndTableFunctionJoin() throws Exception {
     public void testCorrelatedTableFunctionJoin() throws Exception {
         String sql = "SELECT t.id, (SELECT x FROM TABLE(system_range(t.id, t.id))) FROM random_tbl t";
 
-        assertPlan(sql, publicSchema, nodeOrAnyChild(isInstanceOf(IgniteCorrelatedNestedLoopJoin.class)
+        assertPlan(sql, publicSchema, nodeOrAnyChild(isInstanceOf(IgniteNestedLoopJoin.class)

Review comment:
       It's a wrong plan, only `IgniteCorrelatedNestedLoopJoin` sets the correlated variables, so if the query actually will be planned like this, it will not work. This caused by correlation on the top-level node.

##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionServiceImpl.java
##########
@@ -642,13 +644,16 @@ private IgniteRel optimize(SqlNode sqlNode, IgnitePlanner planner) {
             if (rel instanceof Hintable)
                 planner.setDisabledRules(HintUtils.disabledRules((Hintable)rel));
 
+            Set<CorrelationId> correl = RelOptUtil.getVariablesUsed(rel);
+
             // Transformation chain
             rel = planner.transform(PlannerPhase.HEURISTIC_OPTIMIZATION, rel.getTraitSet(), rel);
 
             RelTraitSet desired = rel.getCluster().traitSet()
                 .replace(IgniteConvention.INSTANCE)
                 .replace(IgniteDistributions.single())
                 .replace(root.collation == null ? RelCollations.EMPTY : root.collation)
+                .replace(correl.isEmpty() ? CorrelationTrait.UNCORRELATED : CorrelationTrait.correlations(correl))

Review comment:
       Why the top-level node should be correlated on all variables? It does not actually require these variables. I think it's wrong and it leads to wrong plans (like in `TableFunctionTest`)

##########
File path: modules/calcite/src/test/sql/basic_pred_and_search_cond/quantified_comp.test
##########
@@ -0,0 +1,42 @@
+# name: test/sql/basic_pred_and_search_cond/quantified_comp.test
+# description: Test ANY/ALL queries
+# group: [any_all]
+
+statement ok
+PRAGMA enable_verification
+
+statement ok
+CREATE TABLE integers(i INTEGER)
+
+statement ok
+INSERT INTO integers VALUES (1), (2), (3), (5), (6)
+
+query T
+select i from integers where i > SOME (2, 3) ORDER BY i

Review comment:
       The test doesn't work: `SOME is only supported if expand = false`. If I set `withExpand(false)` in main framework config it works even without other changes, but other tests fail (since there is $SCALAR_QUERY appears). So all other changes don't seem to do anything useful (at least it's not covered by tests).
   
   Also, script tests can't be used for regression now and there should be java tests for the new functionality (in case of this ticket - at least planner tests for SOME and ANY and integration 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org