xiangfu0 commented on code in PR #19247:
URL: https://github.com/apache/pinot/pull/19247#discussion_r3841361823
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -2116,6 +2116,20 @@ private void computeResultsForExpression(Expression
expression, String[] columnN
List<Expression> operands = function.getOperands();
computeResultsForExpression(operands.get(0), columnNames, columnTypes,
values, index);
columnNames[index] = operands.get(1).getIdentifier().getName();
+ } else if (operator.equals("arrayvalueconstructor")) {
Review Comment:
Other supported array literals (INT, LONG, FLOAT, DOUBLE, and STRING) fold
into their existing native Thrift Literal arms and are handled by
computeResultsForLiteral. BYTES_ARRAY uniquely remains an arrayValueConstructor
of legacy binary literals for old-server decoding, so broker-only literal
queries materialize that preserved representation here. The broker test now
covers INT and STRING generic handling alongside BYTES.
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ArrayFunctions.java:
##########
@@ -352,6 +352,13 @@ public static Object arrayValueConstructor(Object... arr) {
}
return strArr;
}
+ if (clazz == byte[].class) {
Review Comment:
Tracked in #19338. TIMESTAMP and UUID need their typed array representation
and conversion contracts defined across both engines, so I kept that broader
work separate from BYTES_ARRAY.
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java:
##########
@@ -147,15 +153,17 @@ public LiteralContext(DataType type, @Nullable Object
value) {
_pinotDataType = getPinotDataType(type, value);
}
- // TODO: Revisit MV support for BOOLEAN, BIG_DECIMAL, BYTES and UUID.
+ // TODO: Revisit MV support for BOOLEAN, BIG_DECIMAL and UUID.
Review Comment:
Yes, tracked in #19338 for BOOLEAN, BIG_DECIMAL, and UUID multi-value
literal support, including the required wire and conversion coverage.
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java:
##########
@@ -297,7 +305,7 @@ public boolean isNull() {
@Override
public int hashCode() {
- return Objects.hash(_value, _type);
+ return Arrays.deepHashCode(new Object[]{_value, _type});
Review Comment:
Yes, this was a pre-existing bug for every array-backed value:
Objects.equals/hashCode used identity semantics for arrays. This now uses deep
equality and hash semantics, with regression coverage for all primitive/object
arrays plus scalar byte[] and multi-value byte[][].
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java:
##########
@@ -333,6 +341,9 @@ public String toString() {
return "'" + Arrays.toString((double[]) _value) + "'";
case STRING_ARRAY:
return "'" + Arrays.toString((String[]) _value) + "'";
+ case BYTES_ARRAY:
+ return "'" + Arrays.toString(
+ Arrays.stream((byte[][])
_value).map(BytesUtils::toHexString).toArray(String[]::new)) + "'";
Review Comment:
Done. Replaced the stream pipeline with an indexed loop that fills the hex
string array directly; the focused tests pass.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java:
##########
@@ -210,6 +219,9 @@ public static Literal getLiteral(@Nullable Object object) {
if (object instanceof byte[]) {
return getLiteral((byte[]) object);
}
+ if (object instanceof byte[][]) {
Review Comment:
Done. The byte[][] branch now follows String[].
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java:
##########
@@ -309,6 +324,10 @@ public static Expression getLiteralExpression(byte[]
value) {
return getLiteralExpression(getLiteral(value));
}
+ public static Expression getLiteralExpression(byte[][] value) {
Review Comment:
Done. The byte[][] overload now follows String[].
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/CompileTimeFunctionsInvoker.java:
##########
@@ -72,9 +72,8 @@ public static Expression
invokeCompileTimeFunctionExpression(@Nullable Expressio
for (int i = 0; i < numOperands; i++) {
Expression operand =
invokeCompileTimeFunctionExpression(operands.get(i));
operands.set(i, operand);
- Literal literal = operand.getLiteral();
- if (compilable && literal != null) {
- Pair<ColumnDataType, Object> typeAndValue =
RequestUtils.getLiteralTypeAndValue(literal);
+ Pair<ColumnDataType, Object> typeAndValue = getCompileTimeValue(operand);
Review Comment:
Restored the compilable guard before extracting operand values. Child
expressions are still visited intentionally so independent deterministic
children can be folded even when the parent is not compilable.
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/CompileTimeFunctionsInvoker.java:
##########
@@ -100,11 +99,39 @@ public static Expression
invokeCompileTimeFunctionExpression(@Nullable Expressio
invoker.convertTypes(arguments);
result = invoker.invoke(arguments);
}
+ // Literal has a BYTES_ARRAY arm for upgraded readers, but ordinary
broker-to-server requests must remain
+ // decodable by older servers. Preserve the constructor with scalar
BINARY_VALUE operands until the wire has
+ // server capability negotiation.
+ if (result instanceof byte[][]) {
+ return expression;
+ }
return RequestUtils.getLiteralExpression(result);
} catch (Exception e) {
throw new SqlCompilationException(
"Caught exception while invoking method: " +
functionInfo.getMethod().getName() + " with arguments: "
+ Arrays.toString(arguments) + ": " + e.getMessage(), e);
}
}
+
+ @Nullable
+ private static Pair<ColumnDataType, Object> getCompileTimeValue(Expression
expression) {
Review Comment:
Renamed the helper to getLiteralOperandTypeAndValue and documented that the
preserved BYTES constructor is treated as a literal so deterministic parent
functions can still be folded.
--
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]
