Re: [PR] Support PostgreSQL hex bytea literals [pinot]

["yashmayya (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org