Jackie-Jiang commented on code in PR #18979:
URL: https://github.com/apache/pinot/pull/18979#discussion_r4000978846
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java:
##########
@@ -611,6 +634,396 @@ public static Object jsonExtractObject(@Nullable Object
object) {
return null;
}
+ /// Extract a scalar (or scalar-array) value from a JSON document and coerce
it to `resultsType`.
+ ///
+ /// Scalar-function counterpart of the `jsonExtractScalar` transform
(`JsonExtractScalarTransformFunction` in
+ /// pinot-core), so that `json_extract_scalar(...)` resolves in the
multi-stage engine and in ad-hoc scalar
+ /// contexts. `resultsType` is a Pinot [DataType] name, optionally suffixed
with `_ARRAY` for a multi-value
+ /// result. Supported types are
`INT/LONG/FLOAT/DOUBLE/BIG_DECIMAL/BOOLEAN/TIMESTAMP/STRING/JSON/BYTES` and
+ /// the `INT/LONG/FLOAT/DOUBLE/BIG_DECIMAL/BOOLEAN/TIMESTAMP/STRING` array
variants.
+ ///
+ /// The document may be a `String`, a UTF-8 encoded `byte[]` (BYTES columns)
or an already-parsed container.
+ /// Coercion mirrors the transform exactly: `BOOLEAN` is returned as its
stored `INT` (0/1), `TIMESTAMP` as
+ /// epoch millis (numeric values as-is, strings via ISO-8601), `BIG_DECIMAL`
/ `STRING` / `JSON` use a
+ /// BigDecimal-preserving parser. The 3-argument form throws on an
unresolved single-value path. The
+ /// 4-argument form returns `defaultValue` (including SQL `NULL`). A
multi-value path yields an empty
+ /// array when unresolved, but a `null` element inside a resolved array
still throws unless a default is
+ /// supplied. A malformed JSON document is treated as unresolved.
+ @ScalarFunction
Review Comment:
[P2] Handle typed results in constant folding
This deterministic registration makes all-literal calls eligible for
`PinotEvaluateLiteralRule`, but its result conversion cannot handle several
values returned here. For example, `SELECT jsonExtractScalar('{"v":[1,2]}',
'$.v', 'INT_ARRAY') FROM myTable` returns `int[]` to the folder, which
special-cases only `double[]` and casts other arrays to `Object[]`, causing SQL
compilation to fail. LONG_ARRAY, FLOAT_ARRAY, BOOLEAN_ARRAY and TIMESTAMP_ARRAY
have the same problem. Likewise, `SELECT jsonExtractScalar('{"v":true}', '$.v',
'BOOLEAN') FROM myTable` returns an Integer that reaches Calcite's Boolean
literal construction without conversion.
Please extend literal conversion or provide compatible results at that
boundary, and add SQL constant-folding coverage for both overloads and the
affected types. The added column-input tests do not exercise this path.
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java:
##########
@@ -611,6 +634,396 @@ public static Object jsonExtractObject(@Nullable Object
object) {
return null;
}
+ /// Extract a scalar (or scalar-array) value from a JSON document and coerce
it to `resultsType`.
+ ///
+ /// Scalar-function counterpart of the `jsonExtractScalar` transform
(`JsonExtractScalarTransformFunction` in
+ /// pinot-core), so that `json_extract_scalar(...)` resolves in the
multi-stage engine and in ad-hoc scalar
+ /// contexts. `resultsType` is a Pinot [DataType] name, optionally suffixed
with `_ARRAY` for a multi-value
+ /// result. Supported types are
`INT/LONG/FLOAT/DOUBLE/BIG_DECIMAL/BOOLEAN/TIMESTAMP/STRING/JSON/BYTES` and
+ /// the `INT/LONG/FLOAT/DOUBLE/BIG_DECIMAL/BOOLEAN/TIMESTAMP/STRING` array
variants.
+ ///
+ /// The document may be a `String`, a UTF-8 encoded `byte[]` (BYTES columns)
or an already-parsed container.
+ /// Coercion mirrors the transform exactly: `BOOLEAN` is returned as its
stored `INT` (0/1), `TIMESTAMP` as
+ /// epoch millis (numeric values as-is, strings via ISO-8601), `BIG_DECIMAL`
/ `STRING` / `JSON` use a
+ /// BigDecimal-preserving parser. The 3-argument form throws on an
unresolved single-value path. The
+ /// 4-argument form returns `defaultValue` (including SQL `NULL`). A
multi-value path yields an empty
+ /// array when unresolved, but a `null` element inside a resolved array
still throws unless a default is
+ /// supplied. A malformed JSON document is treated as unresolved.
+ @ScalarFunction
+ public static Object jsonExtractScalar(Object jsonInput, String jsonPath,
String resultsType) {
+ return jsonExtractScalarInternal(jsonInput, jsonPath, resultsType, null,
false);
+ }
+
+ /// See [#jsonExtractScalar(Object, String, String)]. `defaultValue` is
returned (coerced to `resultsType`)
+ /// when the path resolves to `null` or the document is malformed. An
explicit SQL `NULL` default returns
+ /// Java `null` rather than throwing.
+ @ScalarFunction(nullableParameters = true)
+ public static Object jsonExtractScalar(@Nullable Object jsonInput, String
jsonPath, String resultsType,
+ @Nullable Object defaultValue) {
+ return jsonExtractScalarInternal(jsonInput, jsonPath, resultsType,
defaultValue, true);
+ }
+
+ @Nullable
+ private static Object jsonExtractScalarInternal(@Nullable Object jsonInput,
String jsonPath, String resultsType,
+ @Nullable Object defaultValue, boolean hasDefault) {
+ String type = resultsType.toUpperCase();
+ boolean isSingleValue = !type.endsWith("_ARRAY");
+ DataType dataType;
+ try {
+ dataType = DataType.valueOf(isSingleValue ? type : type.substring(0,
type.length() - 6));
+ } catch (IllegalArgumentException e) {
+ throw new
IllegalArgumentException(unsupportedResultsTypeMessage(resultsType));
+ }
+ // BIG_DECIMAL / STRING / JSON must read floats as BigDecimal to preserve
precision, matching the transform.
+ boolean useBigDecimal =
+ dataType == DataType.BIG_DECIMAL || dataType == DataType.STRING ||
dataType == DataType.JSON;
+ if (isSingleValue) {
+ Object value = readJsonPathValue(jsonInput, jsonPath, useBigDecimal);
+ if (value == null) {
+ if (!hasDefault) {
+ throw new IllegalArgumentException(
+ "Cannot resolve JSON path on some records. Consider setting a
default value.");
+ }
+ if (defaultValue == null) {
+ return null;
+ }
+ return coerceScalar(defaultValue, dataType, true);
+ }
+ return coerceScalar(value, dataType, false);
+ }
+ return coerceScalarArray(readJsonPathArray(jsonInput, jsonPath,
useBigDecimal), dataType, defaultValue, hasDefault);
+ }
+
+ /// Reads `jsonPath` from a JSON `String`, UTF-8 `byte[]`, or already-parsed
document.
+ /// A missing path returns `null` (`Option.SUPPRESS_EXCEPTIONS`). Malformed
input throws.
+ /// Callers that already know the input type (the transform hot path) should
call
+ /// `parseUtf8` / `parse` themselves instead of going through this dispatch.
+ @Nullable
+ private static <T> T readJsonPathInternal(Object jsonInput, String jsonPath,
ParseContext parseContext) {
+ return parseJsonDocument(jsonInput, parseContext).read(jsonPath,
NO_PREDICATES);
+ }
+
+ private static DocumentContext parseJsonDocument(Object jsonInput,
ParseContext parseContext) {
+ if (jsonInput instanceof String) {
+ return parseContext.parse((String) jsonInput);
+ }
+ if (jsonInput instanceof byte[]) {
+ // BYTES columns carry the raw UTF-8 document; parse(Object) would treat
the array as already parsed.
+ return parseContext.parseUtf8((byte[]) jsonInput);
+ }
+ return parseContext.parse(jsonInput);
+ }
+
+ @Nullable
+ private static Object readJsonPathValue(@Nullable Object jsonInput, String
jsonPath, boolean useBigDecimal) {
+ if (jsonInput == null) {
+ return null;
+ }
+ try {
+ return readJsonPathInternal(jsonInput, jsonPath,
+ useBigDecimal ? PARSE_CONTEXT_WITH_BIG_DECIMAL : PARSE_CONTEXT);
+ } catch (Exception e) {
+ // Malformed JSON (e.g. a plain-text row) is treated as unresolved,
mirroring the transform which swallows
+ // per-row extraction errors; the caller then applies the default or
throws.
+ return null;
+ }
+ }
+
+ @Nullable
+ private static Object[] readJsonPathArray(@Nullable Object jsonInput, String
jsonPath, boolean useBigDecimal) {
+ if (jsonInput == null) {
+ return null;
+ }
+ try {
+ return convertObjectToArray(readJsonPathInternal(jsonInput, jsonPath,
Review Comment:
[P2] Preserve transform behavior when an array path resolves to a scalar
`convertObjectToArray` wraps any non-array result in a singleton. For input
`{"v":42}`, extracting `$.v` as INT_ARRAY therefore returns `[42]` here. The
existing transform expects a List, catches the failed cast for this input, and
returns `[]`. Moving the same extraction from a leaf transform to the scalar
implementation changes the result and can affect array filters or counts.
Please align handling of resolved non-array values with the transform and
add parity tests for scalar and object values requested as array result types.
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java:
##########
@@ -611,6 +634,396 @@ public static Object jsonExtractObject(@Nullable Object
object) {
return null;
}
+ /// Extract a scalar (or scalar-array) value from a JSON document and coerce
it to `resultsType`.
+ ///
+ /// Scalar-function counterpart of the `jsonExtractScalar` transform
(`JsonExtractScalarTransformFunction` in
+ /// pinot-core), so that `json_extract_scalar(...)` resolves in the
multi-stage engine and in ad-hoc scalar
+ /// contexts. `resultsType` is a Pinot [DataType] name, optionally suffixed
with `_ARRAY` for a multi-value
+ /// result. Supported types are
`INT/LONG/FLOAT/DOUBLE/BIG_DECIMAL/BOOLEAN/TIMESTAMP/STRING/JSON/BYTES` and
+ /// the `INT/LONG/FLOAT/DOUBLE/BIG_DECIMAL/BOOLEAN/TIMESTAMP/STRING` array
variants.
+ ///
+ /// The document may be a `String`, a UTF-8 encoded `byte[]` (BYTES columns)
or an already-parsed container.
+ /// Coercion mirrors the transform exactly: `BOOLEAN` is returned as its
stored `INT` (0/1), `TIMESTAMP` as
+ /// epoch millis (numeric values as-is, strings via ISO-8601), `BIG_DECIMAL`
/ `STRING` / `JSON` use a
+ /// BigDecimal-preserving parser. The 3-argument form throws on an
unresolved single-value path. The
+ /// 4-argument form returns `defaultValue` (including SQL `NULL`). A
multi-value path yields an empty
+ /// array when unresolved, but a `null` element inside a resolved array
still throws unless a default is
+ /// supplied. A malformed JSON document is treated as unresolved.
+ @ScalarFunction
+ public static Object jsonExtractScalar(Object jsonInput, String jsonPath,
String resultsType) {
+ return jsonExtractScalarInternal(jsonInput, jsonPath, resultsType, null,
false);
+ }
+
+ /// See [#jsonExtractScalar(Object, String, String)]. `defaultValue` is
returned (coerced to `resultsType`)
+ /// when the path resolves to `null` or the document is malformed. An
explicit SQL `NULL` default returns
+ /// Java `null` rather than throwing.
+ @ScalarFunction(nullableParameters = true)
+ public static Object jsonExtractScalar(@Nullable Object jsonInput, String
jsonPath, String resultsType,
+ @Nullable Object defaultValue) {
+ return jsonExtractScalarInternal(jsonInput, jsonPath, resultsType,
defaultValue, true);
+ }
+
+ @Nullable
+ private static Object jsonExtractScalarInternal(@Nullable Object jsonInput,
String jsonPath, String resultsType,
+ @Nullable Object defaultValue, boolean hasDefault) {
+ String type = resultsType.toUpperCase();
+ boolean isSingleValue = !type.endsWith("_ARRAY");
+ DataType dataType;
+ try {
+ dataType = DataType.valueOf(isSingleValue ? type : type.substring(0,
type.length() - 6));
+ } catch (IllegalArgumentException e) {
+ throw new
IllegalArgumentException(unsupportedResultsTypeMessage(resultsType));
+ }
+ // BIG_DECIMAL / STRING / JSON must read floats as BigDecimal to preserve
precision, matching the transform.
+ boolean useBigDecimal =
+ dataType == DataType.BIG_DECIMAL || dataType == DataType.STRING ||
dataType == DataType.JSON;
+ if (isSingleValue) {
+ Object value = readJsonPathValue(jsonInput, jsonPath, useBigDecimal);
+ if (value == null) {
+ if (!hasDefault) {
+ throw new IllegalArgumentException(
+ "Cannot resolve JSON path on some records. Consider setting a
default value.");
+ }
+ if (defaultValue == null) {
+ return null;
+ }
+ return coerceScalar(defaultValue, dataType, true);
+ }
+ return coerceScalar(value, dataType, false);
+ }
+ return coerceScalarArray(readJsonPathArray(jsonInput, jsonPath,
useBigDecimal), dataType, defaultValue, hasDefault);
+ }
+
+ /// Reads `jsonPath` from a JSON `String`, UTF-8 `byte[]`, or already-parsed
document.
+ /// A missing path returns `null` (`Option.SUPPRESS_EXCEPTIONS`). Malformed
input throws.
+ /// Callers that already know the input type (the transform hot path) should
call
+ /// `parseUtf8` / `parse` themselves instead of going through this dispatch.
+ @Nullable
+ private static <T> T readJsonPathInternal(Object jsonInput, String jsonPath,
ParseContext parseContext) {
+ return parseJsonDocument(jsonInput, parseContext).read(jsonPath,
NO_PREDICATES);
+ }
+
+ private static DocumentContext parseJsonDocument(Object jsonInput,
ParseContext parseContext) {
+ if (jsonInput instanceof String) {
+ return parseContext.parse((String) jsonInput);
+ }
+ if (jsonInput instanceof byte[]) {
+ // BYTES columns carry the raw UTF-8 document; parse(Object) would treat
the array as already parsed.
+ return parseContext.parseUtf8((byte[]) jsonInput);
+ }
+ return parseContext.parse(jsonInput);
+ }
+
+ @Nullable
+ private static Object readJsonPathValue(@Nullable Object jsonInput, String
jsonPath, boolean useBigDecimal) {
+ if (jsonInput == null) {
+ return null;
+ }
+ try {
+ return readJsonPathInternal(jsonInput, jsonPath,
+ useBigDecimal ? PARSE_CONTEXT_WITH_BIG_DECIMAL : PARSE_CONTEXT);
+ } catch (Exception e) {
Review Comment:
[P2] Preserve invalid JSONPath errors
The catch still includes path compilation/evaluation, so valid JSON with an
invalid path such as `$[` returns the supplied default instead of reporting the
invalid expression. The array helper similarly returns an empty array. The
transform compiles its path during initialization and rejects this syntax, so
execution placement changes whether a malformed expression succeeds. This
earlier review concern remains unresolved.
Please separate document parsing from path evaluation and suppress only
malformed-document failures, allowing invalid path expressions to propagate.
Cover scalar and array calls with and without defaults.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/JsonNumberUtils.java:
##########
@@ -0,0 +1,217 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils;
+
+
+/// Shared JSON-number parser used by `jsonExtractScalar` (scalar and
transform).
+///
+/// Accepts regular long syntax plus JSON numeric forms: `1E1` → `10`, `1.9` →
`1` (truncate toward
+/// zero), `1.123E1` → `11`. Throws [NumberFormatException] with `For input
string: "<value>"` on
+/// overflow (`9223372036854775808`, `2.0E19`), illegal exponent (`2E20`,
`2E-1`), and other malformed input.
+///
+/// Thread-safe: no mutable state.
+public final class JsonNumberUtils {
+ private static final long[] POWERS_OF_10 = new long[]{
+ 1L,
+ 10L,
+ 100L,
+ 1000L,
+ 10000L,
+ 100000L,
+ 1000000L,
+ 10000000L,
+ 100000000L,
+ 1000000000L,
+ 10000000000L,
+ 100000000000L,
+ 1000000000000L,
+ 10000000000000L,
+ 100000000000000L,
+ 1000000000000000L,
+ 10000000000000000L,
+ 100000000000000000L,
+ 1000000000000000000L,
+ };
+
+ private JsonNumberUtils() {
+ }
+
+ /// Parses a JSON numeric string to a long.
+ ///
+ /// @param cs char sequence to parse
+ /// @return parsed long value
+ /// @throws NumberFormatException if `cs` is null, empty, out of long range,
or not a JSON number
+ public static long parseJsonLong(CharSequence cs) {
+ if (cs == null) {
+ throw new NumberFormatException("Can't parse null string");
+ }
+
+ boolean negative = false;
+ int i = 0;
+ int len = cs.length();
+ long limit = -Long.MAX_VALUE;
+
+ if (len <= 0) {
+ throw formatException(cs);
+ }
+
+ boolean dotFound = false;
+ boolean exponentFound = false;
+
+ char firstChar = cs.charAt(0);
+ if (firstChar < '0') { // Possible leading "+" or "-"
+ if (firstChar == '-') {
+ negative = true;
+ limit = Long.MIN_VALUE;
+ } else if (firstChar != '+') {
+ throw formatException(cs);
+ }
+
+ if (len == 1) { // Cannot have lone "+" or "-"
+ throw formatException(cs);
+ }
+ i++;
+ }
+ long multmin = limit / 10;
+ long result = 0;
+ while (i < len) {
+ // Accumulating negatively avoids surprises near MAX_VALUE
+ char c = cs.charAt(i++);
+ if (c < '0' || c > '9' || result < multmin) {
+ if (c == '.') {
+ // ignore the rest of the integer digits
+ dotFound = true;
+ break;
+ } else if (c == 'e' || c == 'E') {
+ exponentFound = true;
+ break;
+ }
+ throw formatException(cs);
+ }
+
+ int digit = c - '0';
+ result *= 10;
+ if (result < limit + digit) {
+ throw formatException(cs);
+ }
+ result -= digit;
+ }
+
+ if (dotFound) {
+ // scan rest of the string to make sure it's only digits (or an exponent)
+ while (i < len) {
+ char c = cs.charAt(i++);
+ if (c < '0' || c > '9') {
+ if ((c | 32) == 'e') {
+ exponentFound = true;
+ break;
+ } else {
+ throw formatException(cs);
+ }
+ }
+ }
+ }
+
+ if (exponentFound) {
+ if (dotFound) {
+ double parsed;
+ try {
+ parsed = Double.parseDouble(cs.toString());
+ } catch (NumberFormatException ne) {
+ throw formatException(cs);
+ }
+ // Casting a finite double to long saturates at the long bounds.
Reject values
+ // outside [Long.MIN_VALUE, 2^63) so 2.0E19 fails the same way 2E19
does.
+ if (!Double.isFinite(parsed) || parsed < Long.MIN_VALUE || parsed >=
0x1p63) {
Review Comment:
[P1] Validate decimal LONG values without rounding through double
`9.223372036854775807E18` is exactly `Long.MAX_VALUE`, but
`Double.parseDouble` rounds it to `2^63`, so this new check rejects a valid
value. A JSON field containing that quoted value, extracted as LONG, therefore
fails where the previous transform implementation returned `Long.MAX_VALUE`.
Unquoted decimal/exponent values can encounter the same boundary problem in
`JsonFunctions.longFromJsonNumber` because LONG extraction uses the
double-producing parser. The earlier precision issue also remains:
`9007199254740993.0E0` becomes `9007199254740992`.
Please truncate and validate the exact decimal representation before
converting to long, preserving that representation for unquoted numbers too.
Add tests above 2^53 and at both valid long boundaries.
--
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]
