AHeise commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3353818729
##########
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/materializedtable/SqlAlterMaterializedTableAsQuery.java:
##########
@@ -36,16 +38,35 @@ public class SqlAlterMaterializedTableAsQuery extends
SqlAlterMaterializedTable
private final SqlNode asQuery;
+ private final @Nullable SqlParserPos asKeywordPos;
Review Comment:
Why nullable?
##########
flink-table/flink-sql-parser/src/main/codegen/includes/parserImpls.ftl:
##########
@@ -2062,24 +2063,27 @@ SqlCreate SqlCreateOrAlterMaterializedTable(Span s,
boolean replace, boolean isT
}
)
]
- <AS>
+ <AS> { asKeywordPos = getPos(); }
Review Comment:
Shouldn't this be `startPos.plus(getPos())` as well? Not too familiar with
how our parser works but it's just asymmetric.
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/materializedtable/AbstractCreateMaterializedTableConverter.java:
##########
@@ -141,10 +142,17 @@ protected final RefreshMode
getDerivedRefreshMode(LogicalRefreshMode logicalRefr
return
MaterializedTableUtils.fromLogicalRefreshModeToRefreshMode(logicalRefreshMode);
}
+ /**
+ * Returns the user's original {@code AS} query text, sliced verbatim from
the statement so
+ * formatting and identifier casing are preserved (e.g. {@code int} is not
normalized to {@code
+ * INTEGER}). A comment placed between {@code AS} and the query is kept;
the {@code AS} keyword
+ * itself is excluded.
+ */
protected final String getDerivedOriginalQuery(
T sqlCreateMaterializedTable, ConvertContext context) {
- SqlNode selectQuery = sqlCreateMaterializedTable.getAsQuery();
- return context.toQuotedSqlString(selectQuery);
+ return SqlNodeConvertUtils.rawAsQueryText(
Review Comment:
Let's use original everywhere to not mix the names raw and original all the
time.
```suggestion
return SqlNodeConvertUtils.originalAsQueryText(
```
(I could also see skipping as helps with reading, I keep reading it as "raw
as queryText", but I think all other methods also call it asQuery for some
reason - what other query could it be?)
##########
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/view/SqlAlterViewAs.java:
##########
@@ -35,9 +36,16 @@ public class SqlAlterViewAs extends SqlAlterView {
private final SqlNode newQuery;
- public SqlAlterViewAs(SqlParserPos pos, SqlIdentifier viewIdentifier,
SqlNode newQuery) {
+ private final @Nullable SqlParserPos asKeywordPos;
Review Comment:
Why nullable?
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConvertUtils.java:
##########
@@ -46,14 +48,74 @@
import javax.annotation.Nullable;
import java.util.HashMap;
+import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.stream.Collectors;
/** Utilities for SqlNode conversions. */
-class SqlNodeConvertUtils {
+public class SqlNodeConvertUtils {
+
+ /**
Review Comment:
Shorten description, ain't nobody got time to read that wall of text.
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConvertUtils.java:
##########
@@ -46,14 +48,74 @@
import javax.annotation.Nullable;
import java.util.HashMap;
+import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.stream.Collectors;
/** Utilities for SqlNode conversions. */
-class SqlNodeConvertUtils {
+public class SqlNodeConvertUtils {
+
+ /**
+ * Returns the user's original {@code AS}-query text: the verbatim
substring of the statement
+ * from immediately after the {@code AS} keyword to the end of the
statement, trimmed of
+ * surrounding whitespace.
+ *
+ * <p>The {@code AS}-query is the final clause of {@code CREATE/ALTER
MATERIALIZED TABLE} and
+ * {@code CREATE/ALTER VIEW}, so everything after {@code AS} is the query:
comments around it
+ * are kept, the {@code AS} keyword is excluded. Slicing to the statement
end (rather than the
+ * query node's parser position) keeps the whole query for node types
whose position does not
+ * span their full text, e.g. {@code WITH ... SELECT}, where {@code
SqlWith} ends at the
+ * with-list and would otherwise drop the trailing {@code SELECT}. Slicing
from the source also
+ * preserves the user's wording (case, formatting, type aliases) that
re-rendering the parsed
+ * tree would normalize.
+ *
+ * <p>Returns {@link Optional#empty()} when the statement text is
unavailable (e.g. a
+ * synthesized node with no source text); callers decide the fallback.
+ *
+ * @param asKeywordPos parser position of the {@code AS} keyword
+ * @return the verbatim AS-query text, or empty when the statement text is
unavailable
+ */
+ public static Optional<String> rawAsQueryText(
+ ConvertContext context, @Nullable SqlParserPos asKeywordPos) {
+ if (asKeywordPos == null) {
+ return Optional.empty();
+ }
+ final String statementText = context.getStatementText();
+ if (statementText != null) {
Review Comment:
Invert if.
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/SqlNodeConvertContext.java:
##########
@@ -110,6 +123,19 @@ public String toQuotedSqlString(SqlNode sqlNode) {
return sqlNode.toSqlString(getSqlDialect()).getSql();
}
+ @Override
+ @Nullable
+ public String getStatementText() {
+ return this.statement;
+ }
+
+ @Override
+ public String expandSqlIdentifiers(String originalSql) {
Review Comment:
Why did you resort here? Seems unrelated.
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlDdlToOperationConverterTest.java:
##########
@@ -2591,6 +2591,110 @@ void testCreateViewWithDynamicTableOptions() {
assertThat(operation).isInstanceOf(CreateViewOperation.class);
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("viewOriginalQueryCases")
+ void testCreateViewOriginalQuery(String name, String sql, String
expectedOriginalQuery) {
+ final Operation operation = parse(sql);
+ assertThat(operation).isInstanceOf(CreateViewOperation.class);
+ assertThat(((CreateViewOperation)
operation).getCatalogView().getOriginalQuery())
+ .isEqualTo(expectedOriginalQuery);
+ }
+
+ private static Stream<Arguments> viewOriginalQueryCases() {
+ return Stream.of(
+ viewCommentHandlingCases(),
+ viewLineBreakCases(),
+ viewComplexQueryShapeCases(),
+ viewAdversarialTextCases())
+ .flatMap(s -> s);
+ }
+
+ private static Stream<Arguments> viewCommentHandlingCases() {
Review Comment:
Also sprinkle some AS in all comments.
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConvertUtils.java:
##########
@@ -110,6 +166,13 @@ static CatalogView toCatalogView(
schema = ResolvedSchema.physical(aliasFieldNames,
schema.getColumnDataTypes());
}
+ // Capture the user's original wording (case, formatting, type
aliases) by slicing the
Review Comment:
TL;DR?
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlDdlToOperationConverterTest.java:
##########
@@ -2591,6 +2591,110 @@ void testCreateViewWithDynamicTableOptions() {
assertThat(operation).isInstanceOf(CreateViewOperation.class);
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("viewOriginalQueryCases")
+ void testCreateViewOriginalQuery(String name, String sql, String
expectedOriginalQuery) {
+ final Operation operation = parse(sql);
+ assertThat(operation).isInstanceOf(CreateViewOperation.class);
+ assertThat(((CreateViewOperation)
operation).getCatalogView().getOriginalQuery())
+ .isEqualTo(expectedOriginalQuery);
+ }
+
+ private static Stream<Arguments> viewOriginalQueryCases() {
+ return Stream.of(
+ viewCommentHandlingCases(),
+ viewLineBreakCases(),
+ viewComplexQueryShapeCases(),
+ viewAdversarialTextCases())
+ .flatMap(s -> s);
+ }
+
+ private static Stream<Arguments> viewCommentHandlingCases() {
+ return Stream.of(
+ viewAsQueryCase(
+ "comments around the query are kept",
+ "/* leading comment */\nSELECT 1\n/* trailing comment
*/"),
+ viewAsQueryCase(
+ "block comment inside the query is kept", "SELECT /*
inline comment */ 1"),
+ viewAsQueryCase("trailing line comment is kept", "SELECT 1 --
trailing comment"));
+ }
+
+ private static Stream<Arguments> viewLineBreakCases() {
Review Comment:
Add leading line comment.
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlMaterializedTableNodeToOperationConverterTest.java:
##########
@@ -320,6 +321,157 @@ void testCreateMaterializedTableWithUDTFQuery() {
+ "LATERAL
TABLE(`builtin`.`default`.`myFunc`(`b`)) AS `T` (`f1`, `f2`)");
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("originalQueryCases")
+ void testOriginalQuery(String name, String sql, String
expectedOriginalQuery) {
+ final String originalQuery =
+ createMaterializedTableOperation(sql)
+ .getCatalogMaterializedTable()
+ .getOriginalQuery();
+ assertThat(originalQuery).isEqualTo(expectedOriginalQuery);
+ }
+
+ private static Stream<Arguments> originalQueryCases() {
+ return Stream.of(
+ commentHandlingCases(),
+ lineBreakCases(),
+ ddlPrefixTokenCases(),
+ complexQueryShapeCases(),
+ adversarialTextCases())
+ .flatMap(s -> s);
+ }
+
+ private static Stream<Arguments> commentHandlingCases() {
+ return Stream.of(
+ asQueryCase(
+ "inline line comment inside query is kept",
+ "SELECT a, -- inline line comment\n b FROM t1"),
+ asQueryCase(
+ "block comment inside query is kept",
+ "SELECT /* block comment */ * FROM t1"),
+ asQueryCase(
+ "comment between AS and query is kept",
+ "-- leading comment\n SELECT * FROM t1"),
+ asQueryCase(
+ "trailing comment after query is kept",
+ "SELECT * FROM t1 -- trailing comment"));
+ }
+
+ private static Stream<Arguments> lineBreakCases() {
+ final String query = "SELECT * FROM t1";
+ return Stream.of(
+ asQueryCase(
+ "multi-line block comment between AS and query is
kept",
+ "/* multi\n line\n comment */\n" + query),
+ asQueryCase(
+ "multi-line block comment inside query is kept",
+ "SELECT a, /* spanning\n two lines */ b FROM t1"),
+ Arguments.of(
+ "line comment before AS is dropped",
+ "CREATE MATERIALIZED TABLE mtbl1\n-- note before
AS\nAS " + query,
+ query),
+ Arguments.of(
+ "multi-line block comment before AS is dropped",
+ "CREATE MATERIALIZED TABLE mtbl1\n/* a\n multiline\n
note before AS */\nAS "
+ + query,
+ query),
+ Arguments.of(
+ "multi-line DDL prefix before AS",
+ "CREATE MATERIALIZED TABLE mtbl1 (\n"
+ + " CONSTRAINT ct1 PRIMARY KEY(a) NOT
ENFORCED\n"
+ + ")\n"
+ + "COMMENT 'materialized table comment'\n"
Review Comment:
Add `AS` also to the comment and in general whereever possible.
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/materializedtable/SqlAlterMaterializedTableAsQueryConverter.java:
##########
@@ -54,13 +55,12 @@ protected Function<ResolvedCatalogMaterializedTable,
List<TableChange>> gatherTa
SqlAlterMaterializedTableAsQuery sqlAlterTableAsQuery,
ConvertContext context) {
return oldTable -> {
// Validate and extract schema from query
- String originalQuery =
context.toQuotedSqlString(sqlAlterTableAsQuery.getAsQuery());
- SqlNode validatedQuery =
-
context.getSqlValidator().validate(sqlAlterTableAsQuery.getAsQuery());
- String definitionQuery = context.toQuotedSqlString(validatedQuery);
+ SqlNode asQuery = sqlAlterTableAsQuery.getAsQuery();
+ SqlNode validatedQuery =
context.getSqlValidator().validate(asQuery);
+ String expandedQuery = context.toQuotedSqlString(validatedQuery);
PlannerQueryOperation queryOperation =
new PlannerQueryOperation(
- context.toRelRoot(validatedQuery).project(), () ->
definitionQuery);
+ context.toRelRoot(validatedQuery).project(), () ->
expandedQuery);
Review Comment:
Why do we have the changes here? They seem to unrelated. If you are fixing
something, fork out a different commit.
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConvertUtils.java:
##########
@@ -46,14 +48,74 @@
import javax.annotation.Nullable;
import java.util.HashMap;
+import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.stream.Collectors;
/** Utilities for SqlNode conversions. */
-class SqlNodeConvertUtils {
+public class SqlNodeConvertUtils {
+
+ /**
+ * Returns the user's original {@code AS}-query text: the verbatim
substring of the statement
+ * from immediately after the {@code AS} keyword to the end of the
statement, trimmed of
+ * surrounding whitespace.
+ *
+ * <p>The {@code AS}-query is the final clause of {@code CREATE/ALTER
MATERIALIZED TABLE} and
+ * {@code CREATE/ALTER VIEW}, so everything after {@code AS} is the query:
comments around it
+ * are kept, the {@code AS} keyword is excluded. Slicing to the statement
end (rather than the
+ * query node's parser position) keeps the whole query for node types
whose position does not
+ * span their full text, e.g. {@code WITH ... SELECT}, where {@code
SqlWith} ends at the
+ * with-list and would otherwise drop the trailing {@code SELECT}. Slicing
from the source also
+ * preserves the user's wording (case, formatting, type aliases) that
re-rendering the parsed
+ * tree would normalize.
+ *
+ * <p>Returns {@link Optional#empty()} when the statement text is
unavailable (e.g. a
+ * synthesized node with no source text); callers decide the fallback.
+ *
+ * @param asKeywordPos parser position of the {@code AS} keyword
+ * @return the verbatim AS-query text, or empty when the statement text is
unavailable
+ */
+ public static Optional<String> rawAsQueryText(
+ ConvertContext context, @Nullable SqlParserPos asKeywordPos) {
+ if (asKeywordPos == null) {
+ return Optional.empty();
+ }
+ final String statementText = context.getStatementText();
+ if (statementText != null) {
+ final int start = offsetAfterAs(statementText, asKeywordPos);
+ if (start >= 0) {
+ return Optional.of(statementText.substring(start).strip());
+ }
+ }
+ return Optional.empty();
+ }
+
+ /**
+ * Returns the offset of the first character after the {@code AS} keyword,
or {@code -1} if the
+ * keyword position falls outside {@code statement}.
+ */
+ private static int offsetAfterAs(String statement, SqlParserPos
asKeywordPos) {
Review Comment:
OptionalInt would be more consistent but I think we are still stuck on J11
where OptionalInt -> Optional may be awkward but please double-check.
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConvertUtils.java:
##########
@@ -46,14 +48,74 @@
import javax.annotation.Nullable;
import java.util.HashMap;
+import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.stream.Collectors;
/** Utilities for SqlNode conversions. */
-class SqlNodeConvertUtils {
+public class SqlNodeConvertUtils {
+
+ /**
+ * Returns the user's original {@code AS}-query text: the verbatim
substring of the statement
+ * from immediately after the {@code AS} keyword to the end of the
statement, trimmed of
+ * surrounding whitespace.
+ *
+ * <p>The {@code AS}-query is the final clause of {@code CREATE/ALTER
MATERIALIZED TABLE} and
+ * {@code CREATE/ALTER VIEW}, so everything after {@code AS} is the query:
comments around it
+ * are kept, the {@code AS} keyword is excluded. Slicing to the statement
end (rather than the
+ * query node's parser position) keeps the whole query for node types
whose position does not
+ * span their full text, e.g. {@code WITH ... SELECT}, where {@code
SqlWith} ends at the
+ * with-list and would otherwise drop the trailing {@code SELECT}. Slicing
from the source also
+ * preserves the user's wording (case, formatting, type aliases) that
re-rendering the parsed
+ * tree would normalize.
+ *
+ * <p>Returns {@link Optional#empty()} when the statement text is
unavailable (e.g. a
+ * synthesized node with no source text); callers decide the fallback.
+ *
+ * @param asKeywordPos parser position of the {@code AS} keyword
+ * @return the verbatim AS-query text, or empty when the statement text is
unavailable
+ */
+ public static Optional<String> rawAsQueryText(
+ ConvertContext context, @Nullable SqlParserPos asKeywordPos) {
+ if (asKeywordPos == null) {
+ return Optional.empty();
+ }
+ final String statementText = context.getStatementText();
+ if (statementText != null) {
+ final int start = offsetAfterAs(statementText, asKeywordPos);
+ if (start >= 0) {
+ return Optional.of(statementText.substring(start).strip());
+ }
+ }
+ return Optional.empty();
+ }
+
+ /**
+ * Returns the offset of the first character after the {@code AS} keyword,
or {@code -1} if the
+ * keyword position falls outside {@code statement}.
+ */
+ private static int offsetAfterAs(String statement, SqlParserPos
asKeywordPos) {
Review Comment:
The method name should be more general. Just call it `offsetAfter` and
rename argument.
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/SqlNodeToOperationConversion.java:
##########
@@ -151,46 +151,71 @@
public class SqlNodeToOperationConversion {
private final FlinkPlannerImpl flinkPlanner;
private final CatalogManager catalogManager;
+ @Nullable private final String statement;
// ~ Constructors
-----------------------------------------------------------
private SqlNodeToOperationConversion(
- FlinkPlannerImpl flinkPlanner, CatalogManager catalogManager) {
+ FlinkPlannerImpl flinkPlanner,
+ CatalogManager catalogManager,
+ @Nullable String statement) {
this.flinkPlanner = flinkPlanner;
this.catalogManager = catalogManager;
+ this.statement = statement;
}
/**
- * This is the main entrance for executing all kinds of DDL/DML {@code
SqlNode}s, different
- * SqlNode will have its implementation in the #convert(type) method whose
'type' argument is
- * subclass of {@code SqlNode}.
+ * Main entry point for converting any DDL/DML {@link SqlNode} into an
{@link Operation}.
Review Comment:
Keep diff minimal please. I don't see why you needed to rewrite the whole
comment.
--
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]
