yashmayya commented on code in PR #19263:
URL: https://github.com/apache/pinot/pull/19263#discussion_r3941602983
##########
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:
This runs the shuttle over the whole statement list, so it also walks
Pinot's own statement nodes.
`SqlShuttle` rebuilds every ancestor of a changed node with
`operator.createCall(...)`, and that call returns the operator's own node type.
`SqlPhysicalExplain` inherits `SqlExplain.OPERATOR`, whose `createCall`
hardcodes `new SqlExplain(...)`. `SqlPinotCreateMaterializedView` uses a bare
`SqlSpecialOperator` with no `createCall` override, so it rebuilds as
`SqlBasicCall`. One bytea literal anywhere inside such a statement replaces the
statement node with a different class.
I ran both cases against this branch:
```
EXPLAIN IMPLEMENTATION PLAN FOR SELECT a FROM t WHERE b = X'01'
-> SqlPhysicalExplain
EXPLAIN IMPLEMENTATION PLAN FOR SELECT a FROM t WHERE b = '\x01'::bytea
-> org.apache.calcite.sql.SqlExplain
```
`QueryEnvironment:967` picks the physical plan with `explain instanceof
SqlPhysicalExplain`. After the downgrade that test is false, so the broker
returns the logical plan. The user gets no error.
```
CREATE MATERIALIZED VIEW mv AS SELECT a FROM t WHERE b = X'01'
-> SqlPinotCreateMaterializedView, sqlType=DDL
CREATE MATERIALIZED VIEW mv AS SELECT a FROM t WHERE b = '\x01'::bytea
-> SqlBasicCall, sqlType=DQL
```
`extractSqlNodeAndOptions` matches on the node class, so the statement
becomes DQL and the DDL path never runs.
`CAST('\x01' AS BYTEA)` reproduces both, so `::` is not needed to hit this.
A bytea literal in a nested subquery of the view body triggers it too.
Two ways to correct this:
1. Classify the statement first, then rewrite only the query node. For
`SqlExplain`, rewrite operand 0 in place with `setOperand`.
2. Give each custom node's operator a `createCall` override that rebuilds
the concrete class, and give `SqlPhysicalExplain` its own `OPERATOR`.
Option 1 is smaller. Option 2 also protects these nodes from the next
shuttle someone adds.
Please add regression tests that assert the node class and `PinotSqlType`
for `EXPLAIN IMPLEMENTATION PLAN FOR` and for `CREATE MATERIALIZED VIEW` with a
bytea literal. The current tests cover only a plain `SELECT`, so both defects
pass CI.
--
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]
