xiangfu0 commented on code in PR #19012:
URL: https://github.com/apache/pinot/pull/19012#discussion_r3651410113
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/queries/QueryRunnerTest.java:
##########
@@ -333,7 +333,8 @@ protected Iterator<Object[]>
provideTestSqlWithExecutionException() {
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",
+ "SELECT CAST(json_extract_scalar(CONCAT(a.col1, b.col2), 'path',
'INT') AS INT) FROM a JOIN b "
Review Comment:
Good catch — this shouldn't have changed, and I've reverted it to the
original `json_extract_scalar(a.col1, b.col2, 'INT')`.
The rewrite was a symptom of a real bug: the new shared operand type checker
required `jsonPath` to be a literal `SqlNode`. Operand checking runs *before*
`PinotEvaluateLiteralRule` folds constants, so it rejected this query (`b.col2`
isn't a literal) at validation — but it also rejected constant-foldable paths
like `jsonExtractScalar(col, CONCAT('$.', 'foo'), 'INT')` that fold to a
literal and execute fine on master. That's a backward-incompatible narrowing of
the existing `jsonExtractScalar`.
Fixed by relaxing the `jsonPath` operand to accept any `CHARACTER`
expression. I kept the literal requirement on `resultsType` (return-type
inference reads it during validation, before folding — a foldable value there
would silently infer `VARCHAR` while the leaf stage extracts the real type) and
on `defaultValue`. A genuinely non-literal `jsonPath` is still rejected on the
leaf stage by `ParserUtils#validateFunction`.
This test case is back to its original form, and I added regression coverage
in `QueryCompilationTest#testJsonExtractScalarAcceptsFoldableJsonPath` plus
runtime cases in `provideTestSqlWithExecutionException` for both foldable and
genuinely non-literal paths across all three variants.
--
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]
