twalthr commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3356279646
##########
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:
please put this into code, I asked myself the same question
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConvertUtils.java:
##########
@@ -46,14 +48,58 @@
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.OptionalInt;
import java.util.stream.Collectors;
/** Utilities for SqlNode conversions. */
-class SqlNodeConvertUtils {
+public class SqlNodeConvertUtils {
+
+ /**
+ * Returns the {@code AS}-query verbatim: the statement text from just
after {@code AS} to its
Review Comment:
Add more context to external readers.
```suggestion
* Returns the {@code AS}-query of a VIEW or MATERIALIZED TABLE DDL in
verbatim shape: the statement text from just after {@code AS} to its
```
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/SqlNodeToOperationConversion.java:
##########
@@ -168,29 +172,47 @@ private SqlNodeToOperationConversion(
* @param flinkPlanner FlinkPlannerImpl to convertCreateTable sql node to
rel node
* @param catalogManager CatalogManager to resolve full path for operations
* @param sqlNode SqlNode to execute on
+ * @param statement original SQL statement text, or {@code null} when the
node has no source
+ * text (e.g. a synthesized node)
*/
public static Optional<Operation> convert(
- FlinkPlannerImpl flinkPlanner, CatalogManager catalogManager,
SqlNode sqlNode) {
- // validate the query
+ FlinkPlannerImpl flinkPlanner,
+ CatalogManager catalogManager,
+ SqlNode sqlNode,
+ @Nullable String statement) {
Review Comment:
usually the JavaDocs already hint how you should call a variable
`originalSql`
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConvertUtils.java:
##########
@@ -46,14 +48,58 @@
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.OptionalInt;
import java.util.stream.Collectors;
/** Utilities for SqlNode conversions. */
-class SqlNodeConvertUtils {
+public class SqlNodeConvertUtils {
+
+ /**
+ * Returns the {@code AS}-query verbatim: the statement text from just
after {@code AS} to its
+ * end, trimmed. Slicing to the statement end (the {@code AS}-query is the
last clause) keeps a
+ * comment after {@code AS} and queries whose node position is narrower
than their text, e.g.
+ * {@code WITH ... SELECT}.
+ *
+ * @param asKeywordPos parser position of the {@code AS} keyword
+ * @return the verbatim AS-query, or empty when no statement text is
available
+ */
+ public static Optional<String> originalAsQueryText(
Review Comment:
methods start with a verb
```suggestion
public static Optional<String> extractOriginalAsQueryText(
```
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/SqlNodeToOperationConversion.java:
##########
@@ -151,13 +151,17 @@
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) {
Review Comment:
```suggestion
@Nullable String originalSql) {
```
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlDdlToOperationConverterTest.java:
##########
@@ -2591,6 +2591,127 @@ 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 AS query are kept",
+ "/* leading comment */\nSELECT 1\n/* trailing comment
*/"),
+ viewAsQueryCase(
+ "block comment inside the AS query is kept",
+ "SELECT /* inline comment */ 1"),
+ viewAsQueryCase(
+ "trailing comment after the AS query is kept",
+ "SELECT 1 -- trailing comment"));
+ }
+
+ private static Stream<Arguments> viewLineBreakCases() {
+ return Stream.of(
+ Arguments.of(
+ "line comment mentioning AS before the AS keyword is
dropped",
+ "CREATE VIEW v1 --AS\nAS SELECT 1",
+ "SELECT 1"),
+ viewAsQueryCase(
+ "line comment mentioning AS between AS and the query
is kept",
+ "--AS\nSELECT 1"),
+ viewAsQueryCase(
+ "line comments mentioning AS in mixed case between AS
and the query are kept",
+ "--line AS\n--comment as\n--break As\nSELECT 1"),
+ Arguments.of(
+ "blank lines and line comments between AS and the
query are kept",
+ "CREATE VIEW v1 AS \n\n\n--AS\n
--line\n--comment\n--break\nSELECT 1",
+ "--AS\n --line\n--comment\n--break\nSELECT 1"),
+ viewAsQueryCase(
+ "multi-line block comment between AS and the query is
kept",
+ "/* multi\n line */\nSELECT 1"),
+ Arguments.of(
+ "multi-line block comment before AS is dropped",
+ "CREATE VIEW v1\n/* note\n before AS */\nAS SELECT 1",
+ "SELECT 1"),
+ Arguments.of(
+ "column list and comment before AS are dropped",
+ "CREATE VIEW v1 (w, x, y, z)\nCOMMENT 'a view'\nAS
SELECT a, b, c, d FROM t1",
+ "SELECT a, b, c, d FROM t1"));
+ }
+
+ private static Stream<Arguments> viewComplexQueryShapeCases() {
Review Comment:
same for MTs
##########
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:
put comment into code
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlDdlToOperationConverterTest.java:
##########
@@ -2591,6 +2591,127 @@ 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 AS query are kept",
+ "/* leading comment */\nSELECT 1\n/* trailing comment
*/"),
+ viewAsQueryCase(
+ "block comment inside the AS query is kept",
+ "SELECT /* inline comment */ 1"),
+ viewAsQueryCase(
+ "trailing comment after the AS query is kept",
+ "SELECT 1 -- trailing comment"));
+ }
+
+ private static Stream<Arguments> viewLineBreakCases() {
+ return Stream.of(
+ Arguments.of(
+ "line comment mentioning AS before the AS keyword is
dropped",
+ "CREATE VIEW v1 --AS\nAS SELECT 1",
+ "SELECT 1"),
+ viewAsQueryCase(
+ "line comment mentioning AS between AS and the query
is kept",
+ "--AS\nSELECT 1"),
+ viewAsQueryCase(
+ "line comments mentioning AS in mixed case between AS
and the query are kept",
+ "--line AS\n--comment as\n--break As\nSELECT 1"),
+ Arguments.of(
+ "blank lines and line comments between AS and the
query are kept",
+ "CREATE VIEW v1 AS \n\n\n--AS\n
--line\n--comment\n--break\nSELECT 1",
+ "--AS\n --line\n--comment\n--break\nSELECT 1"),
+ viewAsQueryCase(
+ "multi-line block comment between AS and the query is
kept",
+ "/* multi\n line */\nSELECT 1"),
+ Arguments.of(
+ "multi-line block comment before AS is dropped",
+ "CREATE VIEW v1\n/* note\n before AS */\nAS SELECT 1",
+ "SELECT 1"),
+ Arguments.of(
+ "column list and comment before AS are dropped",
+ "CREATE VIEW v1 (w, x, y, z)\nCOMMENT 'a view'\nAS
SELECT a, b, c, d FROM t1",
+ "SELECT a, b, c, d FROM t1"));
+ }
+
+ private static Stream<Arguments> viewComplexQueryShapeCases() {
Review Comment:
I think we can significantly reduce the number of test cases here. My
concern was more about stuff in front of the AS, not afterwards
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/SqlNodeConvertContext.java:
##########
@@ -51,10 +51,15 @@ public class SqlNodeConvertContext implements
SqlNodeConverter.ConvertContext {
private final FlinkPlannerImpl flinkPlanner;
private final CatalogManager catalogManager;
+ private final @Nullable String statement;
Review Comment:
```suggestion
private final @Nullable String originalSql;
```
--
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]
