seb-pereira commented on code in PR #27445:
URL: https://github.com/apache/flink/pull/27445#discussion_r2709935324
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlNodeConvertUtils.java:
##########
@@ -85,7 +85,7 @@ static CatalogView toCatalogView(
context.getSqlValidator().getNamespace(validateQuery);
validateDuplicatedColumnNames(query, viewFields, validatedNamespace);
- String expandedQuery = context.toQuotedSqlString(query);
+ String expandedQuery = context.toQuotedSqlString(validateQuery);
Review Comment:
I understand the concern; I have done this test
https://github.com/seb-pereira/calcite/commit/95bc9adebfbe05cfce230e85e5605604fe02d5de
to confirm the behaviour of Calcite
[`SqlValidator.validate()`](https://github.com/apache/calcite/blob/calcite-1.36.0/core/src/main/java/org/apache/calcite/sql/validate/SqlValidator.java#L143)
and it confirms:
- `validate()` mutates the input parameter; its comment states the method
returns a "validated node (possibly rewritten)", so I guess this is expected
behaviour.
- input parameter ends up with duplicated `ORDER BY`; as input can mutate
and I do not know much about Calcite, I have no idea if this should be
considered as a bug or not...
- output parameter is valid
In Flink, I observe the pattern is to always use the returned value (valid)
from `SqlValidator.validate()`:
-
[SqlNodeToOperationConversion.java](https://github.com/apache/flink/blob/release-2.2.0/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/SqlNodeToOperationConversion.java#L219L221)
-
[SqlCreateTableAsConverter.java](https://github.com/apache/flink/blob/release-2.2.0/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/table/SqlCreateTableAsConverter.java#L50L62)
-
[Expander.java](https://github.com/apache/flink/blob/release-2.2.0/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/utils/Expander.java#L86L112)
-
[SqlAlterMaterializedTableAsQueryConverter.java](https://github.com/apache/flink/blob/release-2.2.0/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlAlterMaterializedTableAsQueryConverter.java#L53L58)
-
[SqlCreateMaterializedTableConverter.java](https://github.com/apache/flink/blob/release-2.2.0/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/converters/SqlCreateMaterializedTableConverter.java#L105L111)
Given that the output is valid, from Flink perspective I think
`validateQuery` is the way to go instead of `query`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
