ggjh-159 commented on PR #12666:
URL: https://github.com/apache/gluten/pull/12666#issuecomment-6076772170
> ## Review comments
> Thanks for the PR — the direction looks good as a planner mapping start
for #12665. A few concerns before merge:
>
> ### 1. `JSON_VALUE` → `get_json_object` is not semantically equivalent
(high)
> Flink `JSON_VALUE` and Velox/Spark `get_json_object` differ in important
ways:
>
> Aspect Flink `JSON_VALUE` `get_json_object`
> Return type Scalar (optional `RETURNING`) Essentially STRING
> Object/array path Usually NULL (non-scalar) Often returns JSON text
> `ON EMPTY` / `ON ERROR` Supported Not supported
> Missing path NULL by default NULL
> `DefaultRexCallConverter("get_json_object")` only remaps the name.
Non-trivial `JSON_VALUE` usage can silently diverge.
>
> Please either:
>
> * Document that only the common subset `JSON_VALUE(json, '$.a.b')`
extracting a **string scalar** is supported, and/or
> * Explicitly reject unsupported forms (`RETURNING`, `ON ERROR`, non-scalar
paths) in the converter, and/or
> * Prefer a closer Velox scalar JSON function if one is available.
>
> Current UT only covers `$.name`, which does not catch the gaps above.
>
> ### 2. `TIMESTAMP` literal handling looks OK; please tighten tests / docs
(medium)
> ```java
> long epochMillis = literal.getValueAs(Long.class);
> TimestampValue.create(
> Math.floorDiv(epochMillis, 1000L),
> Math.floorMod(epochMillis, 1000L) * 1_000_000L);
> ```
>
> This is a reasonable workaround for `getValueAs(Timestamp.class)` and is
fine for `TIMESTAMP(3)`. Please:
>
> * Keep the note that this must stay aligned with
`TimestampData#getMillisecond` / row serializer (no TZ drift).
> * Add a more direct UT (e.g. project the literal alone, or compare with a
column), instead of only going through `COALESCE` + `DATE_FORMAT` (timezone /
formatting sensitive).
>
> ### 3. Jackson owner-classpath change is loosely coupled (medium)
> Resolving `com.fasterxml.jackson` from the owner classpath to fix factory
loading is understandable, but:
>
> * It is unrelated to scalar-expr mapping; prefer a **separate commit/PR**.
> * Owner-first resolution can cause **Jackson version conflicts** with
component jars — please call out the impact in the PR description.
>
> ### 4. Test coverage is thin (medium)
> Existing tests cover happy paths only. Suggest adding at least:
>
> * `SUBSTRING`: 2-arg form, OOB, `NULL`
> * `COALESCE`: mixed numeric/temporal types, all-NULL
> * `JSON_VALUE`: missing key, numeric `$.age`, invalid JSON / unsupported
syntax (should fail if we choose strict mode)
>
> ### 5. `SUBSTRING` / `COALESCE` mapping (low)
> Basic 1-based `SUBSTRING` / `COALESCE` mapping is likely fine. Worth a
short comment or UT for negative start index and SQL `FROM ... FOR ...` form if
they share the same operator name.
>
> **Summary:** Good starting point, but please do not claim full Flink
`JSON_VALUE` support without documenting/enforcing the subset; clarify
TIMESTAMP UTC assumptions with a direct test; consider splitting the Jackson
classpath change.
Thanks @KevinyhZou for the thorough review! All five points are addressed in
the updated head (the branch was rebuilt as a single commit on top of main; the
jackson change is split out):
1. **JSON_VALUE**: Agreed — I removed the `JSON_VALUE -> get_json_object`
mapping (and its UT) from this PR. A name-only remap can silently diverge as
you described, so `JSON_VALUE` now fails fast with `Function not supported:
JSON_VALUE`. Proper Flink-semantics support (documented subset / strict
rejection of `RETURNING`, `ON EMPTY`, `ON ERROR` forms) will be tracked in a
separate issue and delivered in a follow-up PR.
2. **TIMESTAMP literal**: The comment on staying aligned with
`TimestampData#getMillisecond` (UTC wall time, same base as the row serializer)
is kept, and I added a direct projection UT (`select TIMESTAMP '2024-03-15
10:15:30'`) that anchors the converted epoch value without going through
`COALESCE` + `DATE_FORMAT`.
3. **Jackson change**: Split out of this PR — the classloader fix is
independent of the expression mappings and will be submitted separately.
4. **Test coverage**: `testSubstring` now covers the 2-arg form,
out-of-bounds start (empty string), zero length, and NULL input; `testCoalesce`
covers mixed numeric types (int/bigint -> BIGINT) and the all-NULL row. The
`JSON_VALUE` cases are moot now that the mapping is removed.
5. **SUBSTRING**: Flink treats a start of 0 as 1 and counts a negative start
from the string end — the same rules velox substring implements — so starts
need no restriction (verified against a vanilla Flink 1.19.2 cluster: starts 0
/ -3 / -7 give identical results). The one clear divergence is a negative
length: Flink returns NULL while velox returns an empty string, so the planner
rejects a literal negative length with a clear error; a non-literal length
can't be checked at plan time. A negative start beyond the string length also
diverges (Flink returns an empty string, velox returns from the head), but that
depends on each row's length and can't be checked at plan time either. Both
gaps close with a Flink-semantics substring in the velox flinksql function set;
I'll file a separate issue to track it and lift the rejection once it lands.
UTs cover the matching starts, a dynamic start, and the rejection.
--
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]
