xiangfu0 commented on code in PR #18872:
URL: https://github.com/apache/pinot/pull/18872#discussion_r3700951734
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/predicate/InPredicateEvaluatorFactory.java:
##########
@@ -135,12 +137,27 @@ public static InRawPredicateEvaluator
newRawValueBasedEvaluator(InPredicate inPr
}
case BYTES: {
ByteArray[] bytesValues = inPredicate.getBytesValues();
- Set<ByteArray> matchingValues = new
ObjectOpenHashSet<>(HashUtil.getMinHashSetSize(bytesValues.length));
+ // Keyed on the raw byte[] via fastutil's value-semantics strategy so
applySV can probe without wrapping
+ // each scanned value in a ByteArray.
+ Set<byte[]> matchingValues =
+ new
ObjectOpenCustomHashSet<>(HashUtil.getMinHashSetSize(bytesValues.length),
ByteArrays.HASH_STRATEGY);
// NOTE: Add value-by-value to avoid overhead
//noinspection ManualArrayToCollectionCopy
for (ByteArray value : bytesValues) {
//noinspection UseBulkOperation
- matchingValues.add(value);
+ matchingValues.add(value.getBytes());
Review Comment:
Reverted — all three sites are back to `ObjectOpenHashSet<ByteArray>` with
`contains(new ByteArray(value))`, so the BYTES case is byte-for-byte master's
again and UUID just adds a `case` alongside it that feeds the same evaluator.
For the record on *why* it was there: `applySV(byte[])` is handed a raw
`byte[]` per document, so probing a `Set<ByteArray>` allocates one wrapper per
row. Copilot's reviewer independently flagged the same thing on
`InTransformFunction` and suggested exactly this. But it changes shared BYTES
behavior for a non-UUID reason, so it does not belong in this PR — happy to
raise it separately if it seems worth it.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/predicate/InPredicateEvaluatorFactory.java:
##########
@@ -135,12 +137,27 @@ public static InRawPredicateEvaluator
newRawValueBasedEvaluator(InPredicate inPr
}
case BYTES: {
ByteArray[] bytesValues = inPredicate.getBytesValues();
- Set<ByteArray> matchingValues = new
ObjectOpenHashSet<>(HashUtil.getMinHashSetSize(bytesValues.length));
+ // Keyed on the raw byte[] via fastutil's value-semantics strategy so
applySV can probe without wrapping
+ // each scanned value in a ByteArray.
+ Set<byte[]> matchingValues =
+ new
ObjectOpenCustomHashSet<>(HashUtil.getMinHashSetSize(bytesValues.length),
ByteArrays.HASH_STRATEGY);
// NOTE: Add value-by-value to avoid overhead
//noinspection ManualArrayToCollectionCopy
for (ByteArray value : bytesValues) {
//noinspection UseBulkOperation
- matchingValues.add(value);
+ matchingValues.add(value.getBytes());
+ }
+ return new BytesRawValueBasedInPredicateEvaluator(inPredicate,
matchingValues);
+ }
+ // UUID is a logical type stored as 16 raw bytes, so -- like TIMESTAMP
over LONG above -- convert the
+ // literals to their stored form and reuse the stored-type evaluator.
+ case UUID: {
+ ByteArray[] uuidValues = inPredicate.getUuidValues();
+ Set<byte[]> matchingValues =
Review Comment:
Reverted — all three sites are back to `ObjectOpenHashSet<ByteArray>` with
`contains(new ByteArray(value))`, so the BYTES case is byte-for-byte master's
again and UUID just adds a `case` alongside it that feeds the same evaluator.
For the record on *why* it was there: `applySV(byte[])` is handed a raw
`byte[]` per document, so probing a `Set<ByteArray>` allocates one wrapper per
row. Copilot's reviewer independently flagged the same thing on
`InTransformFunction` and suggested exactly this. But it changes shared BYTES
behavior for a non-UUID reason, so it does not belong in this PR — happy to
raise it separately if it seems worth it.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/predicate/NotInPredicateEvaluatorFactory.java:
##########
@@ -135,12 +137,27 @@ public static NotInRawPredicateEvaluator
newRawValueBasedEvaluator(NotInPredicat
}
case BYTES: {
ByteArray[] bytesValues = notInPredicate.getBytesValues();
- Set<ByteArray> nonMatchingValues = new
ObjectOpenHashSet<>(HashUtil.getMinHashSetSize(bytesValues.length));
+ // Keyed on the raw byte[] via fastutil's value-semantics strategy so
applySV can probe without wrapping
+ // each scanned value in a ByteArray.
+ Set<byte[]> nonMatchingValues =
Review Comment:
Reverted — all three sites are back to `ObjectOpenHashSet<ByteArray>` with
`contains(new ByteArray(value))`, so the BYTES case is byte-for-byte master's
again and UUID just adds a `case` alongside it that feeds the same evaluator.
For the record on *why* it was there: `applySV(byte[])` is handed a raw
`byte[]` per document, so probing a `Set<ByteArray>` allocates one wrapper per
row. Copilot's reviewer independently flagged the same thing on
`InTransformFunction` and suggested exactly this. But it changes shared BYTES
behavior for a non-UUID reason, so it does not belong in this PR — happy to
raise it separately if it seems worth it.
--
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]
