clintropolis commented on code in PR #15012:
URL: https://github.com/apache/druid/pull/15012#discussion_r1330943355
##########
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:
hmm, actually is that right? Thinking a bit more about this, maybe I'm wrong
and reverse mapping actually needs to be a list of ints associated with each
value if there is a many to one mapping.. which introduces a lot of extra
complexity in places (i've also found some bugs in the index supplier
implementation of this virtual column). Let me think a bit more about this...
--
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]
