Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19263 +/- ##
============================================
+ Coverage 67.69% 67.73% +0.03%
Complexity 1430 1430
============================================
Files 3488 3489 +1
Lines 224306 224354 +48
Branches 35416 35433 +17
============================================
+ Hits 151850 151956 +106
+ Misses 60447 60383 -64
- Partials 12009 12015 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ebc4124 to
ab14771
Compare
0e27011 to
9ffd670
Compare
f05c9bc to
15ef2c6
Compare
5bb7dea to
ae58f79
Compare
Accept PostgreSQL hex-format BYTEA constants in both the infix `'\x0102'::bytea`
form and the standard `CAST('\x0102' AS BYTEA)` spelling, normalizing them to
Pinot's existing binary literal representation before either engine plans the
query. The SQL-standard `X'0102'` form is unchanged.
Only quoted constants are normalized. A dynamic STRING-to-BYTES conversion would
have to be implemented per row in each engine, and the two would be easy to
drift apart. PostgreSQL's legacy octal escape format stays unsupported.
Parsing:
- `InfixCast` appends the operator and target type to the enclosing expression
list as a `SqlParserUtil.ToTreeListItem`, so precedence is resolved by
`Expression2` the same way as the adjacent `SqlAtTimeZone` and Calcite's own
Babel production. Collapsing the list eagerly would make `::` bind everything
to its left, which breaks the most common shape:
`WHERE bytesCol = '\x01'::bytea` would parse as
`CAST(bytesCol = '\x01' AS BYTEA)`.
- The `[]` and `.ident` postfix forms need no handling here; `Expression2`
already owns those branches.
Rewriting:
- Only ASCII hex digits and ASCII whitespace are accepted. `Character.digit` and
`Character.isWhitespace` also accept full-width and non-Latin forms, which
PostgreSQL rejects. Whitespace is allowed between byte pairs but not inside
one, matching PostgreSQL.
- A `::` whose target type does not survive as a type spec (`col::bytea[1]`,
where the item accessor binds tighter) is rejected rather than passed on as a
malformed call.
- Non-BYTEA `::` casts are rejected. This is a deliberate scope limit, not a
technical one: `INFIX_CAST` is an ordinary `SqlCastOperator` of kind `CAST`,
so `intCol::double` would plan like `CAST(intCol AS DOUBLE)`. Supporting the
full operator means committing to `::` type-name semantics across both
engines and is left to a follow-up. The grammar accepts `::` so that this
rewriter can report a clear error instead of a parse failure.
- Error messages name the offending type or expression and distinguish a
malformed hex constant from a non-constant operand from an unsupported target.
Tests cover scalar literals, BYTES array construction, bytea constants as the
right operand of `=`, `<>`, `>`, `<=` and inside compound predicates, agreement
between the `::`, `CAST` and `X'...'` spellings in both parse paths and both
cases, ingested multi-value BYTES predicates, planner type inference, and
runtime execution against H2.
ae58f79 to
4e70acf
Compare
yashmayya
left a comment
There was a problem hiding this comment.
Nice piece of work — the grammar production is a faithful port of Calcite Babel's InfixCast, the ToTreeListItem precedence handling is right, and the PR description is unusually clear about scope. I checked a few things that came back clean:
- The new
::token takes nothing away.<COLON>is used only inAddOptionalColonPath, which is gated onisColonFieldAccessAllowed()(false for everySqlConformanceEnum, including the BABEL one Pinot sets), and inJsonNameAndValuewith a single colon.a::bwas already a parse error. - Both parser entry points are patched, and they are the only two in the repo, so no engine bypasses the rewrite.
SqlShuttleandSqlBasicVisitorhave no instance fields, so the staticINSTANCEis safe to share, as the Javadoc says.normalizeHexmatches PostgreSQL'shex_decode: whitespace only at even byte boundaries, ASCII digits only.- I parsed about 50 query shapes (CTE, UNION, EXISTS,
FILTER, GROUPING SETS, window, JOIN,IN,CASE,BETWEEN,SET,OPTION) and every one reconstructs correctly.
One blocker, commented inline.
Blocker: the shuttle runs over the whole statement list and downgrades Pinot's custom statement nodes. EXPLAIN IMPLEMENTATION PLAN FOR a query with a bytea literal silently returns the logical plan, and CREATE MATERIALIZED VIEW with one is typed DQL instead of DDL. Details and two suggested fixes are in the inline comment.
Non-blocking notes, all at your discretion:
col::byteaerror names no alternative. Its two sibling messages both end with a remedy. This one leaves a PostgreSQL user at a dead end, and Pinot has what they want:hexToBytes(<expr>)converts a hex string column to BYTES per row. Worth appending.compileToExpressionhides these messages. The rewrite call sits inside thecatch (Throwable)at line 553, which does not appende.getMessage()— unlike line 148, which does. That is why the test has to assert ongetStackTrace(). Moving the call out of thetryfixes it, since it already throwsSqlCompilationException.- Two untested branches. The
source instanceof SqlBinaryStringLiteralearly return is the only support forX'0102'::byteaandCAST(X'0102' AS BYTEA), both advertised in the description, and neither has a test. The\xprefix check is case-sensitive on purpose (PostgreSQL is too), but nothing pins it — one'\X0102'::bytearow ininvalidPostgreSqlByteaLiteralswould. - Optional: guard the shuttle on a text scan. I measured the walk at 0.5–1.4 us against a 25–260 us parse, so about 0.5% — small. Worth knowing that
compileToExpressionis also called per segment on the server forJSON_MATCHand theta-sketch filters, so the multiplier there is segments x queries.sql.indexOf("::") >= 0 || StringUtils.containsIgnoreCase(sql, "BYTEA")has no false negatives and costs tens of nanoseconds. - One line in the description is misleading. The table lists
'0102'::byteaas rejected and suggestsX'0102'orCAST('0102' AS BYTES). In PostgreSQL'0102'::byteais valid escape-format input and gives four bytes (0x30 0x31 0x30 0x32); both substitutes give two. Better to mark it unsupported with no substitute, the way'\046'::byteaalready is one row below. - Worth a maintainer call: is
::meant to stay BYTEA-only? You note in the description thatintCol::doublewould in fact plan and run. Accepting the token and then rejecting every other target type is a permanent public-grammar decision, so it is worth one committer confirming the limit rather than settling it inside this PR. If the limit stays, an issue link in the class Javadoc would help the next reader.
| try (StringReader inStream = new StringReader(sql)) { | ||
| SqlParserImpl sqlParser = newSqlParser(inStream); | ||
| SqlNodeList sqlNodeList = sqlParser.parseSqlStmtList(); | ||
| sqlNodeList = (SqlNodeList) PostgreSqlCastRewriter.rewrite(sqlNodeList); |
There was a problem hiding this 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:
- Classify the statement first, then rewrite only the query node. For
SqlExplain, rewrite operand 0 in place withsetOperand. - Give each custom node's operator a
createCalloverride that rebuilds the concrete class, and giveSqlPhysicalExplainits ownOPERATOR.
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.
PR flow
Parses PostgreSQL :: bytea casts, normalizes to binary literals, and flows them to broker, expression, planner, and integration paths.
AI-generated · Green: added · Yellow: modified · Red: removed · Gray: existing
Diff evidence
Summary
Accept PostgreSQL hex-format
byteaconstants in both the infix'\x0102'::byteaform and the standardCAST('\x0102' AS BYTEA)spelling, and normalize them to Pinot's existing binary-literal representation before either the single-stage or multi-stage engine plans the query.The SQL-standard
X'0102'form is unchanged, and the two are fully interchangeable — including mixed within a single expression. The rewriter converts PostgreSQL constants to Calcite binary literals at parse time, so downstream there is no separate "PostgreSQL path": every spelling below produces the identical literal. Servers never see the new syntax, so there is no mixed-version exposure.Supported syntax
All of these produce the identical
BYTESliteral0x0102:Constant formatting:
Arrays, including mixed spellings in one array:
Usable anywhere a binary literal is valid — predicates,
INlists,CASEbranches,GROUP BY, and function arguments:Result values remain hex strings, and an array result reports
BYTES_ARRAYmetadata.Not supported
CAST(col AS BYTEA)CAST(col AS BYTES)col::byteaCAST(col AS BYTES)1::int::casts are supported only for BYTEA hex constantsCAST(1 AS INT)'0102'::bytea\xX'0102'orCAST('0102' AS BYTES)'\046'::bytea\x(legacy octal escape format)Two boundaries are deliberately asymmetric:
BYTEAaccepts only quoted constants, while the existingBYTEStype still accepts a dynamic per-row cast. A per-rowSTRING-to-BYTESconversion underBYTEAwould have to be implemented separately in each engine and the two would be easy to drift apart, so it is not introduced here.::is BYTEA-only. This is a scope limit, not a technical one:SqlLibraryOperators.INFIX_CASTis an ordinarySqlCastOperatorof kindCAST, andintCol::doubledoes compile to the samecastfunction asCAST(intCol AS DOUBLE). Supporting the full operator means committing to::type-name semantics across both engines, which belongs in its own PR. The grammar accepts::so this rewriter can report a clear error rather than a bare parse failure.Only ASCII hex digits and ASCII whitespace are accepted.
Character.digitandCharacter.isWhitespacealso accept full-width and non-Latin forms, which PostgreSQL rejects.Pre-existing conversions are unaffected:
CAST('0102' AS BYTES),CAST(strCol AS BYTES), andhexToBytes(...)all behave as before.Parsing notes
InfixCastappends the operator and its target type to the enclosing expression list as aSqlParserUtil.ToTreeListItem, so precedence is resolved byExpression2— the same shape as the adjacentSqlAtTimeZoneproduction and Calcite's own BabelInfixCast. This matters: collapsing the accumulated list eagerly makes::bind everything to its left, soWHERE bytesCol = '\x01'::byteaparses asCAST(bytesCol = '\x01' AS BYTEA)and fails to compile. There is direct coverage for bytea constants as the right operand of=,<>,>,<=and inside compound predicates.The
[]and.identpostfix forms need no handling in this production;Expression2already owns those branches.No Thrift or protobuf schema changes.
Validation
Local runs on this commit:
CalciteSqlParserTest,LiteralSerDeTest,RequestUtilsTest— 76 passedLiteralOnlyBrokerRequestTest— 10 passed (includes scalar bytea constants and constant folding on the broker's literal-only path)QueryCompilationTest— 241 passedResourceBasedQueriesTest(includesBinaryTypes.jsonagainst H2, both optimizer variants) — 3691 passedBytesTypeTest,BytesMvTypeTest(real Avroarray<bytes>ingestion, both query engines) — 32 passedspotless:apply,checkstyle:check,license:format,license:checkclean on all five affected modules; no compiler warnings on added linestestPostgreSqlByteaLiteralMatchesStandardCastSpellingpins the::,CASTandX'...'spellings to the same value across both the query and expression parse paths.