xiangfu0 commented on PR #19263:
URL: https://github.com/apache/pinot/pull/19263#issuecomment-5768812942
@yashmayya thanks for the thorough review. I replied to the blocker inline;
the fix is in 6e2b49bf8c, which sits on a fresh rebase onto master. Here are
the non-blocking notes.
1. **`col::bytea` remedy.** Done. The message now ends with *"To convert hex
strings per row, use hexToBytes(<expr>), which takes plain hexadecimal digits
without the \x prefix"*, and a test pins it.
2. **`compileToExpression` hiding messages.** Done. The rewrite now runs
after the `try`, so its `SqlCompilationException` reaches the caller unwrapped.
The tests assert on `getMessage()` instead of the stack trace, and
`testPostgreSqlByteaErrorsSurfaceFromCompileToExpression` covers that path
directly.
3. **Untested branches.** Done. `X'0102'::bytea` and `CAST(X'0102' AS
BYTEA)` are now in the spelling-equivalence test, which checks both the query
and expression paths, and `'\X0102'::bytea` is in
`invalidPostgreSqlByteaLiterals`.
4. **Text-scan guard.** I'd like to leave this out unless you feel strongly.
By your measurement the walk is about 0.5% of parse. A text gate would be a
second, string-level definition of what the rewriter handles, and it would have
to stay in sync with the tree logic by hand. For example, the lowercase
`cast('\x01' as bytea)` form would depend entirely on the case-insensitive half
of the check. I'd rather keep a single source of truth. Happy to add it if you
disagree.
5. **`'0102'::bytea` in the description.** Good catch. PostgreSQL reads it
as escape format and gives four bytes, so both substitutes were wrong. The row
is now marked unsupported with no substitute, like `'\046'`. I also changed the
error message to say *"only the hex format is supported"* rather than implying
the input was just malformed.
6. **Keeping `::` BYTEA-only.** This PR keeps that scope. It has been the
stated scope since the first revision, and it is the reversible direction:
relaxing a rejection later is backward compatible, while narrowing a type the
grammar already accepts would not be. I filed #19628 for the general operator
and linked it from the class Javadoc. It lists the open questions, the biggest
being type names: PostgreSQL users will write `int4`, `float8` and `text`, and
Calcite's `DataType()` doesn't know those. If you or another committer would
rather allow more target types now, I'm happy to discuss it there.
An independent review of the fix turned up one more issue, which is also
fixed. `InfixCast` recorded `s.pos()`, which starts at the token before the
enclosing expression, so the rewritten literal claimed spans like `WHERE b =
'\x01'::bytea` and validator errors highlighted the wrong range. It now records
the `::` token's own position, as `BinaryRowOperator` does.
`testPostgreSqlByteaLiteralKeepsItsSourcePosition` pins it: column 27, where
the old grammar gave 17. This matters beyond error messages, because
`DdlCompiler.extractDefinedSql` slices the stored view SQL from the union of
every node's position.
--
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]
