Jackie-Jiang commented on code in PR #18979: URL: https://github.com/apache/pinot/pull/18979#discussion_r4364678024
########## 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 { Review Comment: Verified in ae353ebf6b. JsonNumberUtils and its tests live in pinot-common, and the existing core NumberUtils.parseJsonLong entry point delegates to it. No new SPI dependency is needed. ########## pinot-common/src/test/java/org/apache/pinot/common/utils/JsonNumberUtilsTest.java: ########## @@ -0,0 +1,105 @@ +/** + * 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; + +import org.testng.Assert; Review Comment: Verified in ae353ebf6b. JsonNumberUtilsTest uses static TestNG assertion imports. ########## 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: Verified in ae353ebf6b. Quoted decimal-exponent values are parsed and range-checked exactly, and the LONG parse context preserves unquoted decimals. The regression cases retain Long.MAX_VALUE and the low bit of 9007199254740993.0E0. Already-materialized floating-point inputs use a separate guarded cast to preserve their represented value. ########## 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: Verified in ae353ebf6b. A resolved scalar or object requested as an array produces an empty array, while actual List/Object[] values remain supported. Runtime tests also cover explicit NULL defaults so resolved non-array values are not confused with unresolved paths. ########## 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: Verified in ae353ebf6b. Literal folding handles primitive arrays and stored BOOLEAN values. I also completed BOOLEAN array-literal numeric/string conversions and narrowed projection type restoration in 996a97fb30. QueryCompilationTest, ResourceBasedQueryPlansTest, and the composed arraySum regression all pass. ########## 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: Verified in ae353ebf6b. Path compilation stays outside the fallback catch, so `$[` is rejected for scalar and array calls, with or without defaults. The latest fix also catches failures while evaluating a valid path, such as avg() on an empty array, so those use the default just as the leaf transform does. Value-conversion errors still propagate. -- 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]
