DanielLeens commented on PR #10221:
URL: https://github.com/apache/seatunnel/pull/10221#issuecomment-6411584576
Thanks @SEZ9 for the careful follow-up at
`c74d0578b0727ba6cda278710ce6d0d67d3c6aeb`, and thanks to everyone keeping this
small alias change moving. Your F2 request turned out to expose a real problem
that my last review got wrong, so I want to correct it explicitly. This is from
code reading only; nothing was executed locally.
**Correction on F2 (runtime `CAST(x AS VARBINARY)` fails today) - I now
agree this blocks the PR.**
In my last review I wrote that the SQL `CAST` branch is covered because "the
existing conversion machinery handles the physical bytes type". That was an
inference I did not trace to the value-conversion code, and it is wrong.
`CastFunction.getCastType` is only the type-resolution half; the row value is
converted by `SystemFunction.castAs(List<Object>)`, reached from
`ZetaSQLFunction.executeCastExpr`, which uppercases the target name and
switches on it:
```text
SELECT CAST(col AS VARBINARY)
-> ZetaSQLType.getExpressionType -> CastFunction.getCastType
(CastFunction.java:135-139) VARBINARY -> PrimitiveByteArrayType [this PR
adds it]
-> ZetaSQLFunction.executeCastExpr (ZetaSQLFunction.java:736-746) args =
[value, "VARBINARY"]
-> SystemFunction.castAs(args) switch (v2)
(SystemFunction.java:188)
case "BYTES": case "BINARY": return v1.toString().getBytes(UTF_8)
(SystemFunction.java:222-224) <- no "VARBINARY" label
fall through to
(SystemFunction.java:360-362)
throw TransformException("Unsupported CAST AS type: VARBINARY")
```
So after this PR the planner accepts `CAST(x AS VARBINARY)` and declares the
output column as BYTES, and then every row throws `Unsupported CAST AS type:
VARBINARY` at execution time. That is arguably worse than the previous behavior
(a clear failure when the statement is prepared). The existing
`CastFunctionTypeTest.java:117` assertion cannot see this because it only
checks the resolved type; the existing
`SystemFunctionTest.testCastAsPrimitiveAndBinaryTypes` (around line 412-445)
covers `BYTES` only and never reaches `VARBINARY`.
Suggested fix (small, in the same style as the existing labels):
- `SystemFunction.java:222-224`: add `case "VARBINARY":` next to `case
"BYTES": case "BINARY":`.
- Add the executing test @SEZ9 asked for, by extending
`SystemFunctionTest.testCastAsPrimitiveAndBinaryTypes` with the same shape as
the BYTES block: args `["hello", "VARBINARY"]`, assert `bytesResult instanceof
byte[]` and `assertArrayEquals("hello".getBytes(UTF_8), ...)`. Without the new
`case` label this assertion fails with the exception above, so it genuinely
guards the regression. An end-to-end `SELECT CAST(... AS VARBINARY)` through
`ZetaSQLEngine` in an existing SQL test would be an even closer match, if you
prefer.
This is a carryover from my earlier review (I should have caught it then),
not something introduced by the latest dev sync.
**F1 - enum Javadoc: +1 to @SEZ9.** `SqlType.java:46` appends `VARBINARY`
after `MULTIPLE_ROW`. No `SeaTunnelDataType` reports it (the parser returns
`PrimitiveByteArrayType.INSTANCE`, i.e. `SqlType.BYTES`), so a short Javadoc
stating "input-side alias only" will stop someone adding a dead `case
VARBINARY` in a physical-type switch later. Note this is different from the
`SystemFunction` label above: that switch is on the SQL type *name text*, which
is why it needs the label.
**F3 - SQL `CAST` docs: +1 to @SEZ9.**
`docs/en/transforms/sql-functions.md:1234` lists "BYTES | BINARY"; please make
it "BYTES | BINARY | VARBINARY" there and in the matching line of
`docs/zh/transforms/sql-functions.md`. The jsonpath doc already has it.
**F4 - whitespace hunk: +1 to @SEZ9.** The only change in
`JsonToRowConverters.java` is an added blank line at line 196 (before `case
DECIMAL:`); it can be dropped to keep the diff focused.
My earlier Issue 3 (extend the existing `TestJsonPathTransformIT` /
`json_path_basic_type_test.conf` with a VARBINARY destination) still stands as
well.
CI note for this head: the apache Build check is `action_required`
("Workflow run detection failed",
https://github.com/apache/seatunnel/runs/121083787689), so there is still no
exact-head CI. After the push, please make sure the fork's Actions are enabled
so the Build workflow runs.
Happy to re-review as soon as a new commit is pushed. Thanks again for the
patience, @xiaochen-zhou and @davidzollo!
--
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]
