xiangfu0 commented on code in PR #18872:
URL: https://github.com/apache/pinot/pull/18872#discussion_r3680151986
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/predicate/EqualsPredicateEvaluatorFactory.java:
##########
@@ -93,8 +100,12 @@ private static final class
DictionaryBasedEqPredicateEvaluator extends BaseDicti
DictionaryBasedEqPredicateEvaluator(EqPredicate eqPredicate, Dictionary
dictionary, DataType dataType) {
super(eqPredicate, dictionary);
- String predicateValue =
PredicateUtils.getStoredValue(eqPredicate.getValue(), dataType);
Review Comment:
Agreed — it already is, so the branch was redundant. Removed from both
`EqualsPredicateEvaluatorFactory` and `NotEqualsPredicateEvaluatorFactory`,
which are now just:
```java
String predicateValue =
PredicateUtils.getStoredValue(eqPredicate.getValue(), dataType);
_matchingDictId = dictionary.indexOf(predicateValue);
```
`PredicateUtils.getStoredValue` has a `case UUID` returning the hex form,
and `BaseImmutableDictionary.indexOf(String)` delegates to
`insertionIndexOf(String)`, which for `BytesDictionary` is
`binarySearch(BytesUtils.toBytes(stringValue))` — i.e. it hex-decodes. So the
string path resolves to the same dict id as the `ByteArray` lookup I had added.
`UuidDictionaryPredicateEvaluatorTest.testStoredValueMatchesRawByteLookup`
asserts exactly that equivalence against a real
`BytesOffHeapMutableDictionary`, so the simplification is covered rather than
assumed.
--
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]
