Jackie-Jiang commented on code in PR #10386:
URL: https://github.com/apache/pinot/pull/10386#discussion_r1153673850
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/BaseTransformFunction.java:
##########
@@ -121,6 +132,11 @@ public int[] transformToIntValuesSV(ValueBlock valueBlock)
{
} else {
DataType resultDataType = getResultMetadata().getDataType();
switch (resultDataType.getStoredType()) {
+ case UNKNOWN:
Review Comment:
Move this case to the end, same for other places
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/BaseTransformFunction.java:
##########
@@ -92,7 +97,13 @@ public abstract class BaseTransformFunction implements
TransformFunction {
protected String[][] _stringValuesMV;
protected byte[][][] _bytesValuesMV;
- @Nullable
+ protected List<TransformFunction> _arguments;
+
+ // NOTE: this init has to be called for default getNullBitmap()
implementation to be effective.
+ public void init(List<TransformFunction> arguments) {
Review Comment:
Suggest calling it `initArguments()` or `setArguments()` to avoid mixing it
with the `TransformFunction.init()`, or override the `init()` with the same
parameters
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/AdditionTransformFunction.java:
##########
@@ -74,6 +74,7 @@ public void init(List<TransformFunction> arguments,
Map<String, ColumnContext> c
if (_resultDataType == DataType.BIG_DECIMAL) {
_literalBigDecimalSum =
_literalBigDecimalSum.add(BigDecimal.valueOf(_literalDoubleSum));
}
+ super.init(arguments);
Review Comment:
(convention) We usually call `super.init()` as first statement
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/BaseTransformFunction.java:
##########
@@ -617,4 +1261,50 @@ public byte[][][] transformToBytesValuesMV(ValueBlock
valueBlock) {
}
return _bytesValuesMV;
}
+
+ @Override
+ public Pair<byte[][][], RoaringBitmap>
transformToBytesValuesMVWithNull(ValueBlock valueBlock) {
+ int length = valueBlock.getNumDocs();
+ if (_bytesValuesMV == null) {
+ _bytesValuesMV = new byte[length][][];
+ }
+ RoaringBitmap bitmap;
+ DataType resultDataType = getResultMetadata().getDataType();
+ switch (resultDataType) {
+ case UNKNOWN:
+ bitmap = new RoaringBitmap();
+ bitmap.add(0L, length);
+ for (int i = 0; i < length; i++) {
+ _bytesValuesMV[i] = (byte[][])
DataSchema.ColumnDataType.BYTES_ARRAY.getNullPlaceholder();
+ }
+ break;
+ case STRING:
+ Pair<String[][], RoaringBitmap> stringResult =
transformToStringValuesMVWithNull(valueBlock);
+ bitmap = stringResult.getRight();
+ ArrayCopyUtils.copy(stringResult.getLeft(), _bytesValuesMV, length);
+ break;
+ case BYTES:
+ _bytesValuesMV = transformToBytesValuesMV(valueBlock);
+ bitmap = getNullBitmap(valueBlock);
+ break;
+ default:
+ throw new IllegalStateException(String.format("Cannot read MV %s as
bytes", resultDataType));
+ }
+ return ImmutablePair.of(_bytesValuesMV, bitmap);
+ }
+
+ @Override
+ public @Nullable RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
Review Comment:
(code format) Put `@Nullable` above `@Override`, same for other places
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/BaseTransformFunction.java:
##########
@@ -617,4 +1261,50 @@ public byte[][][] transformToBytesValuesMV(ValueBlock
valueBlock) {
}
return _bytesValuesMV;
}
+
+ @Override
+ public Pair<byte[][][], RoaringBitmap>
transformToBytesValuesMVWithNull(ValueBlock valueBlock) {
+ int length = valueBlock.getNumDocs();
+ if (_bytesValuesMV == null) {
+ _bytesValuesMV = new byte[length][][];
+ }
+ RoaringBitmap bitmap;
+ DataType resultDataType = getResultMetadata().getDataType();
+ switch (resultDataType) {
+ case UNKNOWN:
+ bitmap = new RoaringBitmap();
+ bitmap.add(0L, length);
+ for (int i = 0; i < length; i++) {
+ _bytesValuesMV[i] = (byte[][])
DataSchema.ColumnDataType.BYTES_ARRAY.getNullPlaceholder();
+ }
+ break;
+ case STRING:
+ Pair<String[][], RoaringBitmap> stringResult =
transformToStringValuesMVWithNull(valueBlock);
+ bitmap = stringResult.getRight();
+ ArrayCopyUtils.copy(stringResult.getLeft(), _bytesValuesMV, length);
+ break;
+ case BYTES:
+ _bytesValuesMV = transformToBytesValuesMV(valueBlock);
+ bitmap = getNullBitmap(valueBlock);
+ break;
+ default:
+ throw new IllegalStateException(String.format("Cannot read MV %s as
bytes", resultDataType));
+ }
+ return ImmutablePair.of(_bytesValuesMV, bitmap);
+ }
+
+ @Override
+ public @Nullable RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
+ if (_arguments == null) {
Review Comment:
`_arguments` shouldn't be `null` if all the transform functions call the
`init()`. Do we plan to do that separately? If so, let's add a TODO
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/BaseTransformFunction.java:
##########
@@ -617,4 +1261,50 @@ public byte[][][] transformToBytesValuesMV(ValueBlock
valueBlock) {
}
return _bytesValuesMV;
}
+
+ @Override
+ public Pair<byte[][][], RoaringBitmap>
transformToBytesValuesMVWithNull(ValueBlock valueBlock) {
+ int length = valueBlock.getNumDocs();
+ if (_bytesValuesMV == null) {
+ _bytesValuesMV = new byte[length][][];
+ }
+ RoaringBitmap bitmap;
+ DataType resultDataType = getResultMetadata().getDataType();
+ switch (resultDataType) {
+ case UNKNOWN:
+ bitmap = new RoaringBitmap();
+ bitmap.add(0L, length);
+ for (int i = 0; i < length; i++) {
+ _bytesValuesMV[i] = (byte[][])
DataSchema.ColumnDataType.BYTES_ARRAY.getNullPlaceholder();
+ }
+ break;
+ case STRING:
+ Pair<String[][], RoaringBitmap> stringResult =
transformToStringValuesMVWithNull(valueBlock);
+ bitmap = stringResult.getRight();
+ ArrayCopyUtils.copy(stringResult.getLeft(), _bytesValuesMV, length);
+ break;
+ case BYTES:
+ _bytesValuesMV = transformToBytesValuesMV(valueBlock);
+ bitmap = getNullBitmap(valueBlock);
+ break;
+ default:
+ throw new IllegalStateException(String.format("Cannot read MV %s as
bytes", resultDataType));
+ }
+ return ImmutablePair.of(_bytesValuesMV, bitmap);
+ }
+
+ @Override
+ public @Nullable RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
+ if (_arguments == null) {
+ return null;
+ }
+ RoaringBitmap bitmap = new RoaringBitmap();
+ for (TransformFunction arg : _arguments) {
+ RoaringBitmap argBitmap = arg.getNullBitmap(valueBlock);
+ if (argBitmap != null) {
+ bitmap.or(argBitmap);
+ }
+ }
+ return bitmap;
Review Comment:
(minor) Should we return `null` when no argument has bitmap?
--
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]
