clintropolis commented on code in PR #15012:
URL: https://github.com/apache/druid/pull/15012#discussion_r1330969196
##########
processing/src/main/java/org/apache/druid/query/dimension/ListFilteredDimensionSpec.java:
##########
@@ -92,15 +92,17 @@ public static IdMapping buildAllowListIdMapping(
IndexedGetter<String> fn
)
{
- final IdMapping.Builder builder =
IdMapping.Builder.ofCardinality(values.size());
+ IdMapping.Builder builder;
if (idLookup != null) {
+ builder = IdMapping.Builder.ofCardinality(values.size());
for (String value : values) {
int i = idLookup.lookupId(value);
if (i >= 0) {
builder.addMapping(i);
}
}
} else {
+ builder = IdMapping.Builder.ofCardinality(cardinality);
Review Comment:
ok, ignore all of this, we talked about this offline, but to capture that
discussion here: I think the _real_ right thing to do here is have
`ListFilteredVirtualColumn` and `ListFilteredDimensionSpec` force use of
`PredicateFilteredDimensionSelector` whenever the extractionFn is present on
the `delegate` `DimensionSpec`, for both allow or deny lists.
Additionally `ListFilteredVirtualColumn` also should return
`NoIndexesColumnIndexSupplier.getInstance()` whenever `getIndexSupplier` is
called and the `delegate` has an `extractionFn`.
The reason is that use of `IdMapping` requires a 1:1 relation between the
filtered dimension and underlying dimension, and that isn't really possible to
do with a many to one lookup.
--
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]
