Jackie-Jiang commented on code in PR #18979: URL: https://github.com/apache/pinot/pull/18979#discussion_r3929909784
########## pinot-spi/src/main/java/org/apache/pinot/spi/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: Decimal-exponent LONG values are parsed through double, which silently changes integers above 2^53. For example, 9007199254740993.0E0 becomes 9007199254740992. Consider using exact decimal parsing, truncating toward zero, then validating the long range exactly; boundary tests above 2^53 and around both long limits would cover this. ########## 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) Review Comment: This overload can return Java null when an explicit SQL NULL default is supplied, but its public return contract is not annotated nullable. Consider adding @Nullable to the return value so callers and static analysis see that behavior. ########## pinot-spi/src/main/java/org/apache/pinot/spi/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: This is a concrete JSON-number implementation utility rather than an SPI contract. Consider placing it and its test in pinot-common and updating the core wrapper import, avoiding expansion of the lowest-level SPI surface. ########## pinot-spi/src/test/java/org/apache/pinot/spi/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: (minor) The repository's test convention prefers static TestNG assertion imports. Consider importing assertEquals and assertThrows statically and using the direct calls below. ########## 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: This broad catch also swallows invalid JSONPath syntax, so malformed paths are reported as unresolved or replaced by the caller's default. Consider separating document parsing from path evaluation and suppressing only malformed-document failures so invalid path expressions still propagate. The array helper has the same behavior. -- 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]
