You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Copilot (via GitHub)" <gi...@apache.org> on 2026/04/25 01:16:16 UTC
Re: [PR] Support MV BIG_DECIMAL and BYTES [pinot]
Copilot commented on code in PR #18326:
URL: https://github.com/apache/pinot/pull/18326#discussion_r3141022515
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datablock/DataBlockBuilder.java:
##########
@@ -458,6 +479,21 @@ private static void serializeColumnData(List<Object[]> columns, DataSchema dataS
});
break;
}
+ case BYTES_ARRAY: {
+ ByteArray[] nullPlaceholder = (ByteArray[]) storedType.getNullPlaceholder();
+ interruptableLoop(0, numRows, interruptableLoopStep, (start, end) -> {
+ for (int rowId = 0; rowId < numRows; rowId++) {
+ Object value = column[rowId];
+ if (value == null) {
+ nullBitmap.add(rowId);
+ setColumn(fixedSize, varSize, nullPlaceholder);
+ } else {
+ setColumn(fixedSize, varSize, (ByteArray[]) value);
+ }
+ }
Review Comment:
Same chunking issue as BIG_DECIMAL_ARRAY: the BYTES_ARRAY branch ignores interruptableLoop's (start,end) and loops rowId from 0..numRows, which will duplicate writes when numRows > interruptableLoopStep. Use rowId from start to end.
##########
pinot-core/src/main/java/org/apache/pinot/core/common/datablock/DataBlockBuilder.java:
##########
@@ -443,6 +449,21 @@ private static void serializeColumnData(List<Object[]> columns, DataSchema dataS
});
break;
}
+ case BIG_DECIMAL_ARRAY: {
+ BigDecimal[] nullPlaceholder = (BigDecimal[]) storedType.getNullPlaceholder();
+ interruptableLoop(0, numRows, interruptableLoopStep, (start, end) -> {
+ for (int rowId = 0; rowId < numRows; rowId++) {
+ Object value = column[rowId];
+ if (value == null) {
+ nullBitmap.add(rowId);
+ setColumn(fixedSize, varSize, nullPlaceholder);
+ } else {
+ setColumn(fixedSize, varSize, (BigDecimal[]) value);
+ }
+ }
Review Comment:
In the BIG_DECIMAL_ARRAY branch, the interruptableLoop callback ignores the provided (start,end) and loops rowId from 0..numRows. This will re-serialize the full column for every chunk when numRows > interruptableLoopStep, corrupting the buffers / causing BufferOverflow. Iterate rowId from start to end (like the primitive cases above).
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datablock/DataBlockTestUtils.java:
##########
@@ -129,6 +129,22 @@ public static Object[] getRandomRow(DataSchema dataSchema, int nullPercentile) {
}
row[colId] = timestampArray;
break;
+ case STRING_ARRAY:
+ length = RANDOM.nextInt(ARRAY_SIZE);
+ String[] stringArray = new String[length];
+ for (int i = 0; i < length; i++) {
+ stringArray[i] = RandomStringUtils.random(RANDOM.nextInt(20));
+ }
+ row[colId] = stringArray;
+ break;
+ case BYTES_ARRAY:
+ length = RANDOM.nextInt(ARRAY_SIZE);
+ ByteArray[] bytesArray = new ByteArray[length];
+ for (int i = 0; i < length; i++) {
+ bytesArray[i] = new ByteArray(RandomStringUtils.random(RANDOM.nextInt(20)).getBytes());
+ }
Review Comment:
Test utility uses String.getBytes() without specifying a charset when building ByteArray values. This is platform-default dependent and can make tests flaky across environments; prefer StandardCharsets.UTF_8 (or reuse the charset used elsewhere in the test suite).
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RexExpressionUtils.java:
##########
@@ -148,16 +147,6 @@ public static RexLiteral toRexLiteral(RelBuilder builder, RexExpression.Literal
ByteString byteString = new ByteString(bytes);
return rexBuilder.makeBinaryLiteral(byteString);
}
- case BOOLEAN_ARRAY:
- case BYTES_ARRAY:
- case DOUBLE_ARRAY:
- case FLOAT_ARRAY:
- case INT_ARRAY:
- case LONG_ARRAY:
- case STRING_ARRAY:
- case TIMESTAMP_ARRAY:
- case OBJECT:
- case UNKNOWN:
default:
throw new IllegalStateException("Unsupported ColumnDataType: " + literal.getDataType());
Review Comment:
RexExpressionUtils.toRexLiteral() no longer handles ColumnDataType.JSON (it will now hit the default branch and throw). JSON literals were previously treated similarly to STRING; if JSON literals can appear in RexExpression.Literal (e.g., from proto conversion), this is a regression. Consider adding a JSON case (likely mapping to rexBuilder.makeLiteral(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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org