Jackie-Jiang commented on code in PR #19247:
URL: https://github.com/apache/pinot/pull/19247#discussion_r3839904876
##########
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:
How are other data types handled?
##########
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:
Move it to follow `String[]`
##########
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:
Not introduced in this PR, but do we need to support Timestamp and UUID here?
##########
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:
Do you want to have a follow up to fix other types as well?
##########
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:
Why compiling without compilable check?
##########
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:
Is this a bug before? The value can be array even without BYTES_ARRAY
##########
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:
Avoid using stream API
##########
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:
What is time value?
##########
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:
Move it to follow `String[]`
--
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]
