You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "raminqaf (via GitHub)" <gi...@apache.org> on 2026/05/29 09:12:53 UTC
[PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
raminqaf opened a new pull request, #28277:
URL: https://github.com/apache/flink/pull/28277
## What is the purpose of the change
`CREATE/ALTER MATERIALIZED TABLE … AS …` and `CREATE VIEW … AS …` currently persist `originalQuery` by re-rendering the parsed `SqlNode` via `SqlNode#toSqlString()`. This normalizes identifier casing, expands type aliases (`int` → `INTEGER`), reflows whitespace, and rewrites quoting, so `SHOW CREATE MATERIALIZED TABLE` / `SHOW CREATE VIEW` no longer reflect what the user typed.
This PR makes `originalQuery` the verbatim substring of the user's input statement, sliced via `SqlParserPos` of the AS-query subtree. `expandedQuery` continues to hold the planner-normalized form.
### Example
Input:
```sql
CREATE MATERIALIZED TABLE mt AS
select a, b, cast(c as int) as int_c from t1 where c > 200
```
Before:
```sql
SELECT `a`, `b`, CAST(`c` AS INTEGER) AS `int_c`
FROM `t1`
WHERE `c` > 200
```
after:
```sql
select a, b, cast(c as int) as int_c from t1 where c > 200
```
## Brief Changelog
- Thread original SQL statement text from ParserImpl through SqlNodeToOperationConversion.convert(...) into SqlNodeConvertContext.
- Add ConvertContext#toRawSqlString(SqlNode) returning the substring of the original statement matching SqlParserPos. Counterpart to existing toQuotedSqlString(SqlNode). Falls back to toQuotedSqlString if raw statement text is unavailable.
- Switch AbstractCreateMaterializedTableConverter#getDerivedOriginalQuery, SqlAlterMaterializedTableAsQueryConverter, and SqlNodeConvertUtils#toCatalogView (covers CREATE VIEW and ALTER VIEW … AS) to the new method.
- Keep existing SqlNodeToOperationConversion.convert(...) signature as an overload that delegates with null raw text for backward compatibility.
## Verifying this Change
- Unit tests in SqlNodeConvertContextTest for toRawSqlString covering nested AS-query nodes, comments inside the query, and statements spanning multiple lines.
- Planner tests in MaterializedTableTest and ViewTest asserting originalQuery equals the input substring across mixed casing, type aliases (int/varchar), whitespace, and quoting styles. Existing expandedQuery assertions retained unchanged.
- Updated planner tests that previously asserted the normalized form of originalQuery.
- SQL gateway round-trip test: create a materialized table / view with mixed-case query, run SHOW CREATE and assert the output preserves user wording.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with @Public(Evolving): no (signature change on SqlNodeToOperationConversion is @Internal; backward-compatible overload kept)
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
- Note: catalog entries persisted before this change still hold the normalized originalQuery. No migration performed.
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not applicable
#### Was generative AI tooling used to co-author this PR?
- [x] Yes - Claude Opus 4.7 to go through the implementation once and check for inconsistencies
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3324267966
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/TableChange.java:
##########
@@ -1435,15 +1434,15 @@ public String toString() {
class ModifyDefinitionQuery implements MaterializedTableChange {
private final String originalQuery;
- private final String definitionQuery;
+ private final String expandedQuery;
- public ModifyDefinitionQuery(String originalQuery, String definitionQuery) {
+ public ModifyDefinitionQuery(String originalQuery, String expandedQuery) {
this.originalQuery = originalQuery;
- this.definitionQuery = definitionQuery;
+ this.expandedQuery = expandedQuery;
}
- public String getDefinitionQuery() {
- return definitionQuery;
+ public String getExpandedQuery() {
Review Comment:
there is a doc telling about this
in case we revert it, then should revert everywhere the change `definitionQuery` -> `expandedQuery`
otherwise in some places it is expanded, in other it is definition
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "AHeise (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3356698778
##########
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:
Looking into this again, I have to correct myself:
the `SqlAlterMaterializedTableAsQuery` is only called after the `AS` token is consumed so it is always available and **non**-nullable. Excuse me for the confusion. Corrected it.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin merged PR #28277:
URL: https://github.com/apache/flink/pull/28277
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3324234696
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/TableChange.java:
##########
@@ -1435,15 +1434,15 @@ public String toString() {
class ModifyDefinitionQuery implements MaterializedTableChange {
private final String originalQuery;
- private final String definitionQuery;
+ private final String expandedQuery;
- public ModifyDefinitionQuery(String originalQuery, String definitionQuery) {
+ public ModifyDefinitionQuery(String originalQuery, String expandedQuery) {
this.originalQuery = originalQuery;
- this.definitionQuery = definitionQuery;
+ this.expandedQuery = expandedQuery;
}
- public String getDefinitionQuery() {
- return definitionQuery;
+ public String getExpandedQuery() {
Review Comment:
IMO we should tolerate these type of changes after a minor release. For now reverted it get things unblocked.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3323533416
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlNodeToOperationSqlCreateOrAlterMaterializedTableConverterTest.java:
##########
@@ -201,7 +201,7 @@ void testCreateOrAlterMaterializedTableForExistingTable() throws TableNotExistEx
TableChange.add(Column.physical("e", DataTypes.VARCHAR(Integer.MAX_VALUE))),
TableChange.add(Column.physical("f", DataTypes.VARCHAR(Integer.MAX_VALUE))),
TableChange.modifyDefinitionQuery(
- "SELECT `a`, `b`, `c`, `d`, `d` AS `e`, CAST('123' AS STRING) AS `f`\nFROM `t1`",
+ "SELECT a, b, c, d, d as e, cast('123' as string) as f FROM t1",
Review Comment:
can we have tests for queries containing different 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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3356732247
##########
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:
addressed
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "twalthr (via GitHub)" <gi...@apache.org>.
twalthr commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3334787309
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/materializedtable/AbstractCreateMaterializedTableConverter.java:
##########
@@ -141,10 +141,20 @@ protected final RefreshMode getDerivedRefreshMode(LogicalRefreshMode logicalRefr
return MaterializedTableUtils.fromLogicalRefreshModeToRefreshMode(logicalRefreshMode);
}
+ /**
+ * Returns the {@code AS} query exactly as the user wrote it.
+ *
+ * <p>The query text is sliced out of the original sql statement using the parser position
Review Comment:
nit: SQL should be upper case in comments.
```suggestion
* <p>The query text is sliced out of the original SQL statement using the parser position
```
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConvertUtils.java:
##########
@@ -110,6 +103,14 @@ static CatalogView toCatalogView(
schema = ResolvedSchema.physical(aliasFieldNames, schema.getColumnDataTypes());
}
+ // Capture the user's original wording (case, formatting, type aliases) by slicing the
+ // sql statement using the parser position on the query node, instead of unparsing the
+ // SqlNode. Re-rendering the parsed tree would normalize identifier casing and expand
+ // type aliases (e.g. `int` to `INTEGER`), which is undesirable for the original query
+ // we persist on the catalog view.
+ final String originalQuery =
+ context.toRawSqlString(query).orElse(context.toQuotedSqlString(query));
Review Comment:
Is there a chance this can ever happen? If not just throw. Otherwise it just spams the code.
```suggestion
context.toRawSqlString(query).orElseThfow(IllegalState::new);
```
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/table/SqlCreateTableAsConverter.java:
##########
@@ -47,21 +47,30 @@ public class SqlCreateTableAsConverter extends AbstractCreateTableConverter<SqlC
public Operation convertSqlNode(SqlCreateTableAs sqlCreateTableAs, ConvertContext context) {
final FlinkPlannerImpl flinkPlanner = context.getFlinkPlanner();
final CatalogManager catalogManager = context.getCatalogManager();
- SqlNode asQuerySqlNode = sqlCreateTableAs.getAsQuery();
- SqlNode validatedAsQuery = flinkPlanner.validate(asQuerySqlNode);
+ final SqlNode asQuerySqlNode = sqlCreateTableAs.getAsQuery();
+ final SqlNode validatedAsQuery = flinkPlanner.validate(asQuerySqlNode);
+ final String rawSqlString =
+ context.toRawSqlString(asQuerySqlNode)
+ .orElseThrow(
+ () ->
+ new TableException(
+ "Failed to get raw SQL string for the AS query in CREATE TABLE AS statement."));
Review Comment:
Why do we throw for some cases and for other we fall back to quoted?
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlMaterializedTableNodeToOperationConverterTest.java:
##########
@@ -320,6 +320,38 @@ void testCreateMaterializedTableWithUDTFQuery() {
+ "LATERAL TABLE(`builtin`.`default`.`myFunc`(`b`)) AS `T` (`f1`, `f2`)");
}
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("originalQueryCommentCases")
+ void testOriginalQueryCommentHandling(String name, String sql, String expectedOriginalQuery) {
+ final String originalQuery =
+ createMaterializedTableOperation(sql)
+ .getCatalogMaterializedTable()
+ .getOriginalQuery();
+ assertThat(originalQuery).isEqualTo(expectedOriginalQuery);
+ }
+
+ private static Collection<Arguments> originalQueryCommentCases() {
+ // comments before the first token or after the last token fall outside the node and are
+ // not preserved.
+ return List.of(
+ Arguments.of(
+ "inline line comment inside query is kept",
Review Comment:
can you test:
```
CREATE MATERIALIZED TABLE `mtbl` AS
CREATE MATERIALIZED TABLE `m``tbl` AS
CREATE MATERIALIZED TABLE `m``tbl` COMMENT 'a' AS
CREATE MATERIALIZED TABLE `m``tbl` COMMENT 'a' || 'a' AS
```
a bit more sophisticated prefix, to check the parser pos logic.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3324210564
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/SqlNodeConvertContext.java:
##########
@@ -110,6 +127,55 @@ public String toQuotedSqlString(SqlNode sqlNode) {
return sqlNode.toSqlString(getSqlDialect()).getSql();
}
+ @Override
+ public Optional<String> toRawSqlString(SqlNode node) {
Review Comment:
No, and that's expected. I ran your exact snippet:
```
outer statement "CREATE TABLE x AS SELECT a FROM t1"
innerAsQuery.getParserPosition() -> line 1, column 19
new SqlNodeConvertContext(planner, catalogManager, outerStatement).toRawSqlString(innerAsQuery)
-> Optional[SELECT a FROM t1]
new SqlNodeConvertContext(planner, catalogManager, "SELECT a FROM t1").toRawSqlString(innerAsQuery)
-> Optional.empty
```
the method has a precondition: the statement passed to the context must be the same text the node was parsed from. When that holds, the slice is correct. It fails safe. It never returns a wrong substring and never throws.
I will update the javaDocs with better explanation of this case.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3324210564
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/SqlNodeConvertContext.java:
##########
@@ -110,6 +127,55 @@ public String toQuotedSqlString(SqlNode sqlNode) {
return sqlNode.toSqlString(getSqlDialect()).getSql();
}
+ @Override
+ public Optional<String> toRawSqlString(SqlNode node) {
Review Comment:
No, and that's expected. I ran your exact snippet:
```
outer statement "CREATE TABLE x AS SELECT a FROM t1"
innerAsQuery.getParserPosition() -> line 1, column 19
new SqlNodeConvertContext(planner, catalogManager, outerStatement).toRawSqlString(innerAsQuery)
-> Optional[SELECT a FROM t1]
new SqlNodeConvertContext(planner, catalogManager, "SELECT a FROM t1").toRawSqlString(innerAsQuery)
-> Optional.empty
```
the method has a precondition: the statement passed to the context must be the same text the node was parsed from. When that holds, the slice is correct. It fails safe. It never returns a wrong substring and never throws.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3360837980
##########
flink-table/flink-sql-parser/src/main/codegen/includes/parserImpls.ftl:
##########
@@ -1923,6 +1923,7 @@ SqlCreate SqlCreateOrAlterMaterializedTable(Span s, boolean replace, boolean isT
SqlNode freshness = null;
SqlRefreshMode refreshMode = null;
SqlNode asQuery = null;
+ SqlParserPos asKeywordPos = null;
Review Comment:
As far as I understood and debuged the `asQuery` SqlNode` does not contain the `AS` keyword. In other words, it starts from the `S` of the `SELECT...`.
For instance the debugger shows on this query
```sql
CREATE MATERIALIZED TABLE base_mtbl (
CONSTRAINT ct1 PRIMARY KEY(a) NOT ENFORCED)
COMMENT 'materialized table comment'
PARTITIONED BY (a, d)
WITH (
'connector' = 'filesystem',
'format' = 'json'
)
FRESHNESS = INTERVAL '30' SECOND
REFRESH_MODE = FULL
AS SELECT * FROM t1
```
<img width="451" height="339" alt="image" src="https://github.com/user-attachments/assets/fc97fd40-8aab-4c7f-bc67-3d1dc701b065" />
##########
flink-table/flink-sql-parser/src/main/codegen/includes/parserImpls.ftl:
##########
@@ -1923,6 +1923,7 @@ SqlCreate SqlCreateOrAlterMaterializedTable(Span s, boolean replace, boolean isT
SqlNode freshness = null;
SqlRefreshMode refreshMode = null;
SqlNode asQuery = null;
+ SqlParserPos asKeywordPos = null;
Review Comment:
As far as I understood and debugged the `asQuery` SqlNode` does not contain the `AS` keyword. In other words, it starts from the `S` of the `SELECT...`.
For instance the debugger shows on this query
```sql
CREATE MATERIALIZED TABLE base_mtbl (
CONSTRAINT ct1 PRIMARY KEY(a) NOT ENFORCED)
COMMENT 'materialized table comment'
PARTITIONED BY (a, d)
WITH (
'connector' = 'filesystem',
'format' = 'json'
)
FRESHNESS = INTERVAL '30' SECOND
REFRESH_MODE = FULL
AS SELECT * FROM t1
```
<img width="451" height="339" alt="image" src="https://github.com/user-attachments/assets/fc97fd40-8aab-4c7f-bc67-3d1dc701b065" />
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3354310991
##########
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:
Changes are minimal:
- extracted asQuery as variable
- renamed `definitionQuery` to `expandedQuery`
- Moved the extraction of the `originalQuery` closer to its caller
Should I revert and keep it even smaller?
##########
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:
Changes are minimal:
- extracted `asQuery` as variable
- renamed `definitionQuery` to `expandedQuery`
- Moved the extraction of the `originalQuery` closer to its caller
Should I revert and keep it even smaller?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3354893063
##########
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:
Reverted the javadocs and added a `@param`
##########
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:
added
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "twalthr (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3356636007
##########
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:
Fair enough. I just added these to make sure the approach is sound and there is no doubt in the logic.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3357579715
##########
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/materializedtable/SqlAlterMaterializedTableAsQuery.java:
##########
@@ -36,16 +36,24 @@ public class SqlAlterMaterializedTableAsQuery extends SqlAlterMaterializedTable
private final SqlNode asQuery;
+ private final SqlParserPos asKeywordPos;
+
public SqlAlterMaterializedTableAsQuery(
- SqlParserPos pos, SqlIdentifier tableName, SqlNode asQuery) {
+ SqlParserPos pos, SqlIdentifier tableName, SqlNode asQuery, SqlParserPos asKeywordPos) {
super(pos, tableName);
this.asQuery = asQuery;
+ this.asKeywordPos = asKeywordPos;
}
public SqlNode getAsQuery() {
return asQuery;
}
+ /** Returns the parser position of the {@code AS} keyword. */
+ public SqlParserPos getAsKeywordPos() {
Review Comment:
```suggestion
public SqlParserPos getAsQueryPos() {
```
the method name is vague since
there might be multiple `AS`
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3324089142
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/materializedtable/AbstractCreateMaterializedTableConverter.java:
##########
@@ -141,10 +141,20 @@ protected final RefreshMode getDerivedRefreshMode(LogicalRefreshMode logicalRefr
return MaterializedTableUtils.fromLogicalRefreshModeToRefreshMode(logicalRefreshMode);
}
+ /**
+ * Returns the {@code AS} query exactly as the user wrote it.
+ *
+ * <p>The query text is sliced out of the original sql statement using the parser position
+ * recorded on the {@link SqlNode}. This avoids the loss of formatting and identifier casing
+ * that would otherwise occur if the query were re-rendered from the parsed tree (e.g. {@code
+ * int} would be normalized to {@code INTEGER}).
+ *
+ * <p>If the raw sql string cannot be fetched it fals back to the quoted sql string.
Review Comment:
```suggestion
* <p>If the raw sql string cannot be fetched it falls back to the quoted sql string.
```
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3324059478
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/SqlNodeConvertContext.java:
##########
@@ -110,6 +127,55 @@ public String toQuotedSqlString(SqlNode sqlNode) {
return sqlNode.toSqlString(getSqlDialect()).getSql();
}
+ @Override
+ public Optional<String> toRawSqlString(SqlNode node) {
Review Comment:
is the implementation consistent?
what happen if
```java
final String outerStatement = "CREATE TABLE x AS SELECT a FROM t1";
final FlinkPlannerImpl planner = getPlannerBySqlDialect(SqlDialect.DEFAULT);
final CalciteParser parser = getParserBySqlDialect(SqlDialect.DEFAULT);
final SqlNode innerAsQuery =
((SqlCreateTableAs) parser.parse(outerStatement)).getAsQuery();
new SqlNodeConvertContext(planner, catalogManager, outerStatement)
.toRawSqlString(innerAsQuery));
// AND
new SqlNodeConvertContext(planner, catalogManager, "SELECT a FROM t1")
.toRawSqlString(innerAsQuery));
```
will they have same result?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3324243502
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/SqlNodeConvertContext.java:
##########
@@ -28,6 +28,7 @@
import org.apache.flink.table.planner.utils.Expander;
import org.apache.flink.table.types.DataType;
+import com.google.common.base.Splitter;
Review Comment:
Addressed
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3360842668
##########
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/materializedtable/SqlAlterMaterializedTableAsQuery.java:
##########
@@ -36,16 +36,24 @@ public class SqlAlterMaterializedTableAsQuery extends SqlAlterMaterializedTable
private final SqlNode asQuery;
+ private final SqlParserPos asKeywordPos;
Review Comment:
Good point here. But I am also not the biggest fan of `asQueryPos`. Since the `asQuery` contains `SELECT...` and not even the `AS` keyword
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3354274365
##########
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:
The `AS` keyword in `ALTER VIEW` is optional
```sql
ALTER VIEW v1 SET TBLPROPERTIES ('p1' = 'v1');
```
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #28277:
URL: https://github.com/apache/flink/pull/28277#issuecomment-4573136589
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e695caa5b4101fe4bb80ba2747464328171f4c18",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "e695caa5b4101fe4bb80ba2747464328171f4c18",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e695caa5b4101fe4bb80ba2747464328171f4c18 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3323862642
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/SqlNodeConvertContext.java:
##########
@@ -28,6 +28,7 @@
import org.apache.flink.table.planner.utils.Expander;
import org.apache.flink.table.types.DataType;
+import com.google.common.base.Splitter;
Review Comment:
we shouldn't used non shaded guava
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/SqlNodeConvertContext.java:
##########
@@ -28,6 +28,7 @@
import org.apache.flink.table.planner.utils.Expander;
import org.apache.flink.table.types.DataType;
+import com.google.common.base.Splitter;
Review Comment:
we shouldn't use non shaded guava
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3324115181
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/table/SqlCreateTableAsConverter.java:
##########
@@ -50,10 +50,19 @@ public Operation convertSqlNode(SqlCreateTableAs sqlCreateTableAs, ConvertContex
SqlNode asQuerySqlNode = sqlCreateTableAs.getAsQuery();
SqlNode validatedAsQuery = flinkPlanner.validate(asQuerySqlNode);
+ final String rawSqlString =
Review Comment:
is there a reason to have one var `final` while others are not?
can we make all final or all not final?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3360843372
##########
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/materializedtable/SqlAlterMaterializedTableAsQuery.java:
##########
@@ -36,16 +36,24 @@ public class SqlAlterMaterializedTableAsQuery extends SqlAlterMaterializedTable
private final SqlNode asQuery;
+ private final SqlParserPos asKeywordPos;
+
public SqlAlterMaterializedTableAsQuery(
- SqlParserPos pos, SqlIdentifier tableName, SqlNode asQuery) {
+ SqlParserPos pos, SqlIdentifier tableName, SqlNode asQuery, SqlParserPos asKeywordPos) {
super(pos, tableName);
this.asQuery = asQuery;
+ this.asKeywordPos = asKeywordPos;
}
public SqlNode getAsQuery() {
return asQuery;
}
+ /** Returns the parser position of the {@code AS} keyword. */
+ public SqlParserPos getAsKeywordPos() {
Review Comment:
Please see: https://github.com/apache/flink/pull/28277#discussion_r3360842668
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3356684864
##########
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:
Addressed
##########
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:
Addressed
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3361423721
##########
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/materializedtable/SqlAlterMaterializedTableAsQuery.java:
##########
@@ -36,16 +36,24 @@ public class SqlAlterMaterializedTableAsQuery extends SqlAlterMaterializedTable
private final SqlNode asQuery;
+ private final SqlParserPos asKeywordPos;
Review Comment:
Sounds good to me!
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3354269919
##########
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:
The `AS` keyword in alter is optional.
```sql
ALTER MATERIALIZED TABLE mt SUSPEND;
```
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3354894052
##########
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:
added
##########
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:
Added
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3324105914
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/MergeTableAsUtil.java:
##########
@@ -156,7 +156,7 @@ public PlannerQueryOperation maybeRewriteQuery(
() -> "Unsupported node type " + origQueryNode.getKind());
return (PlannerQueryOperation)
- SqlNodeToOperationConversion.convert(flinkPlanner, catalogManager, newSelect)
+ SqlNodeToOperationConversion.convert(flinkPlanner, catalogManager, newSelect, null)
Review Comment:
why is it null here?
could it be potentially in the future same issue as with CTAS/RTAS?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3324212984
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/table/SqlCreateTableAsConverter.java:
##########
@@ -50,10 +50,19 @@ public Operation convertSqlNode(SqlCreateTableAs sqlCreateTableAs, ConvertContex
SqlNode asQuerySqlNode = sqlCreateTableAs.getAsQuery();
SqlNode validatedAsQuery = flinkPlanner.validate(asQuerySqlNode);
+ final String rawSqlString =
Review Comment:
Made everything final in this method
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3334534806
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/materializedtable/AbstractCreateMaterializedTableConverter.java:
##########
@@ -141,10 +141,20 @@ protected final RefreshMode getDerivedRefreshMode(LogicalRefreshMode logicalRefr
return MaterializedTableUtils.fromLogicalRefreshModeToRefreshMode(logicalRefreshMode);
}
+ /**
+ * Returns the {@code AS} query exactly as the user wrote it.
Review Comment:
is this statement true?
what if there is a query like
```sql
create view myview
/* this is my view start comment */
select 1;
/* this is my view end comment */
```
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/materializedtable/AbstractCreateMaterializedTableConverter.java:
##########
@@ -141,10 +141,20 @@ protected final RefreshMode getDerivedRefreshMode(LogicalRefreshMode logicalRefr
return MaterializedTableUtils.fromLogicalRefreshModeToRefreshMode(logicalRefreshMode);
}
+ /**
+ * Returns the {@code AS} query exactly as the user wrote it.
Review Comment:
is this statement true?
what if there is a query like
```sql
create view myview as
/* this is my view start comment */
select 1;
/* this is my view end 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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3357588128
##########
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/materializedtable/SqlAlterMaterializedTableAsQuery.java:
##########
@@ -36,16 +36,24 @@ public class SqlAlterMaterializedTableAsQuery extends SqlAlterMaterializedTable
private final SqlNode asQuery;
+ private final SqlParserPos asKeywordPos;
Review Comment:
```suggestion
private final SqlParserPos asQueryPos;
```
otherwise there might be a question
```sql
CREATE MATERIALIZED TABLE myMaterializedTable
AS SELECT 1 AS 'AS';
```
which `AS`?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3354420135
##########
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:
Renamed to `originalAsQueryText`. I agree with you that the `As...` in the method name makes it harder to read. In order to keep it consistent with the other variable like the `asQuery`, I would keep the `As...` in the method name.
##########
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:
`getPos()` returns the position of the token that was just consumed.
```sql
CREATE MATERIALIZED TABLE m AS SELECT 1
^^
cols 29–30 → getPos() = pos(line 1, col 29 → line 1, col 30)
```
`startPos.plus(getPos())` the position captured at the start of the statement (the CREATE keyword, col 1)
```sql
CREATE MATERIALIZED TABLE m AS SELECT 1
^---------------------------^
cols 1 .. 30 = the whole statement so far (CREATE … AS)
```
so `getPos()` here is correct, since we want just the position of the `AS` keyword.
##########
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:
Use `OptionalInt` now
##########
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:
Just wanted to have all the Overrides in the correct order. It is unrelated so I moved it back.
##########
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:
The wall of text is gone now
##########
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:
Made the method more general now
##########
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:
Removed the comments
##########
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:
Inverted and refactored the method
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3361387031
##########
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/materializedtable/SqlAlterMaterializedTableAsQuery.java:
##########
@@ -36,16 +36,24 @@ public class SqlAlterMaterializedTableAsQuery extends SqlAlterMaterializedTable
private final SqlNode asQuery;
+ private final SqlParserPos asKeywordPos;
Review Comment:
how about `asQueryKeywordPos`?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3356731286
##########
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:
addressed
##########
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:
addressed
##########
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:
addressed
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3356695344
##########
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:
Looking into this again, I have to correct myself:
the `SqlAlterViewAs` is only called after the `AS` token is consumed so it is always available and **non**-nullable.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3357602994
##########
flink-table/flink-sql-parser/src/main/codegen/includes/parserImpls.ftl:
##########
@@ -1923,6 +1923,7 @@ SqlCreate SqlCreateOrAlterMaterializedTable(Span s, boolean replace, boolean isT
SqlNode freshness = null;
SqlRefreshMode refreshMode = null;
SqlNode asQuery = null;
+ SqlParserPos asKeywordPos = null;
Review Comment:
why do we need it?
and why can't we use `pos` from `asQuery` `SqlNode`?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3323740909
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlNodeToOperationSqlCreateOrAlterMaterializedTableConverterTest.java:
##########
@@ -201,7 +201,7 @@ void testCreateOrAlterMaterializedTableForExistingTable() throws TableNotExistEx
TableChange.add(Column.physical("e", DataTypes.VARCHAR(Integer.MAX_VALUE))),
TableChange.add(Column.physical("f", DataTypes.VARCHAR(Integer.MAX_VALUE))),
TableChange.modifyDefinitionQuery(
- "SELECT `a`, `b`, `c`, `d`, `d` AS `e`, CAST('123' AS STRING) AS `f`\nFROM `t1`",
+ "SELECT a, b, c, d, d as e, cast('123' as string) as f FROM t1",
Review Comment:
Thanks for pointing this out. I have adde a parasitized test with for comments at different locations of the `asQuery`
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3323919950
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/TableChange.java:
##########
@@ -1435,15 +1434,15 @@ public String toString() {
class ModifyDefinitionQuery implements MaterializedTableChange {
private final String originalQuery;
- private final String definitionQuery;
+ private final String expandedQuery;
- public ModifyDefinitionQuery(String originalQuery, String definitionQuery) {
+ public ModifyDefinitionQuery(String originalQuery, String expandedQuery) {
this.originalQuery = originalQuery;
- this.definitionQuery = definitionQuery;
+ this.expandedQuery = expandedQuery;
}
- public String getDefinitionQuery() {
- return definitionQuery;
+ public String getExpandedQuery() {
Review Comment:
this is a `@PublicEvolving` interface, such change usually requires a FLIP
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3324173061
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/MergeTableAsUtil.java:
##########
@@ -156,7 +156,7 @@ public PlannerQueryOperation maybeRewriteQuery(
() -> "Unsupported node type " + origQueryNode.getKind());
return (PlannerQueryOperation)
- SqlNodeToOperationConversion.convert(flinkPlanner, catalogManager, newSelect)
+ SqlNodeToOperationConversion.convert(flinkPlanner, catalogManager, newSelect, null)
Review Comment:
The conversion path here never calls `toRawString` here. So it is safe. Even if it gets fired the `newSelect` is synthesized by `rewriteCall`, so the slicing would produce wrong values here. So the correct behavior would be to pass null to get an `optional#empty`
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-39799][table] Preserve user-typed query text for materialized table and view definitions [flink]
Posted by "raminqaf (via GitHub)" <gi...@apache.org>.
raminqaf commented on code in PR #28277:
URL: https://github.com/apache/flink/pull/28277#discussion_r3324173061
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/MergeTableAsUtil.java:
##########
@@ -156,7 +156,7 @@ public PlannerQueryOperation maybeRewriteQuery(
() -> "Unsupported node type " + origQueryNode.getKind());
return (PlannerQueryOperation)
- SqlNodeToOperationConversion.convert(flinkPlanner, catalogManager, newSelect)
+ SqlNodeToOperationConversion.convert(flinkPlanner, catalogManager, newSelect, null)
Review Comment:
The conversion path here never calls `toRawString` here. So it is safe. Even if it gets fired the newSelect is synthesized be rewriteCall, so the slicing would produce wrong values here. So the correct behavior would be to pass null to get an `optional#empty`
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org