Vamsi-klu commented on code in PR #18979:
URL: https://github.com/apache/pinot/pull/18979#discussion_r4051881073
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java:
##########
@@ -69,6 +76,13 @@ private JsonFunctions() {
.mappingProvider(new
JacksonMappingProvider()).options(Option.SUPPRESS_EXCEPTIONS)
.build());
+ // Mirrors JsonExtractScalarTransformFunction's BigDecimal-preserving
parser: BIG_DECIMAL / STRING / JSON
+ // extraction reads JSON floats as BigDecimal to avoid precision loss.
+ private static final ParseContext PARSE_CONTEXT_WITH_BIG_DECIMAL =
JsonPath.using(
Review Comment:
I kept the parser contexts shared in `JsonFunctions`, and the transform now
references those same instances. I removed the public read helper after the
later visibility feedback, so the transform uses its known STRING and BYTES
types directly.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/JsonExtractScalarTransformFunction.java:
##########
@@ -831,7 +813,9 @@ private <T> IntFunction<T> getResultExtractor(ValueBlock
valueBlock, ParseContex
boolean useBigDecimal) {
if (_jsonFieldTransformFunction.getResultMetadata().getDataType() ==
DataType.BYTES) {
byte[][] jsonBytes =
_jsonFieldTransformFunction.transformToBytesValuesSV(valueBlock);
- IntFunction<T> jaywayExtractor = i ->
parseContext.parseUtf8(jsonBytes[i]).read(_jsonPath);
+ IntFunction<T> jaywayExtractor =
Review Comment:
Reverted both generic dispatch calls. The transform now calls `parseUtf8`
for BYTES and `parse` for STRING directly, so there is no extra type check on
this path.
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java:
##########
@@ -611,6 +630,321 @@ 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/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. Without a default value an unresolved
single-value path throws; a
+ /// multi-value path yields an empty array, but a `null` element inside a
resolved array still throws.
+ /// A malformed JSON document is treated as unresolved.
+ @ScalarFunction
+ public static Object jsonExtractScalar(Object jsonInput, String jsonPath,
String resultsType) {
+ return jsonExtractScalar(jsonInput, jsonPath, resultsType, null);
+ }
+
+ /// See [#jsonExtractScalar(Object, String, String)]. `defaultValue` is
returned (coerced to `resultsType`)
+ /// when the path resolves to `null` or the document is malformed.
+ @ScalarFunction(nullableParameters = true)
+ public static Object jsonExtractScalar(@Nullable Object jsonInput, String
jsonPath, String resultsType,
+ @Nullable Object defaultValue) {
+ 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 (defaultValue != null) {
+ return coerceScalar(defaultValue, dataType);
+ }
+ throw new IllegalArgumentException(
+ "Cannot resolve JSON path on some records. Consider setting a
default value.");
+ }
+ return coerceScalar(value, dataType);
+ }
+ return coerceScalarArray(readJsonPathArray(jsonInput, jsonPath,
useBigDecimal), dataType, defaultValue);
+ }
+
+ /// Reads `jsonPath` from a JSON `String`, UTF-8 `byte[]`, or already-parsed
document.
+ /// Shared with `JsonExtractScalarTransformFunction` so both stay on the
same input dispatch.
+ /// A missing path returns `null` (`Option.SUPPRESS_EXCEPTIONS`). Malformed
input throws.
+ @Nullable
+ public static <T> T readJsonPathInternal(Object jsonInput, String jsonPath,
ParseContext parseContext) {
Review Comment:
Done. I removed the public `readJsonPathInternal` helper. The scalar parsing
helpers are private, and the transform only shares the parser contexts and
coercion methods it needs.
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java:
##########
@@ -611,6 +630,321 @@ 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/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. Without a default value an unresolved
single-value path throws; a
+ /// multi-value path yields an empty array, but a `null` element inside a
resolved array still throws.
+ /// A malformed JSON document is treated as unresolved.
+ @ScalarFunction
+ public static Object jsonExtractScalar(Object jsonInput, String jsonPath,
String resultsType) {
+ return jsonExtractScalar(jsonInput, jsonPath, resultsType, null);
+ }
+
+ /// See [#jsonExtractScalar(Object, String, String)]. `defaultValue` is
returned (coerced to `resultsType`)
+ /// when the path resolves to `null` or the document is malformed.
+ @ScalarFunction(nullableParameters = true)
+ public static Object jsonExtractScalar(@Nullable Object jsonInput, String
jsonPath, String resultsType,
+ @Nullable Object defaultValue) {
+ 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 (defaultValue != null) {
+ return coerceScalar(defaultValue, dataType);
+ }
+ throw new IllegalArgumentException(
+ "Cannot resolve JSON path on some records. Consider setting a
default value.");
+ }
+ return coerceScalar(value, dataType);
+ }
+ return coerceScalarArray(readJsonPathArray(jsonInput, jsonPath,
useBigDecimal), dataType, defaultValue);
+ }
+
+ /// Reads `jsonPath` from a JSON `String`, UTF-8 `byte[]`, or already-parsed
document.
+ /// Shared with `JsonExtractScalarTransformFunction` so both stay on the
same input dispatch.
+ /// A missing path returns `null` (`Option.SUPPRESS_EXCEPTIONS`). Malformed
input throws.
+ @Nullable
+ public static <T> T readJsonPathInternal(Object jsonInput, String jsonPath,
ParseContext parseContext) {
+ return parseJsonDocument(jsonInput, parseContext).read(jsonPath,
NO_PREDICATES);
+ }
+
+ /// Compiled-path counterpart of [#readJsonPathInternal(Object, String,
ParseContext)].
+ @Nullable
+ public static <T> T readJsonPathInternal(Object jsonInput, JsonPath
jsonPath, ParseContext parseContext) {
+ return parseJsonDocument(jsonInput, parseContext).read(jsonPath);
+ }
+
+ 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,
+ useBigDecimal ? PARSE_CONTEXT_WITH_BIG_DECIMAL : PARSE_CONTEXT));
+ } catch (Exception e) {
+ return null;
+ }
+ }
+
+ private static Object coerceScalar(Object value, DataType dataType) {
+ switch (dataType) {
+ case INT:
+ return toInt(value, false);
+ case BOOLEAN:
+ return toInt(value, true);
+ case LONG:
+ return toLong(value, false);
+ case TIMESTAMP:
+ return toLong(value, true);
+ case FLOAT:
+ return toFloat(value);
+ case DOUBLE:
+ return toDouble(value);
+ case BIG_DECIMAL:
+ return toBigDecimal(value);
+ case STRING:
+ case JSON:
+ return toStringValue(value);
+ case BYTES:
+ return BytesUtils.toBytes(value.toString());
+ default:
+ throw new
IllegalArgumentException(unsupportedResultsTypeMessage(dataType.name()));
+ }
+ }
+
+ private static Object coerceScalarArray(@Nullable Object[] array, DataType
dataType, @Nullable Object defaultValue) {
+ switch (dataType) {
Review Comment:
Added both types. `BOOLEAN_ARRAY` uses Pinot's stored `int[]` form and
`TIMESTAMP_ARRAY` uses `long[]`, with focused scalar and query coverage.
##########
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.spi.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());
Review Comment:
Fixed. Decimal and exponent LONG values now use exact `BigDecimal` parsing,
truncate toward zero, and check the long range before conversion. Tests cover
values above 2^53 and both long boundaries.
##########
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:
Fixed. The code compiles the JSONPath before parsing the document, and only
malformed document parsing is suppressed. Invalid paths now propagate for
scalar and array calls, with and without defaults.
--
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]
