zabetak commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1372866281
##########
site/_docs/algebra.md:
##########
@@ -309,10 +309,6 @@ LogicalRepeatUnion(all=[true])
LogicalTableScan(table=[[aux]])
{% endhighlight %}
-Note that there is no support for recursive queries in the SQL layer yet
-([CALCITE-129](https://issues.apache.org/jira/browse/CALCITE-129));
-the `WITH RECURSIVE` query above is only for illustrative purposes.
-
Review Comment:
I think the https://calcite.apache.org/docs/reference.html part of the
documentation should be updated as well to reflect the new changes.
##########
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##########
@@ -7487,6 +7487,21 @@ private void checkSemiJoinRuleOnAntiJoin(RelOptRule
rule) {
.checkUnchanged();
}
+ @Test void testRecursiveQuery() {
+ final String sql = "WITH RECURSIVE aux(i) AS (\n"
+ + " VALUES (1)\n"
+ + " UNION ALL\n"
+ + " SELECT i+1 FROM aux WHERE i < 10\n"
+ + ")\n"
+ + "SELECT * FROM aux";
+
+ sql(sql)
+ .withLateDecorrelate(true)
+ .withTrim(true)
+ .withRule() // empty program
+ .checkUnchanged();
+ }
+
Review Comment:
We are not testing any rules here so the test seems misplaced. Consider
removing it.
##########
core/src/main/java/org/apache/calcite/plan/RelOptUtil.java:
##########
@@ -3304,6 +3305,29 @@ public static RelNode createProject(RelNode child,
Mappings.TargetMapping mappin
return createProject(projectFactory, child,
Mappings.asListNonNull(mapping.inverse()));
}
+ /** Returns the relational table node for {@code tableName} if it occurs
within a
+ * relational expression {@code root} otherwise an empty option is returned.
*/
+ public static Optional<RelOptTable> findTable(RelNode root, final String
tableName) {
Review Comment:
I don't think we use `Optional` much in the project. I don't have a strong
opinion for using it here or not but it seems that all the other methods in
this class are simply returning null so for keeping things more consistent I
would not add the wrapper.
##########
core/src/main/java/org/apache/calcite/sql/SqlWithItem.java:
##########
@@ -30,13 +30,16 @@
public class SqlWithItem extends SqlCall {
public SqlIdentifier name;
public @Nullable SqlNodeList columnList; // may be null
+ public SqlLiteral recursive;
public SqlNode query;
public SqlWithItem(SqlParserPos pos, SqlIdentifier name,
- @Nullable SqlNodeList columnList, SqlNode query) {
+ @Nullable SqlNodeList columnList, SqlNode query,
+ SqlLiteral recursive) {
Review Comment:
Do we want to keep previous constructor for backward compatibility purposes?
Please check other SqlNode classes and check if we are using a deprecation
pattern.
##########
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##########
@@ -7487,6 +7487,21 @@ private void checkSemiJoinRuleOnAntiJoin(RelOptRule
rule) {
.checkUnchanged();
}
+ @Test void testRecursiveQuery() {
+ final String sql = "WITH RECURSIVE aux(i) AS (\n"
+ + " VALUES (1)\n"
+ + " UNION ALL\n"
+ + " SELECT i+1 FROM aux WHERE i < 10\n"
+ + ")\n"
+ + "SELECT * FROM aux";
+
+ sql(sql)
+ .withLateDecorrelate(true)
+ .withTrim(true)
+ .withRule() // empty program
+ .checkUnchanged();
+ }
+
Review Comment:
This seems like a test that should be added in `SqlToRelConverterTest` class.
##########
core/src/test/java/org/apache/calcite/test/enumerable/EnumerableRepeatUnionTest.java:
##########
@@ -45,6 +45,17 @@
*/
class EnumerableRepeatUnionTest {
+ @Test void testGenerateNumbersUsingSql() {
Review Comment:
The new tests added here are basically testing recursive queries end-to-end.
For end-to-end tests it is better to use the .iq files so consider moving the
tests in `recursive_queries.iq` and remove them from here unless there is a
specific reason to have them here.
##########
core/src/main/java/org/apache/calcite/sql/validate/WithItemRecursiveNameSpace.java:
##########
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.sql.validate;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.sql.SqlBasicCall;
+import org.apache.calcite.sql.SqlIdentifier;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlWith;
+import org.apache.calcite.sql.SqlWithItem;
+import org.apache.calcite.sql.parser.SqlParserPos;
+import org.apache.calcite.util.Pair;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+/** Very similar to {@link WithItemNamespace} but created only for RECURSIVE
queries. */
+public class WithItemRecursiveNameSpace extends WithItemNamespace {
Review Comment:
Should the class be `public`?
--
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]
