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 columnList; // may be null
+ public SqlLiteral recursive;
Review Comment:
Do you have to use the `SqlLiteral` type? I think using `boolean` is simpler.
##########
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.
Review Comment:
I am not sure if you check `UNION` operands type. Can you give me a test like
```sql
WITH RECURSIVE cte (n) AS
(
SELECT 1, 2
UNION ALL
SELECT n + 1 FROM cte WHERE n < 5
)
SELECT * FROM cte;
```
This case is an illegal SQL, it should fail.
##########
core/src/main/codegen/templates/Parser.jj:
##########
@@ -3486,14 +3486,16 @@ SqlNodeList WithList() :
{
final Span s;
final List<SqlWithItem> list = new ArrayList<SqlWithItem>();
+ boolean recursive = false;
}
{
- <WITH> { s = span(); }
- AddWithItem(list) ( <COMMA> AddWithItem(list) )*
+ <WITH> [ <RECURSIVE> { recursive = true; } ]{ s = span(); }
Review Comment:
I think you'd better add some tests for `WITH RECURSIVE` in `SqlParserTest`.
##########
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:
Use `Namespace`.
##########
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:
Did you not check if the with item is recursive, was it intentional?
Can you give me a test like
```
with t as (select 1 union select 2)
select * from t;
```
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?
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -3126,17 +3141,21 @@ private void registerWith(
SqlValidatorScope scope = parentScope;
for (SqlNode withItem_ : with.withList) {
final SqlWithItem withItem = (SqlWithItem) withItem_;
- final WithScope withScope = new WithScope(scope, withItem);
+
+ final boolean isRecursiveWith = withItem.recursive.booleanValue();
+ final SqlValidatorScope withScope =
Review Comment:
What are the benefits of using a nested scope compared to using
`WithRecursiveScope` directly
--
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]
