Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
asfgit closed pull request #3480: [CALCITE-129] Support recursive WITH queries URL: https://github.com/apache/calcite/pull/3480 -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
julianhyde commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1375732912
##
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. */
+class WithItemRecursiveNameSpace extends WithItemNamespace {
+ private final SqlWithItem withItem;
+ private final SqlWithItemTableRef withItemTableRef;
+ WithItemRecursiveNameSpace(SqlValidatorImpl validator,
+ SqlWithItem withItem,
+ @Nullable SqlNode enclosingNode) {
+super(validator, withItem, enclosingNode);
+this.withItem = withItem;
+this.withItemTableRef = new SqlWithItemTableRef(SqlParserPos.ZERO,
withItem);
+ }
+
+ @Override public RelDataType getRowType() {
+if (rowType == null) {
+ SqlBasicCall call;
+ if (this.withItem.query.getKind() == SqlKind.WITH) {
+call = (SqlBasicCall) ((SqlWith) this.withItem.query).body;
+ } else {
+call = (SqlBasicCall) this.withItem.query;
+ }
+ // As this is a recursive query we only should evaluate left child for
getting the rowType.
+ RelDataType leftChildType =
+ validator.getNamespaceOrThrow(
+ call.operand(0)).getRowType();
+ SqlNodeList columnList = withItem.columnList;
+ if (columnList == null || columnList.size() == 0) {
+// This should never happen but added to protect against the
NullPointerException.
+return leftChildType;
+ }
+ final RelDataTypeFactory.Builder builder =
+ validator.getTypeFactory().builder();
+ Pair.forEach(SqlIdentifier.simpleNames(columnList),
+ leftChildType.getFieldList(),
+ (name, field) -> builder.add(name, field.getType()));
+ rowType = builder.build();
Review Comment:
I agree, @macroguo-ghy. And I think that's the only remaining issue.
@HanumathRao, I have created
https://github.com/julianhyde/calcite/tree/129-with-recursive to address that
one issue. Please take a look. If it's OK I will merge.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
sonarcloud[bot] commented on PR #3480: URL: https://github.com/apache/calcite/pull/3480#issuecomment-1784006334 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3480) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [11 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_coverage=list) [85.4% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_duplicated_lines_density=list) [3.8% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_duplicated_lines_density=list) -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
HanumathRao commented on code in PR #3480: URL: https://github.com/apache/calcite/pull/3480#discussion_r1375334002 ## core/src/test/resources/sql/recursive_queries.iq: ## @@ -0,0 +1,507 @@ +# recursive_queries.iq - recursive queries using WITH CTE +# +# 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. +# +!use scott +!set outputformat mysql + +WITH RECURSIVE FactorialCTE(n, a) AS ( +SELECT 1, 1 +UNION ALL +SELECT n + 1, a * (n + 1) +FROM FactorialCTE +WHERE n < 11 +) +SELECT a FROM FactorialCTE WHERE n = 10; ++-+ +| A | ++-+ +| 3628800 | ++-+ +(1 row) + +!ok + +WITH RECURSIVE FibonacciCTE(n, a, b) AS ( Review Comment: Thanks for asking this question. Yes, mutually recursive queries are valid as per SQL standard. However, this implementation doesn't support mutually recursive queries. In the latest commit, I have added a test case which reports an error (in SqlValidatorTest). I think it is better to track support for mutually recursive queries as a new functionality rather than clubbing it with the general recursive query support. WDYT? Please find the sqlfiddle [link](http://sqlfiddle.com/#!17/9eecb/111736) for postgres which reports an error for mutually recursive queries. -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
sonarcloud[bot] commented on PR #3480: URL: https://github.com/apache/calcite/pull/3480#issuecomment-1783940770 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3480) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [11 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_coverage=list) [85.4% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_duplicated_lines_density=list) [3.8% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_duplicated_lines_density=list) -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
HanumathRao commented on PR #3480: URL: https://github.com/apache/calcite/pull/3480#issuecomment-1783940719 Thanks @julianhyde and @macroguo-ghy for the review. I have addressed all the review comments. Please take a look when you get a chance. -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
julianhyde commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1375148442
##
core/src/main/java/org/apache/calcite/plan/RelOptUtil.java:
##
@@ -3304,6 +3304,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 @Nullable RelOptTable findTable(RelNode root, final String
tableName) {
+RelOptTable[] table = new RelOptTable[1];
+try {
+ RelShuttle visitor = new RelHomogeneousShuttle() {
+@Override public RelNode visit(TableScan scan) {
+ final RelOptTable scanTable = scan.getTable();
+ final List qualifiedName = scanTable.getQualifiedName();
+ if (qualifiedName.get(qualifiedName.size() - 1).equals(tableName)) {
+table[0] = scanTable;
+throw Util.FoundOne.NULL;
Review Comment:
a bit simpler is `throw new Util.FoundOne(scanTable)`; then you don't need
the `table[]` variable.
##
core/src/main/java/org/apache/calcite/sql/validate/WithItemRecursiveNameSpace.java:
##
@@ -0,0 +1,78 @@
+/*
+ * 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;
+
+import java.util.Objects;
+
+/** Very similar to {@link WithItemNamespace} but created only for RECURSIVE
queries. */
+class WithItemRecursiveNameSpace extends WithItemNamespace {
+ private final SqlWithItem withItem;
+ private final SqlWithItemTableRef withItemTableRef;
+ WithItemRecursiveNameSpace(SqlValidatorImpl validator,
Review Comment:
and blank line and javadoc comment before constructor
##
core/src/main/java/org/apache/calcite/sql/SqlKind.java:
##
@@ -195,6 +195,9 @@ public enum SqlKind {
/** Item in WITH clause. */
WITH_ITEM,
+ /** Represents a recursive CTE as a table ref. **/
+ WITH_ITEM_TABLE_REF,
Review Comment:
change `**/` to `*/`
##
core/src/main/java/org/apache/calcite/sql/validate/WithRecursiveScope.java:
##
@@ -0,0 +1,76 @@
+/*
+ * 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.StructKind;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlWithItem;
+
+import com.google.common.collect.ImmutableList;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.List;
+
+/** Scope providing the objects that are being defined using the with clause to
+ * the with clause query definitions.
+ *
+ * For example, in
+ *
+ * {@code WITH t1 AS (q1_can_use_t1) t2 AS (q2_can_use_t1_and_t2)
q3}
+ *
+ * {@code t1} provides a scope that is used
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
julianhyde commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1375145871
##
core/src/main/java/org/apache/calcite/sql/SqlWithItem.java:
##
@@ -30,13 +30,26 @@
public class SqlWithItem extends SqlCall {
public SqlIdentifier name;
public @Nullable SqlNodeList columnList; // may be null
+ public SqlLiteral recursive;
Review Comment:
SqlLiteral is better. It contains a position, so you can use it in error
messages if necessary.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
julianhyde commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1375147643
##
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##
@@ -2759,6 +2769,13 @@ protected void convertUnpivot(Blackboard bb, SqlUnpivot
unpivot) {
bb.setRoot(relBuilder.build(), true);
}
+ private void convertTransientScan(Blackboard bb, SqlWithItem withItem) {
+final SqlValidatorNamespace fromNamespace =
getNamespace(withItem).resolve();
+bb.setRoot(
+relBuilder.transientScan(withItem.name.getSimple(),
+fromNamespace.getRowType()).build(), true);
+ }
Review Comment:
need more indentation before `fromNamespace`
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
julianhyde commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1375146709
##
core/src/main/java/org/apache/calcite/sql/validate/SqlWithItemTableRef.java:
##
@@ -0,0 +1,62 @@
+/*
+ * 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.sql.SqlCall;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.SqlSpecialOperator;
+import org.apache.calcite.sql.SqlTableRef;
+import org.apache.calcite.sql.SqlWithItem;
+import org.apache.calcite.sql.parser.SqlParserPos;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * A SqlWithItemTableRef is a node created during validation for
+ * recursive queries which represents a recursive with table reference.
+ */
Review Comment:
I'd change "a recursive with table reference" to "a table reference in a
{@code WITH RECURSIVE} clause". The sentence is easier for humans to parse.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
julianhyde commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1375146021
##
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. */
+class WithItemRecursiveNameSpace extends WithItemNamespace {
Review Comment:
Agreed. (@HanumathRao, note lower-case 's'.)
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
julianhyde commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1375145871
##
core/src/main/java/org/apache/calcite/sql/SqlWithItem.java:
##
@@ -30,13 +30,26 @@
public class SqlWithItem extends SqlCall {
public SqlIdentifier name;
public @Nullable SqlNodeList columnList; // may be null
+ public SqlLiteral recursive;
Review Comment:
SqlLiteral contains a position, so you can use it in error messages if
necessary.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
sonarcloud[bot] commented on PR #3480: URL: https://github.com/apache/calcite/pull/3480#issuecomment-1783629738 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3480) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [12 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_coverage=list) [85.8% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_duplicated_lines_density=list) [3.8% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_duplicated_lines_density=list) -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
mihaibudiu commented on PR #3480: URL: https://github.com/apache/calcite/pull/3480#issuecomment-1783319871 I will need more time to review this, but if you want to get it merged for 1.36 you can go ahead. I will file issues later if I find problems. -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
HanumathRao commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1374840761
##
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##
@@ -3099,9 +3100,23 @@ private void registerSetop(
// A setop is in the same scope as its parent.
scopes.put(call, parentScope);
-for (SqlNode operand : call.getOperandList()) {
+@NonNull SqlValidatorScope recursiveScope = parentScope;
+if (enclosingNode.getKind() == SqlKind.WITH_ITEM) {
+ if (node.getKind() != SqlKind.UNION) {
Review Comment:
The recursive flag is not explicitly checked here because for only recursive
queries we pass WITH_ITEM as enclosed node(essentially it is kind of an
implicit check). For non recursive queries we pass the original enclosed node
which is either WITH or its parent.
`And I think we should implement this validation logic in
org.apache.calcite.sql.validate.WithItemNamespace#validateImpl or
org.apache.calcite.sql.validate.SetopNamespace#validateImpl instead of
registerSetop. WDYT?`
As regards to the validation logic implementation I did go that path but I
think code is getting messy. I thought if we can create a WithRecursive*
classes for these we could do custom logic for recursive queries alone.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
rubenada commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1374746714
##
core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties:
##
@@ -63,6 +63,8 @@ IncompatibleValueType=Values passed to {0} operator must have
compatible types
IncompatibleTypesInList=Values in expression list must have compatible types
IncompatibleCharset=Cannot apply {0} to the two different charsets {1} and {2}
InvalidOrderByPos=ORDER BY is only allowed on top-level SELECT
+RecursiveWithMustHaveUnionSetOp=A recursive query only supports UNION [ALL]
operator
Review Comment:
ok, np
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
macroguo-ghy commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1374651769
##
core/src/main/java/org/apache/calcite/sql/SqlWith.java:
##
@@ -96,6 +96,10 @@ private SqlWithOperator() {
final SqlWith with = (SqlWith) call;
final SqlWriter.Frame frame =
writer.startList(SqlWriter.FrameTypeEnum.WITH, "WITH", "");
+ boolean isRecursive = ((SqlWithItem)
with.withList.get(0)).recursive.booleanValue();
+ if (isRecursive) {
+writer.keyword(" RECURSIVE ");
Review Comment:
We don't need to add space, just ` writer.keyword("RECURSIVE");`
##
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##
@@ -3099,9 +3100,23 @@ private void registerSetop(
// A setop is in the same scope as its parent.
scopes.put(call, parentScope);
-for (SqlNode operand : call.getOperandList()) {
+@NonNull SqlValidatorScope recursiveScope = parentScope;
+if (enclosingNode.getKind() == SqlKind.WITH_ITEM) {
+ if (node.getKind() != SqlKind.UNION) {
+throw newValidationError(node,
RESOURCE.recursiveWithMustHaveUnionSetOp());
+ } else if (call.getOperandList().size() > 2) {
+throw newValidationError(node,
RESOURCE.recursiveWithMustHaveTwoChildUnionSetOp());
+ }
+ WithScope scope = (WithScope) scopes.get(enclosingNode);
Review Comment:
Make variable `final` as much as possiable.
##
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. */
+class WithItemRecursiveNameSpace extends WithItemNamespace {
+ private final SqlWithItem withItem;
+ private final SqlWithItemTableRef withItemTableRef;
+ WithItemRecursiveNameSpace(SqlValidatorImpl validator,
+ SqlWithItem withItem,
+ @Nullable SqlNode enclosingNode) {
+super(validator, withItem, enclosingNode);
+this.withItem = withItem;
+this.withItemTableRef = new SqlWithItemTableRef(SqlParserPos.ZERO,
withItem);
+ }
+
+ @Override public RelDataType getRowType() {
+if (rowType == null) {
+ SqlBasicCall call;
+ if (this.withItem.query.getKind() == SqlKind.WITH) {
+call = (SqlBasicCall) ((SqlWith) this.withItem.query).body;
+ } else {
+call = (SqlBasicCall) this.withItem.query;
+ }
+ // As this is a recursive query we only should evaluate left child for
getting the rowType.
+ RelDataType leftChildType =
+ validator.getNamespaceOrThrow(
+ call.operand(0)).getRowType();
+ SqlNodeList columnList = withItem.columnList;
+ if (columnList == null || columnList.size() == 0) {
+// This should never happen but added to protect against the
NullPointerException.
+return leftChildType;
+ }
+ final RelDataTypeFactory.Builder builder =
+ validator.getTypeFactory().builder();
+ Pair.forEach(SqlIdentifier.simpleNames(columnList),
+ leftChildType.getFieldList(),
+ (name, field) -> builder.add(name, field.getType()));
+ rowType = builder.build();
Review Comment:
We don't need to set the `rowtype`. Method
`org.apache.calcite.sql.validate.AbstractNamespace#validate` will help us do
this.
##
core/src/main/java/org/apache/calcite/sql/SqlWithItem.java:
##
@@ -30,13 +30,26 @@
public class SqlWithItem extends SqlCall {
public SqlIdentifier name;
public @Nullable SqlNodeList
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
sonarcloud[bot] commented on PR #3480: URL: https://github.com/apache/calcite/pull/3480#issuecomment-1782793672 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3480) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [12 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_coverage=list) [84.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_duplicated_lines_density=list) [4.4% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_duplicated_lines_density=list) -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
HanumathRao commented on PR #3480: URL: https://github.com/apache/calcite/pull/3480#issuecomment-1782758993 Thanks @zabetak and @rubenada for the review, I addressed the review comments. -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
HanumathRao commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1374440499
##
core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties:
##
@@ -63,6 +63,8 @@ IncompatibleValueType=Values passed to {0} operator must have
compatible types
IncompatibleTypesInList=Values in expression list must have compatible types
IncompatibleCharset=Cannot apply {0} to the two different charsets {1} and {2}
InvalidOrderByPos=ORDER BY is only allowed on top-level SELECT
+RecursiveWithMustHaveUnionSetOp=A recursive query only supports UNION [ALL]
operator
Review Comment:
These errors are protected by "Object not found", currently it is very hard
to hit these errors but I thought it is good to protect code path in regards to
any future changes with scope etc.
Please let me know if you think these are redundant, then I can remove them.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
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 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
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
rubenada commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1372704477
##
core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties:
##
@@ -63,6 +63,8 @@ IncompatibleValueType=Values passed to {0} operator must have
compatible types
IncompatibleTypesInList=Values in expression list must have compatible types
IncompatibleCharset=Cannot apply {0} to the two different charsets {1} and {2}
InvalidOrderByPos=ORDER BY is only allowed on top-level SELECT
+RecursiveWithMustHaveUnionSetOp=A recursive query only supports UNION [ALL]
operator
Review Comment:
Would it be possible to have unit tests forcing to have these two error
messages?
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
rubenada commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1372703471
##
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##
@@ -5232,6 +5232,39 @@ void testReturnsCorrectRowTypeOnCombinedJoin() {
+ "select * from emp2")
.type(EMP_RECORD_TYPE);
+// simplest with recursive fails
+sql("with RECURSIVE emp2 as (select * from ^emp2^)\n"
++ "select * from emp2")
+.fails("Object 'EMP2' not found");
+
+sql("with RECURSIVE emp2 as ("
++ "select * from emp "
++ " union select * from ^emp2^"
++ " union select * from emp2"
++ ")\n"
++ "select * from emp2")
+.fails("Object 'EMP2' not found");
+
+// simplest with RECURSIVE working case.
+sql("with RECURSIVE emp2 as (select * from emp union select * from emp2)\n"
++ "select * from emp2")
+.type(EMP_RECORD_TYPE);
+
+// union all with recursive working case.
+sql("with RECURSIVE emp2 as (select * from emp union all select * from
emp2)\n"
++ "select * from emp2")
+.type(EMP_RECORD_TYPE);
+
+// union all with recursive working case.
Review Comment:
minor: this comment seems wrong ("working case" but the test actually checks
for a failure)
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
HanumathRao commented on PR #3480: URL: https://github.com/apache/calcite/pull/3480#issuecomment-1780163695 > Nice work @HanumathRao ! I have left some (minor) comments. I'm not the biggest expert on the parser and validator, so I'd prefer if someone else would take a look at that part. Thanks for adding such a thorough amount of tests. > > I think there is one part of the documentation that needs to be updated in the PR: in `algebra.md`, in the "Recursive Queries" section, this paragraph should be removed :) > > ``` > 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. > ``` Thanks @rubenada for the review. I have addressed the review comments, please take a look when you get a chance. -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
sonarcloud[bot] commented on PR #3480: URL: https://github.com/apache/calcite/pull/3480#issuecomment-1780157195 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3480) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [10 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_coverage=list) [85.2% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_duplicated_lines_density=list) [4.5% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_duplicated_lines_density=list) -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
rubenada commented on PR #3480: URL: https://github.com/apache/calcite/pull/3480#issuecomment-1778712954 Nice work @HanumathRao ! I have left some (minor) comments. I'm not the biggest expert on the parser and validator, so I'd prefer if someone else would take a look at that part. Thanks for adding such a thorough amount of tests. I think there is one part of the documentation that needs to be updated in the PR: in `algebra.md`, in the "Recursive Queries", this paragraph should be removed :) ``` 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. ``` -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
rubenada commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1371284083
##
core/src/test/java/org/apache/calcite/test/enumerable/EnumerableRepeatUnionTest.java:
##
@@ -70,6 +81,47 @@ class EnumerableRepeatUnionTest {
.returnsOrdered("i=1", "i=2", "i=3", "i=4", "i=5", "i=6", "i=7",
"i=8", "i=9", "i=10");
}
+ @Test void testGenerateNumbers2UsingSql() {
+CalciteAssert.that()
+.query("WITH RECURSIVE aux(i) AS (\n"
++ " VALUES (0)"
++ " UNION "
++ " SELECT MOD((i+1), 10) FROM aux WHERE i < 10"
++ " )"
++ " SELECT * FROM aux\n")
+.returnsOrdered("I=0", "I=1", "I=2", "I=3", "I=4", "I=5", "I=6",
"I=7", "I=8", "I=9");
+ }
+
+ @Test void testGenerateNumbers2UsingSqlCheckPlan() {
+CalciteAssert.that()
+.with(CalciteConnectionProperty.LEX, Lex.JAVA)
+.with(CalciteConnectionProperty.FORCE_DECORRELATE, false)
+.withSchema("s", new ReflectiveSchema(new HierarchySchema()))
+.withHook(Hook.PLANNER, (Consumer) planner -> {
Review Comment:
Do we really need the Hook.PLANNER for this test? It adds/removes
join-related rules, but the query does not have any join
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
rubenada commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1371279349
##
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##
@@ -3774,6 +3791,48 @@ protected RelRoot convertQueryRecursive(SqlNode query,
boolean top,
}
}
+ public static boolean hasATableScanWithName(RelNode root, final String
recurTableName) {
Review Comment:
There is a RelOptTableFinder class inside RelBuider which seems to have the
same purpose. Currently it is private static, but perhaps it could be moved to
another localion (RelOptUtil?) as a public element, and reuse the same code
here and in RelBuilder.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
rubenada commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1371257262
##
core/src/test/java/org/apache/calcite/test/enumerable/EnumerableRepeatUnionTest.java:
##
@@ -45,6 +45,17 @@
*/
class EnumerableRepeatUnionTest {
+ @Test void testGenerateNumbersUsingSql() {
+CalciteAssert.that()
+.query("WITH RECURSIVE delta(n) AS (\n"
++ " VALUES (1)\n"
++ " UNION ALL\n"
Review Comment:
More info about how RECURSIVE UNION [ALL] works internally can be found here:
https://www.postgresql.org/docs/current/queries-with.html#QUERIES-WITH-RECURSIVE
Calcite implementation was heavily inspired by this postgresql description.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
HanumathRao commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1370589771
##
core/src/test/java/org/apache/calcite/test/enumerable/EnumerableRepeatUnionTest.java:
##
@@ -45,6 +45,17 @@
*/
class EnumerableRepeatUnionTest {
+ @Test void testGenerateNumbersUsingSql() {
+CalciteAssert.that()
+.query("WITH RECURSIVE delta(n) AS (\n"
++ " VALUES (1)\n"
++ " UNION ALL\n"
Review Comment:
@mihaibudiu Thanks for reviewing the changes. I agree with @rubenada
regarding the explanation as to why this use case doesn't run into infinite
growth. I believe in general that semantics of UNION / UNION ALL are subtle in
Recursive queries.
Here is an example (again from postgresql) where usage of the union all
(http://sqlfiddle.com/#!17/9eecb/111278) will run into infinite loop but usage
of union works fine.
The infinite loop for the above example actually is because of the result
set being a multi set when union all is used.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
mihaibudiu commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1370476759
##
core/src/test/java/org/apache/calcite/test/enumerable/EnumerableRepeatUnionTest.java:
##
@@ -45,6 +45,17 @@
*/
class EnumerableRepeatUnionTest {
+ @Test void testGenerateNumbersUsingSql() {
+CalciteAssert.that()
+.query("WITH RECURSIVE delta(n) AS (\n"
++ " VALUES (1)\n"
++ " UNION ALL\n"
Review Comment:
The `WHERE` breaks the growth in the value of n, but UNION would break the
growth in cardinality.
UNION ALL should produce a multiset, so you should get the following values
in `delta` in successive iterations: [1], [1, 2], [1, 2, 2, 3], [1, 2, 2, 2, 3,
3, 4] etc in your relation in successive iterations. I understand how this
should work with `UNION`, but not with `UNION ALL`. Why is delta a set and not
a multiset?
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
rubenada commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1370435167
##
core/src/test/java/org/apache/calcite/test/enumerable/EnumerableRepeatUnionTest.java:
##
@@ -45,6 +45,17 @@
*/
class EnumerableRepeatUnionTest {
+ @Test void testGenerateNumbersUsingSql() {
+CalciteAssert.that()
+.query("WITH RECURSIVE delta(n) AS (\n"
++ " VALUES (1)\n"
++ " UNION ALL\n"
Review Comment:
@mihaibudiu the `WHERE n < 10` breaks the "infinite growth". We get the same
behavior [in e.g. PostgreSQL](http://sqlfiddle.com/#!17/9eecb/111270)
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
mihaibudiu commented on code in PR #3480:
URL: https://github.com/apache/calcite/pull/3480#discussion_r1369643978
##
core/src/test/java/org/apache/calcite/test/enumerable/EnumerableRepeatUnionTest.java:
##
@@ -45,6 +45,17 @@
*/
class EnumerableRepeatUnionTest {
+ @Test void testGenerateNumbersUsingSql() {
+CalciteAssert.that()
+.query("WITH RECURSIVE delta(n) AS (\n"
++ " VALUES (1)\n"
++ " UNION ALL\n"
Review Comment:
shouldn't UNION ALL actually never converge?
Why isn't the result collection a multiset that grows infinitely?
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-129] Support recursive WITH queries [calcite]
sonarcloud[bot] commented on PR #3480: URL: https://github.com/apache/calcite/pull/3480#issuecomment-1774256074 Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite=3480) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=VULNERABILITY) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3480=false=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [10 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite=3480=false=CODE_SMELL) [](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_coverage=list) [85.5% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_coverage=list) [](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_duplicated_lines_density=list) [4.7% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite=3480=new_duplicated_lines_density=list) -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
