imply-cheddar commented on code in PR #15687:
URL: https://github.com/apache/druid/pull/15687#discussion_r1452857623
##########
processing/src/main/java/org/apache/druid/segment/serde/NestedCommonFormatColumnPartSerde.java:
##########
@@ -308,6 +308,10 @@ public void read(ByteBuffer buffer, ColumnBuilder builder,
ColumnConfig columnCo
ColumnType logicalType = simpleType == null ? ColumnType.NESTED_DATA :
simpleType;
builder.setType(logicalType);
builder.setNestedCommonFormatColumnSupplier(supplier);
+ // nested columns only have a null value index
+ if (hasNulls) {
+ builder.setIndexSupplier(supplier, false, false);
+ }
Review Comment:
I wonder if it shouldn't be another class that depends on the `supplier`
instead of just the supplier itself. Specifically, I think that if the index
supplier ends up needing to track resources to like decompress something or
whatever, we will have lifecycle management issues. The `Column` itself
operates in a managed lifecycle, so it might actually be better to move this
`IndexSupplier` to be expected to exist on the `Column` itself, which might
simplify reuse as well?
Definitely a larger change than what you have here though.
--
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]
