Jackie-Jiang commented on code in PR #18979:
URL: https://github.com/apache/pinot/pull/18979#discussion_r4076796596
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/operands/FunctionOperand.java:
##########
@@ -121,6 +130,19 @@ public Object apply(List<Object> row) {
Object value = operand.apply(row);
_reusableOperandHolder[i] = value != null ?
operand.getResultType().toExternal(value) : null;
}
+ if (_replaceNullJsonOperands) {
+ if (_reusableOperandHolder[0] == null) {
+ ColumnDataType inputType = _operands.get(0).getResultType();
+ Object nullPlaceholder = inputType.getNullPlaceholder();
+ // An untyped SQL NULL has UNKNOWN type and therefore no generic
placeholder. The leaf transform reads every
+ // non-BYTES JSON input through transformToStringValuesSV(), whose
NULL literal value is the empty string.
+ _reusableOperandHolder[0] = nullPlaceholder != null ?
inputType.toExternal(nullPlaceholder) : "";
+ }
+ if (_reusableOperandHolder.length == 4 && _reusableOperandHolder[3] ==
null
+ && _reusableOperandHolder[2] != null) {
+ _reusableOperandHolder[3] =
getJsonNullDefault(_reusableOperandHolder[2].toString());
Review Comment:
Optional simplification: precompute the NULL default
The result-type argument and explicit default are validated literals, so the
replacement for a NULL default can be prepared once in the constructor. Calling
`getJsonNullDefault()` here repeats uppercasing, `_ARRAY` suffix handling, type
dispatch, and the empty byte-array allocation for every affected row. Caching
the prepared replacement would simplify this evaluation path while preserving
the null-handling behavior.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/ArrayLiteralTransformFunction.java:
##########
@@ -144,6 +144,17 @@ public
ArrayLiteralTransformFunction(List<ExpressionContext> literalContexts) {
}
_dataType = literalContexts.get(0).getLiteral().getType();
switch (_dataType) {
+ case BOOLEAN:
Review Comment:
[P2] Complete conversions for the new BOOLEAN array literal
This branch stores Boolean values in `_intArrayLiteral` while leaving
`_dataType` as BOOLEAN, but the long/float/double/string conversion switches
still lack BOOLEAN handling. For example, `SELECT
arraySum(jsonExtractScalar('{"v":[true,false]}', '$.v', 'BOOLEAN_ARRAY')) FROM
a` folds the inner expression to an ARRAY call and leaves the outer function
for runtime evaluation. At the leaf, `ArraySumTransformFunction` accepts
BOOLEAN through its numeric stored INT type, then calls
`transformToDoubleValuesMV()`, which throws `Unable to convert data type:
BOOLEAN to double array`. Please complete the conversions for this
representation and cover a composed consumer such as arraySum; the direct
int-array assertion does not exercise this path.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotEvaluateLiteralRule.java:
##########
@@ -88,13 +91,15 @@ private static LogicalProject
constructNewProject(LogicalProject oldProject, Log
List<RexNode> castedNewProjects = new ArrayList<>(numProjects);
boolean needCast = false;
for (int i = 0; i < numProjects; i++) {
- RexNode oldNode = oldProjects.get(i);
RexNode newNode = newProjects.get(i);
+ RelDataType expectedType =
oldProject.getRowType().getFieldList().get(i).getType();
// Need to cast the result to the original type if the literal type is
changed, e.g. VARCHAR literal is typed as
- // CHAR(STRING_LENGTH) in Calcite, but we need to cast it back to
VARCHAR.
- if (!oldNode.getType().equals(newNode.getType())) {
+ // CHAR(STRING_LENGTH) in Calcite, but we need to cast it back to
VARCHAR. Use the project's declared row type
+ // instead of the original expression type because a nullable
user-defined function can retain a non-null operand
+ // type while the validated projection is nullable.
+ if (!expectedType.equals(newNode.getType())) {
needCast = true;
- newNode = rexBuilder.makeCast(oldNode.getType(), newNode, true);
+ newNode = rexBuilder.makeAbstractCast(expectedType, newNode, false);
Review Comment:
[P2] Narrow the projection cast change to the required type-restoration case
Replacing `makeCast` with `makeAbstractCast` here affects every restored
projection type, including unrelated folded string and spatial functions. It
leaves explicit CAST nodes around `substr('month', 2)`, `concat('month', '
1')`, nested upper/lower/substr, and `ST_Point(20, 10, 1)`. These are the four
failing `literal_evaluation_tests` cases in [Unit Test Set
1](https://github.com/apache/pinot/actions/runs/35414159549/job/105819375038),
which also prevents the downstream query-runtime tests from running. Please
preserve ordinary literal normalization and narrowly handle the
JSON/nullability case that needs different restoration. The demonstrated
regression here is plan churn and failing planner tests; I have not established
incorrect query results.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotEvaluateLiteralRule.java:
##########
@@ -322,4 +354,38 @@ private static Object convertResultValue(@Nullable Object
resultValue, RelDataTy
// TODO: Add more type handling
return resultValue;
}
+
+ /// Boxes a Java array so the folder can build a Calcite array literal.
`jsonExtractScalar`
Review Comment:
Optional simplification: build the converted array values in one pass
This planner-time helper duplicates the same boxing loop for Object[],
int[], long[], float[], double[], and boolean[], and its caller immediately
copies the resulting list into another list while converting each element. A
single loop using `java.lang.reflect.Array.getLength()` and `Array.get()` can
handle the supported primitive/object arrays, call `convertResultValue()`
directly, and produce the final list. That would remove the type branches and
intermediate collection without removing support for primitive-array results.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/JsonNumberUtils.java:
##########
@@ -0,0 +1,237 @@
+/**
+ * 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;
+
+import java.math.BigDecimal;
+
+
+/// 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`. Dotted-exponent values are parsed as an exact
decimal ([BigDecimal]),
+/// truncated toward zero, then range-checked, so `9.223372036854775807E18` is
[Long#MAX_VALUE] and
+/// `9007199254740993.0E0` keeps the bit that a `double` would drop. 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 BigDecimal LONG_MIN_MINUS_ONE = new
BigDecimal("-9223372036854775809");
+ private static final BigDecimal LONG_MAX_PLUS_ONE = new
BigDecimal("9223372036854775808");
+
+ 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) {
+ // Do not go through double: 9.223372036854775807E18 is
Long.MAX_VALUE, but
+ // Double.parseDouble rounds it to 2^63, which is out of range.
+ return parseExactLong(cs);
+ }
+
+ long exp;
+ try {
+ exp = parseWholeLong(cs, i, len);
+ } catch (NumberFormatException nfe) {
+ throw new NumberFormatException("Wrong exponent");
+ }
+
+ if (exp < 0 || exp >= POWERS_OF_10.length) {
+ throw new NumberFormatException("Wrong exponent");
+ }
+
+ try {
+ return Math.multiplyExact(negative ? result : -result,
POWERS_OF_10[(int) exp]);
+ } catch (ArithmeticException e) {
+ throw formatException(cs);
+ }
+ }
+
+ return negative ? result : -result;
+ }
+
+ /// Parses a JSON numeric string as an exact decimal, truncates toward zero,
then requires a long.
+ private static long parseExactLong(CharSequence cs) {
+ try {
+ return truncateToLong(new BigDecimal(cs.toString()));
+ } catch (NumberFormatException e) {
+ throw formatException(cs);
+ }
+ }
+
+ /// Truncates an exact decimal toward zero and requires the result to fit in
a long.
+ ///
+ /// The comparisons happen before conversion so a value such as
`1E999999999` is rejected
+ /// without expanding its exponent into an enormous integer. Values strictly
+ /// between `Long.MIN_VALUE - 1` and `Long.MAX_VALUE + 1` truncate into the
long range.
+ public static long truncateToLong(BigDecimal value) {
+ if (value.compareTo(LONG_MIN_MINUS_ONE) <= 0 ||
value.compareTo(LONG_MAX_PLUS_ONE) >= 0) {
+ throw formatException(value.toString());
+ }
+ return value.longValue();
+ }
+
+ /// Parses `cs[start, end)` as a whole long (sign allowed). Used for the
exponent field.
+ private static long parseWholeLong(CharSequence cs, int start, int end) {
Review Comment:
Optional simplification: reuse the JDK parser for the exponent
`parseWholeLong()` is a second signed-long parser used only for the exponent
substring. An ASCII-digit validation pass (allowing the existing leading sign),
followed by `Long.parseLong(cs, start, end, 10)`, can replace the manual
accumulation and overflow checks. Keep the ASCII validation because the JDK
accepts Unicode digits, and retain the caller's `Wrong exponent` exception
translation so behavior stays consistent.
--
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]
