xiangfu0 commented on code in PR #19263:
URL: https://github.com/apache/pinot/pull/19263#discussion_r4067182449
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -129,6 +129,7 @@ public static SqlNodeAndOptions
compileToSqlNodeAndOptions(String sql)
try (StringReader inStream = new StringReader(sql)) {
SqlParserImpl sqlParser = newSqlParser(inStream);
SqlNodeList sqlNodeList = sqlParser.parseSqlStmtList();
+ sqlNodeList = (SqlNodeList) PostgreSqlCastRewriter.rewrite(sqlNodeList);
Review Comment:
Thanks. This was a real bug, and your repro matched what I saw:
`SqlPhysicalExplain` → `SqlExplain`, and `SqlPinotCreateMaterializedView`/DDL →
`SqlBasicCall`/DQL, including with a nested subquery in the view body.
Fixed in 6e2b49bf8c with a variant of option 1 that doesn't need to know the
statement types. The rewriter no longer rebuilds anything: it replaces only the
constant's direct parent in place (`SqlCall.setOperand` / `SqlNodeList.set`),
so every other node keeps its identity and class. I preferred this to
classifying first because 13 Pinot node types subclass `SqlCall`, and any of
them that holds a query would have hit the same bug. It is also smaller than
option 2.
For it to be safe, every node that can directly hold a bytea constant has to
support `setOperand`. I checked Calcite 1.42 and the grammar:
- Every node under which Pinot's grammar accepts an arbitrary expression
supports it: `SqlBasicCall`, `SqlNodeList`, `SqlSelect`, `SqlCase`, `SqlJoin`,
`SqlWindow`, `SqlExplain`, `SqlWith`, and the rest.
- The nodes that don't only appear where the grammar is restricted:
`SqlOrderBy` offset/fetch (numeric only), `SqlHint`, `SqlTableRef`, and star
`EXCLUDE`/`REPLACE` (disabled).
- Pinot's own nodes hold queries, identifiers and literals. The one that
holds an expression, `SqlPinotColumnDeclaration`'s `DEFAULT`, only accepts
`Literal()`.
If a future node does hold a constant somewhere it can't be replaced, the
rewriter throws a `SqlCompilationException` naming the node instead of
rebuilding it.
Regression tests, each of which fails against the previous copy-on-write
rewriter:
- `CalciteSqlParserTest.testPostgreSqlByteaLiteralPreservesStatementNode`
checks the node class and `PinotSqlType` for `EXPLAIN IMPLEMENTATION PLAN FOR`,
`EXPLAIN PLAN FOR`, and `CREATE MATERIALIZED VIEW`, with the constant both
directly in the body and in a nested subquery. Each case runs with
`'\x01'::bytea`, `CAST('\x01' AS BYTEA)` and `X'01'`, and asserts the constant
was still normalized.
- `QueryCompilationTest.testPostgreSqlByteaLiteralKeepsPhysicalExplain`
checks the user-visible symptom: the `EXPLAIN IMPLEMENTATION PLAN` output must
equal the `X'01'` physical plan. Against the old code it gets the logical plan
back.
- `DdlCompilerMaterializedViewTest.definedSqlWithPostgreSqlByteaConstant`
compiles a real materialized view end to end and asserts the stored
`definedSQL`. Against the old code, the DDL compiler rejects it as an
unsupported statement.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
