KevinyhZou commented on PR #12666:
URL: https://github.com/apache/gluten/pull/12666#issuecomment-5788326767
## 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.
--
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]
