libenchao commented on code in PR #2813:
URL: https://github.com/apache/calcite/pull/2813#discussion_r945399098
##########
core/src/test/java/org/apache/calcite/plan/RelWriterTest.java:
##########
@@ -998,6 +999,21 @@ void testCorrelateQuery(SqlExplainFormat format) {
.assertThatPlan(isLinux(expected));
}
+ @Test void testProjectionWithCorrelationVaribles() {
+ final Function<RelBuilder, RelNode> relFn = b -> b.scan("EMP")
+ .project(
+ ImmutableList.of(b.field("ENAME")),
+ ImmutableList.of("ename"),
+ true,
+ ImmutableSet.of(b.getCluster().createCorrel()))
+ .build();
+
+ final String expected = "LogicalProject(variablesSet=[[$cor0]],
ename=[$1])\n"
+ + " LogicalTableScan(table=[[scott, EMP]])\n";
Review Comment:
> It would be better to make this more realistic to correspond to an actual
SQL query.
I agree with this, however, we do not support `RexSubQuery` expression in
`RelJson` for now. How about we create another separate issue to track this? If
you agree with this, I'll create to issue, and start to work on it once we get
this one merged.
##########
core/src/main/java/org/apache/calcite/rel/RelNode.java:
##########
@@ -152,8 +152,27 @@ public interface RelNode extends RelOptNode, Cloneable {
* expression but also used and therefore not available to parents of this
* relational expression.
*
- * <p>Note: only {@link org.apache.calcite.rel.core.Correlate} should set
- * variables.
+ * <p> For now, only {@link org.apache.calcite.rel.core.Correlate},
+ * {@link org.apache.calcite.rel.core.Project},
+ * {@link org.apache.calcite.rel.logical.LogicalFilter} could set variables.
+ *
+ * <p> {@link org.apache.calcite.rel.core.Project} and
+ * {@link org.apache.calcite.rel.logical.LogicalFilter} are designed to carry
+ * the variables just before {@link
org.apache.calcite.rel.rules.SubQueryRemoveRule}.
+ * After that, the sub-query expressions will be expanded with
+ * {@link org.apache.calcite.rel.core.Correlate} or
+ * {@link org.apache.calcite.rel.core.Join} depending on whether it's
+ * correlated or not.
+ *
+ * <p> The reason why we put the variables in
+ * {@link org.apache.calcite.rel.logical.LogicalFilter} instead of
+ * {@link org.apache.calcite.rel.core.Filter} is that we tried to not expose
+ * the variables to the optimization/physical phase for Filter.
+ * However, as discussed in
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-5127";>[CALCITE-5127]</a>,
+ * we now put the variables in {@link org.apache.calcite.rel.core.Project}
instead
+ * of {@link org.apache.calcite.rel.logical.LogicalProject} because some
downstream
+ * projects used their own logical RelNode which extends from the core
RelNode.
Review Comment:
Yes, it is a little strange to put the implementation comment in the parent
class, I will move them to the commit message.
##########
core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java:
##########
@@ -3285,6 +3285,30 @@ void checkCorrelatedMapSubQuery(boolean expand) {
sql(sql).withDecorrelate(true).ok();
}
+ @Test void testCorrelationInProjection1() {
+ final String sql = "select array(select e.deptno) from emp e";
+ sql(sql).withExpand(false).withDecorrelate(false).ok();
+ }
+
+ @Test void testCorrelationInProjection2() {
+ final String sql = "select array(select e.deptno)\n"
+ + "from (select deptno, ename from emp) e";
+ sql(sql).withExpand(false).withDecorrelate(false).ok();
+ }
+
+ @Test void testCorrelationInProjection3() {
+ final String sql = "select cardinality(array(select e.deptno)),
array(select e.ename)[0]\n"
+ + "from (select deptno, ename from emp) e";
+ sql(sql).withExpand(false).withDecorrelate(false).ok();
+ }
+
+ @Test void testCorrelationInProjection4() {
+ final String sql = "select cardinality(arr) from"
+ + "(select array(select e.deptno) arr\n"
+ + "from (select deptno, ename from emp) e)";
+ sql(sql).withExpand(false).withDecorrelate(false).ok();
+ }
+
Review Comment:
sounds good, will change to test names.
##########
core/src/main/java/org/apache/calcite/rel/rules/ProjectWindowTransposeRule.java:
##########
@@ -97,9 +99,11 @@ public ProjectWindowTransposeRule(RelBuilderFactory
relBuilderFactory) {
builder.add(relDataTypeField);
}
+ Preconditions.checkArgument(project.getVariablesSet().isEmpty(),
+ "Correlated project should be decorrelated before.");
Review Comment:
Good point, skipping the rule is better than throwing exception, will change.
##########
core/src/test/java/org/apache/calcite/test/JdbcTest.java:
##########
@@ -2122,6 +2122,28 @@ void checkMultisetQueryWithSingleColumn() {
"EXPR$0=2");
}
+ @Test void testSelectInsideFromAndList() {
+ CalciteAssert.that()
+ .query("SELECT ARRAY(SELECT s.x)\n"
+ + "FROM (SELECT 1 as x) s")
+ .returnsValue("[{1}]");
+
+ CalciteAssert.that()
+ .query("SELECT ARRAY(SELECT s.x)\n"
+ + "FROM (SELECT ARRAY[1,2,3] as x) s")
+ .returnsCount(1);
+
+ CalciteAssert.that()
+ .query("SELECT ARRAY(SELECT * FROM UNNEST(s.x) y)\n"
+ + "FROM (SELECT ARRAY[1,2,3] as x) s")
+ .returnsCount(1);
+
+ CalciteAssert.that()
+ .query("SELECT (SELECT CARDINALITY(s.x) LIMIT 1)\n"
+ + "FROM (SELECT ARRAY[1,2,3] as x) s")
+ .returnsUnordered("EXPR$0=3");
+ }
+
Review Comment:
These tests are written by @dssysolyatin originally in
https://github.com/apache/calcite/commit/27e68ded2c3bea7d7af73dd1dc156e46fb3591a8,
I put it here just to demonstrate this PR solves the problems reported in
CALCITE-5127.
I agree we could put one of them into `sub-query.iq`
##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -1908,9 +1927,11 @@ private RelBuilder project_(
fieldNameList.add(null);
}
+ // Do not merge projection when top projection has correlation variables
bloat:
if (frame.rel instanceof Project
- && config.bloat() >= 0) {
+ && config.bloat() >= 0
+ && variables.isEmpty()) {
final Project project = (Project) frame.rel;
Review Comment:
Sure, I'll create the follow up JIRA.
##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -1817,7 +1817,24 @@ public RelBuilder project(Iterable<? extends RexNode>
nodes,
*/
public RelBuilder project(Iterable<? extends RexNode> nodes,
Iterable<? extends @Nullable String> fieldNames, boolean force) {
- return project_(nodes, fieldNames, ImmutableList.of(), force);
+ return project(nodes, fieldNames, force, ImmutableSet.of());
+ }
+
+ /**
+ * The same with {@link #project(Iterable, Iterable, boolean)}, with
additional
+ * variablesSet param.
+ *
+ * @param nodes Expressions
+ * @param fieldNames Suggested field names
+ * @param force create project even if it is identity
+ * @param variablesSet Correlating variables that are set when reading a row
+ * from the input, and which may be referenced from the
+ * projection expressions
+ */
+ public RelBuilder project(Iterable<? extends RexNode> nodes,
+ Iterable<? extends @Nullable String> fieldNames, boolean force,
+ Iterable<CorrelationId> variablesSet) {
+ return project_(nodes, fieldNames, ImmutableList.of(), force,
variablesSet);
Review Comment:
I also tried to avoid this and do it separately. However, it seems that not
possible for the following reasons:
1. we must use `ProjectFactory` to construct a `Project` instead of
`LogicalProject`, and it's hidden in `RelBuilder` now, it might be weird to
expose it for this purpose
2. one of the main change in this PR is to avoid projection merging in
`RelBuilder`, we cannot do it without passing in the `variableSet`
Do you have any good suggestions about this?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
