AHeise commented on code in PR #28551:
URL: https://github.com/apache/flink/pull/28551#discussion_r3481470086
##########
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/dql/SqlRichExplain.java:
##########
@@ -65,7 +79,11 @@ public SqlOperator getOperator() {
@Override
public List<SqlNode> getOperandList() {
- return Collections.singletonList(statement);
+ SqlNodeList details = new SqlNodeList(SqlParserPos.ZERO);
+ for (String detail : explainDetails) {
+ details.add(SqlLiteral.createCharString(detail,
SqlParserPos.ZERO));
+ }
+ return List.of(statement, details);
Review Comment:
Is this an unrelated fix? (okay to keep)
##########
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/dql/SqlShowCall.java:
##########
@@ -59,9 +61,33 @@ public SqlShowCall(
@Override
public List<SqlNode> getOperandList() {
- return Objects.isNull(sqlIdentifier)
- ? Collections.emptyList()
- : Collections.singletonList(sqlIdentifier);
+ // Fixed layout so that deep-copy via operator.createCall can
faithfully reconstruct the
+ // subclass: [identifier, preposition, likeType, likeLiteral, notLike].
+ return ImmutableNullableList.of(
+ sqlIdentifier,
+ stringToOperand(preposition),
+ stringToOperand(likeType),
+ likeLiteral,
+ SqlLiteral.createBoolean(notLike, SqlParserPos.ZERO));
+ }
+
+ /** Encodes a nullable {@code String} field as an operand for {@link
#getOperandList()}. */
+ protected static SqlNode stringToOperand(String value) {
+ return value == null
+ ? SqlLiteral.createNull(SqlParserPos.ZERO)
+ : SqlLiteral.createCharString(value, SqlParserPos.ZERO);
+ }
+
+ /** Decodes a {@code String} field previously encoded by {@link
#stringToOperand(String)}. */
+ protected static String operandToString(SqlNode operand) {
+ return operand instanceof SqlCharStringLiteral
+ ? ((SqlCharStringLiteral) operand).getValueAs(String.class)
+ : null;
+ }
+
+ /** Decodes a {@code boolean} field encoded as a {@link SqlLiteral}. */
+ protected static boolean operandToBoolean(SqlNode operand) {
Review Comment:
Any value in doing some assertion on type for better error message? I think
it's so deep in the stack that classcast is just fine.
##########
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/dql/SqlShowTables.java:
##########
@@ -69,14 +72,53 @@ public SqlTableKind getTableKind() {
return kind;
}
+ private static SqlCall createCallForKind(
+ SqlOperator operator, SqlParserPos pos, SqlNode... operands) {
+ SqlTableKind tableKind = SqlTableKind.TABLE;
+ for (SqlTableKind candidate : SqlTableKind.values()) {
+ if (candidate.getOperator() == operator) {
+ tableKind = candidate;
+ break;
+ }
+ }
+ return new SqlShowTables(
+ pos,
+ tableKind,
+ operandToString(operands[1]),
+ (SqlIdentifier) operands[0],
+ operandToBoolean(operands[4]),
+ (SqlCharStringLiteral) operands[3]);
+ }
+
/**
* The kind of table. Keep in sync with {@link
* org.apache.flink.table.catalog.CatalogBaseTable.TableKind}.
*/
public enum SqlTableKind {
- MATERIALIZED_TABLE(new SqlSpecialOperator("SHOW MATERIALIZED TABLES",
SqlKind.OTHER)),
- TABLE(new SqlSpecialOperator("SHOW TABLES", SqlKind.OTHER)),
- VIEW(new SqlSpecialOperator("SHOW VIEWS", SqlKind.OTHER));
+ MATERIALIZED_TABLE(
+ new SqlSpecialOperator("SHOW MATERIALIZED TABLES",
SqlKind.OTHER) {
Review Comment:
Add inner class to reduce duplication?
##########
flink-table/flink-sql-parser/src/test/java/org/apache/flink/sql/parser/DeepCopyUnparsingTesterImpl.java:
##########
@@ -0,0 +1,155 @@
+/*
+ * 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.flink.sql.parser;
+
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDialect;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlWriterConfig;
+import org.apache.calcite.sql.dialect.AnsiSqlDialect;
+import org.apache.calcite.sql.parser.SqlParser;
+import org.apache.calcite.sql.parser.SqlParserTest;
+import org.apache.calcite.sql.parser.StringAndPos;
+import org.apache.calcite.sql.test.SqlTestFactory;
+import org.apache.calcite.sql.util.SqlShuttle;
+import org.apache.calcite.util.Util;
+
+import java.util.List;
+import java.util.function.Consumer;
+import java.util.function.UnaryOperator;
+import java.util.stream.Collectors;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Unparsing tester that, on top of the regular parse/unparse round-trips
performed by {@link
+ * SqlParserTest.UnparsingTesterImpl}, also makes a deep copy of every parsed
{@link SqlNode} and
+ * asserts that the copy unparses to the same SQL as the original.
+ *
+ * <p>The class should be dropped after upgrading to Calcite 1.42.0 since
similar logic is there.
+ * More details are at CALCITE-7301.
+ */
+class DeepCopyUnparsingTesterImpl extends SqlParserTest.UnparsingTesterImpl {
+
+ private static final String FLINK_PARSER_PACKAGE =
"org.apache.flink.sql.parser";
+
+ /** Whether {@code node} or any node in its subtree is a Flink parser
node. */
+ private static boolean containsFlinkNode(SqlNode node) {
+ if (isFlinkNode(node)) {
+ return true;
+ }
+ final boolean[] found = {false};
+ node.accept(
+ new SqlShuttle() {
+ @Override
+ public SqlNode visit(final SqlCall call) {
+ if (isFlinkNode(call)) {
+ found[0] = true;
+ }
+ // Default traversal copies nothing
(alwaysCopy=false), so it never
+ // invokes createCall and is safe even for unfixed
core operators.
+ return super.visit(call);
+ }
+ });
+ return found[0];
+ }
+
+ private static boolean isFlinkNode(SqlNode node) {
+ return node.getClass().getName().startsWith(FLINK_PARSER_PACKAGE);
+ }
+
+ private static SqlNode deepCopy(SqlNode sqlNode) {
+ return sqlNode.accept(
+ new SqlShuttle() {
+ @Override
+ public SqlNode visit(final SqlCall call) {
+ // Handler always creates a new copy of 'call'.
+ CallCopyingArgHandler argHandler = new
CallCopyingArgHandler(call, true);
+ call.getOperator().acceptCall(this, call, false,
argHandler);
+ return argHandler.result();
+ }
+ });
+ }
+
+ private static UnaryOperator<SqlWriterConfig> simple() {
+ return c ->
+ c.withSelectListItemsOnSeparateLines(false)
+ .withUpdateSetListNewline(false)
+ .withIndentation(0)
+ .withFromFolding(SqlWriterConfig.LineFolding.TALL);
+ }
+
+ private static SqlWriterConfig simpleWithParens(SqlWriterConfig c) {
+ return simple().apply(c).withAlwaysUseParentheses(true);
+ }
+
+ private static String toSqlString(
+ SqlNodeList sqlNodeList, UnaryOperator<SqlWriterConfig> transform)
{
+ return sqlNodeList.stream()
+ .map(node -> node.toSqlString(transform).getSql())
+ .collect(Collectors.joining(";"));
+ }
+
+ @Override
+ public void checkList(
+ SqlTestFactory factory,
+ StringAndPos sap,
+ SqlDialect dialect,
+ UnaryOperator<String> converter,
+ List<String> expected) {
+ super.checkList(factory, sap, dialect, converter, expected);
+
+ final SqlNodeList sqlNodeList = parseStmtsAndHandleEx(factory,
sap.sql);
+ if (!containsFlinkNode(sqlNodeList)) {
+ return;
+ }
+ final String sql1 = toSqlString(sqlNodeList, simple());
+
+ // Make a deep copy of the SqlNodeList, unparse it.
+ final SqlNodeList sqlNodeList3 = (SqlNodeList) deepCopy(sqlNodeList);
+ final String sql4 = toSqlString(sqlNodeList3, simple());
+ // Should be the same as we started with.
+ assertThat(sql4).isEqualTo(sql1);
+ }
Review Comment:
Find better var names? Also in below method.
##########
flink-table/flink-sql-parser/src/test/java/org/apache/flink/sql/parser/FlinkSqlUnParserTest.java:
##########
@@ -39,7 +39,9 @@ public FlinkSqlUnParserTest() {}
public SqlParserFixture fixture() {
return super.fixture()
- .withTester(new UnparsingTesterImpl())
+ // Replace back with UnparsingTesterImpl after upgrading
Calcite to 1.42.0
+ // since after that the logic is embedded into Calcite
Review Comment:
AI comment. I don't think this comment is useful going forward.
--
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]
