imply-cheddar commented on code in PR #13977:
URL: https://github.com/apache/druid/pull/13977#discussion_r1158089484
##########
processing/src/main/java/org/apache/druid/segment/nested/NestedFieldColumnIndexSupplier.java:
##########
@@ -119,8 +121,8 @@ public NestedFieldColumnIndexSupplier(
this.arrayElementBitmaps = arrayElementBitmaps;
this.adjustLongId = globalStringDictionarySupplier.get().size();
this.adjustDoubleId = adjustLongId +
globalLongDictionarySupplier.get().size();
- this.skipRangeIndexThreshold = (int) Math.ceil(skipValueRangeIndexScale *
numRows);
- this.skipPredicateIndex = localDictionarySupplier.get().size() >
Math.ceil(skipValuePredicateIndexScale * numRows);
+ this.skipRangeIndexThreshold = (int)
Math.ceil(columnConfig.skipValueRangeIndexScale() * numRows);
+ this.skipPredicateIndex = localDictionarySupplier.get().size() >
Math.ceil(columnConfig.skipValuePredicateIndexScale() * numRows);
Review Comment:
I think it's a bit nicer to make these methods instead of fields that are
computed on every constructor. The computation that's being done is minimal,
that's true, but it's better to only do it when needed. I think that if you
just store the reference to the `ColumnConfig` you can pretty easily move this
check into private methods. All of the places where you would check for
skipping the predicate index, you would probably also be grabbing a
`localDictionarySupplier` anyway, so you can reuse that singular reference too.
--
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]
