Add jsonExtractScalar scalar function for JSON_EXTRACT_SCALAR - #18979
Add jsonExtractScalar scalar function for JSON_EXTRACT_SCALAR#18979Vamsi-klu wants to merge 1 commit into
Conversation
|
Ready for review. This replaces #16910 and rebases it onto current master. I also went back through the points raised in that PR's review: it now handles all the same result types as the transform function, throws in the same situations when a path can't be resolved, and I added a lot more coverage in JsonFunctionsTest. @Jackie-Jiang you reviewed the original #16910, and @xiangfu0 you've been active in JsonFunctions lately, so a look from either of you would be really helpful whenever you get a chance. Happy to tweak anything. Drafted-by: Claude Code (Opus 4.8); reviewed by @Vamsi-klu before posting |
There was a problem hiding this comment.
Pull request overview
This PR adds a scalar-function implementation of jsonExtractScalar/json_extract_scalar in pinot-common so the function can be resolved and executed in scalar contexts (not just as a transform), notably enabling multi-stage (v2) engine intermediate stages to use JSON scalar extraction.
Changes:
- Added
@ScalarFunction-registeredjsonExtractScalartopinot-common’sJsonFunctions, mirroring the transform function’s coercion and error/default semantics across supported result types (including array variants). - Expanded
JsonFunctionsTestto cover coercions, defaults, unresolved-path behavior, malformed JSON handling, array/null-element behavior, and unsupported types. - Removed the now-obsolete multi-stage negative test that asserted
json_extract_scalarwas unsupported.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/QueryRunnerTest.java |
Removes the prior “unsupported function” assertion for json_extract_scalar in multi-stage contexts. |
pinot-common/src/test/java/org/apache/pinot/common/function/JsonFunctionsTest.java |
Adds extensive unit coverage for the new scalar function’s coercion and error/default behavior. |
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java |
Registers and implements the jsonExtractScalar scalar function (and default overload) with transform-parity semantics. |
Suppressed comments (1)
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java:490
- readJsonPathArray() has the same BYTES-input gap as readJsonPathValue(): JsonExtractScalarTransformFunction uses ParseContext#parseUtf8 for BYTES JSON columns, but the scalar version currently calls parseContext.parse(jsonInput) for byte[], which can fail to extract arrays from BYTES JSON inputs.
ParseContext parseContext = useBigDecimal ? PARSE_CONTEXT_WITH_BIG_DECIMAL : PARSE_CONTEXT;
Object read = jsonInput instanceof String ? parseContext.parse((String) jsonInput).read(jsonPath, NO_PREDICATES)
: parseContext.parse(jsonInput).read(jsonPath, NO_PREDICATES);
return convertObjectToArray(read);
| ParseContext parseContext = useBigDecimal ? PARSE_CONTEXT_WITH_BIG_DECIMAL : PARSE_CONTEXT; | ||
| if (jsonInput instanceof String) { | ||
| return parseContext.parse((String) jsonInput).read(jsonPath, NO_PREDICATES); | ||
| } | ||
| return parseContext.parse(jsonInput).read(jsonPath, NO_PREDICATES); |
There was a problem hiding this comment.
Good catch, thanks. The transform parses BYTES columns via ParseContext.parseUtf8 (JsonExtractScalarTransformFunction line 804), while my helpers only special-case String, so a byte[] falls into parse(Object), which Jayway treats as an already parsed document instead of raw JSON. Because of the catch block, that would silently resolve to the default or throw instead of extracting. I will add an instanceof byte[] branch in both readJsonPathValue and readJsonPathArray that routes through parseContext.parseUtf8, and push it with the next update.
| @Test | ||
| public void testJsonExtractScalarBytes() { | ||
| assertEquals((byte[]) JsonFunctions.jsonExtractScalar("{\"h\":\"0a0b\"}", "$.h", "BYTES"), | ||
| new byte[]{0x0a, 0x0b}); | ||
| } |
There was a problem hiding this comment.
Agreed. The current byte[] input path is broken (it falls through to parse(Object)), so this gap is real. Along with the parseUtf8 fix in readJsonPathValue and readJsonPathArray, I will add tests that feed the same sample JSON as UTF-8 bytes (json.getBytes(StandardCharsets.UTF_8)) and assert parity with the String-input results, covering single-value extraction, array extraction, and the default-value path.
| // - PlaceholderScalarFunction registered will throw on intermediate stage, but works on leaf stage. | ||
| // - checked "Illegal Json Path" as col1 is not actually a json string, but the call is correctly triggered. | ||
| testCases.add( | ||
| new Object[]{"SELECT CAST(jsonExtractScalar(col1, 'path', 'INT') AS INT) FROM a", "Cannot resolve JSON path"}); | ||
| // - checked function cannot be found b/c there's no intermediate stage impl for json_extract_scalar | ||
| testCases.add(new Object[]{ | ||
| "SELECT CAST(json_extract_scalar(a.col1, b.col2, 'INT') AS INT) FROM a JOIN b ON a.col1 = b.col1", | ||
| "Unsupported function: JSONEXTRACTSCALAR" | ||
| }); | ||
|
|
There was a problem hiding this comment.
Fair point. The removed case only asserted the old failure mode, and the remaining case at line 333 exercises the leaf stage. I will add a positive case to the row-count data provider that forces intermediate-stage evaluation across a JOIN, for example SELECT json_extract_scalar(a.col1, '$.x', 'INT', 0) FROM a JOIN b ON a.col1 = b.col1 (with a default value since col1 test data is not JSON), asserting the expected row count. That verifies the function resolves and runs in the v2 intermediate stage and guards against registration regressions.
json_extract_scalar was only implemented as a transform function, so it could not be resolved in the multi-stage query engine or in ad-hoc scalar contexts. Register it as a scalar function with full result-type parity to the transform: INT/LONG/FLOAT/DOUBLE/BIG_DECIMAL/BOOLEAN/TIMESTAMP/STRING/JSON/BYTES single values plus INT/LONG/FLOAT/DOUBLE/BIG_DECIMAL/STRING arrays, reusing the transform's coercion rules (BooleanUtils numeric convention, TimestampUtils for ISO-8601 strings, a BigDecimal-preserving parser) and its throw-on-unresolved semantics. A BigDecimal-based long parser stands in for pinot-core's NumberUtils.parseJsonLong, which pinot-common cannot depend on. Supersedes apache#16910. Co-authored-by: Manik Somayaji <somayajimanik@gmail.com>
3f38d79 to
cd28a76
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18979 +/- ##
=============================================
- Coverage 66.59% 38.90% -27.70%
+ Complexity 1423 1422 -1
=============================================
Files 3443 3443
Lines 218536 218673 +137
Branches 34780 34813 +33
=============================================
- Hits 145538 85077 -60461
- Misses 61273 125841 +64568
+ Partials 11725 7755 -3970
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:
|
What's the problem
json_extract_scalarexists only as a transform function(
JsonExtractScalarTransformFunction). It was never registered as ascalar function, so it cannot be resolved wherever the query planner needs
a scalar implementation — most visibly the multi-stage (v2) engine's
intermediate stages. A query such as
SELECT CAST(json_extract_scalar(a.col1, b.col2, 'INT') AS INT) FROM a JOIN b ...fails outright with
Unsupported function: JSONEXTRACTSCALAR.Why it matters
The multi-stage engine is Pinot's path for JOINs, sub-queries, and other
intermediate-stage computation. Without a scalar form, one of the most common
JSON-extraction operations is unusable across that whole class of queries, and
in ad-hoc scalar/constant-folding contexts. Issue #16486 tracks exactly this
gap.
What I did
Registered
jsonExtractScalaras a@ScalarFunctioninpinot-common'sJsonFunctions, with full parity to the transform:INT,LONG,FLOAT,DOUBLE,BIG_DECIMAL,BOOLEAN,TIMESTAMP,STRING,JSON,BYTES; arrays forINT/LONG/FLOAT/DOUBLE/BIG_DECIMAL/STRING.BOOLEANuses Pinot's numericconvention and returns stored
INT(0/1);TIMESTAMPtakes numeric epochmillis directly and ISO-8601 strings via
TimestampUtils;BIG_DECIMAL/STRING/JSONread through a BigDecimal-preserving parser.default throws; a multi-value path yields an empty array; a null element in a
resolved array throws unless a default is supplied; malformed JSON is treated
as unresolved.
was unsupported, and extended
JsonFunctionsTestacross every result type(happy / default / throw paths, arrays incl. null elements, malformed JSON,
BigDecimal precision, unsupported type).
Why I did it this way
transform forms are behaviorally identical — no second, subtly-different JSON
semantics for users to reason about.
JsonFunctions: the existingjsonPath*methods(including
master'scanExtractJsonPath()ingestion-hot-path fast-path) areuntouched, so this carries zero regression risk for current callers.
pinot-core'sNumberUtils.parseJsonLong, becausepinot-commoncannot depend onpinot-core; it reproduces the same truncate-toward-zero and exponenthandling for JSON numeric strings.
Impact
json_extract_scalarnow resolves in the multi-stage engine and scalarcontexts, with behavior identical to the transform. No existing function
changes behavior. Supersedes #16910 (revived, rebased on current
master, withthat PR's review gaps on parity, throw semantics, and coverage closed).
closes #16486
Co-authored-by: Manik Somayaji somayajimanik@gmail.com
Generated-by: Claude Code (Opus 4.8)