Jackie-Jiang commented on code in PR #18872:
URL: https://github.com/apache/pinot/pull/18872#discussion_r3700668004
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/LiteralContext.java:
##########
@@ -174,6 +174,8 @@ private static PinotDataType getPinotDataType(DataType
type, @Nullable Object va
return PinotDataType.BIG_DECIMAL;
case STRING:
return singleValue ? PinotDataType.STRING : PinotDataType.STRING_ARRAY;
+ case UUID:
Review Comment:
Since we don't support BYTES_ARRAY, how do we support UUID_ARRAY here?
Is BYTES_ARRAY already supported but this class not updated?
##########
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:
Why changing this and also the map?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunction.java:
##########
@@ -872,6 +886,22 @@ protected byte[][]
transformToBytesValuesSVUsingValueAndNull(ValueBlock valueBlo
return _bytesValuesSV;
}
+ private byte[][] getBytesValues(TransformFunction transformFunction,
ValueBlock valueBlock) {
+ if (_resultMetadata.getDataType() != DataType.UUID || !(transformFunction
instanceof LiteralTransformFunction)) {
Review Comment:
Why does UUID need this special handling, while BOOLEAN and TIMESTAMP don't?
##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionUtils.java:
##########
@@ -184,6 +189,8 @@ public static RelDataType getRelDataType(RelDataTypeFactory
typeFactory, Class<?
case STRING:
case JSON:
return typeFactory.createSqlType(SqlTypeName.VARCHAR);
+ case UUID:
Review Comment:
It is still above BYTES
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/filter/PredicateRowMatcher.java:
##########
@@ -78,6 +79,8 @@ public boolean isMatch(Object[] row) {
return _predicateEvaluator.applySV((String) value);
case BYTES:
return _predicateEvaluator.applySV((byte[]) value);
+ case UUID:
+ return _predicateEvaluator.applySV(UuidUtils.toBytes(value));
Review Comment:
Is `value` always `UUID`?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CastTransformFunction.java:
##########
@@ -300,13 +310,91 @@ public String[] transformToStringValuesSV(ValueBlock
valueBlock) {
byte[][] bytesValues = transformToBytesValuesSV(valueBlock);
ArrayCopyUtils.copy(bytesValues, _stringValuesSV, length);
break;
+ case UUID:
+ byte[][] uuidValues = transformToBytesValuesSV(valueBlock);
+ for (int i = 0; i < length; i++) {
+ _stringValuesSV[i] = UuidUtils.toString(uuidValues[i]);
+ }
Review Comment:
Move this to `ArrayCopyUtils` and call it `copyFromUuid()`. Same for the
other direction
##########
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:
Why do we use this special map?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunction.java:
##########
@@ -822,19 +830,25 @@ protected byte[][]
transformToBytesValuesSVUsingValue(ValueBlock valueBlock) {
return _bytesValuesSV;
}
+ /// The BYTES placeholder is zero-length, which every UUID consumer rejects
("Invalid UUID byte length: 0"). A UUID
+ /// result must fall back to the 16-byte nil UUID instead.
+ private byte[] nullBytesPlaceHolder() {
Review Comment:
IIRC, place holder can be anything, and is never read
##########
pinot-core/src/main/java/org/apache/pinot/core/query/pruner/ValueBasedSegmentPruner.java:
##########
@@ -230,15 +232,30 @@ public void ensureDataType(DataType dt) {
}
public boolean mightBeContained(BloomFilterReader bloomFilter) {
+ // The rendering and hashing below run once per (value, data type):
the resulting hashes are memoized and
+ // every subsequent segment in the query reuses them. Deliberately not
precomputed in ensureDataType, so a
+ // query that only reaches min/max pruning never pays for it.
if (!_hashed) {
GuavaBloomFilterReaderUtils.Hash128AsLongs hash128AsLongs =
-
GuavaBloomFilterReaderUtils.hashAsLongs(_comparableValue.toString());
+ GuavaBloomFilterReaderUtils.hashAsLongs(bloomFilterKey());
_hash1 = hash128AsLongs.getHash1();
_hash2 = hash128AsLongs.getHash2();
_hashed = true;
}
return bloomFilter.mightContain(_hash1, _hash2);
}
+
+ /// Renders the value exactly as `BloomFilterCreator#add(Object, int)`
did when the index was built. If the
+ /// two disagree the lookup silently misses and the segment is wrongly
pruned, dropping matching rows with no
+ /// error. That creator special-cases UUID to the canonical string and
renders everything else with
+ /// `value.toString()` -- which for BYTES is already hex via [ByteArray].
+ ///
+ /// Deliberately NOT routed through `DataType#toString`: that renders
BIG_DECIMAL with
+ /// `toPlainString()`, which the creator does not, so every BIG_DECIMAL
bloom filter would start missing.
+ private String bloomFilterKey() {
Review Comment:
What value do we feed into bloom filter builder? Do we feed canonical value
(hex string)?
##########
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:
Same here
--
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]
