[UUID 4/8] Server-side predicate evaluation for the logical UUID type - #18872
[UUID 4/8] Server-side predicate evaluation for the logical UUID type#18872xiangfu0 wants to merge 1 commit into
Conversation
9a2a841 to
54e9158
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18872 +/- ##
============================================
- Coverage 66.62% 66.60% -0.02%
Complexity 1423 1423
============================================
Files 3443 3443
Lines 218577 218605 +28
Branches 34792 34799 +7
============================================
- Hits 145624 145612 -12
- Misses 61218 61269 +51
+ Partials 11735 11724 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
25e3d0c to
8f7a1d6
Compare
4969a9a to
b501df6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
pinot-common/src/test/java/org/apache/pinot/common/request/context/RequestContextUtilsTest.java:92
- This test temporarily changes the JVM default Locale, which is global mutable state. If tests are executed in parallel (or other tests read Locale concurrently), this can introduce flaky failures outside this class. Consider synchronizing the locale mutation to minimize the window and improve test isolation.
Locale originalDefault = Locale.getDefault();
Locale.setDefault(Locale.forLanguageTag("tr-TR"));
try {
FilterContext filter = compileFilter("uuidCol = CAST('" + UUID_1 + "' AS uuid)");
assertEquals(filter.getType(), FilterContext.Type.PREDICATE);
EqPredicate predicate = (EqPredicate) filter.getPredicate();
assertEquals(predicate.getValue(), UUID_1);
} finally {
Locale.setDefault(originalDefault);
}
c624c5d to
581cbfb
Compare
| /// | ||
| /// 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() { |
There was a problem hiding this comment.
What value do we feed into bloom filter builder? Do we feed canonical value (hex string)?
There was a problem hiding this comment.
Canonical string, not hex — and that asymmetry with BYTES is exactly why this line is not just _comparableValue.toString().
The writer is BloomFilterCreator#add(Object, int) in pinot-segment-spi:
if (getDataType() == FieldSpec.DataType.BYTES) {
add(BytesUtils.toHexString((byte[]) value));
} else if (getDataType() == FieldSpec.DataType.UUID) {
add(uuidToCanonicalString(value)); // canonical dashed form
} else {
add(value.toString());
}So BYTES is indexed as hex and UUID as the canonical string. On the reader side _comparableValue is a ByteArray for both (DataType#convertInternal returns ByteArray for BYTES and UUID), and ByteArray.toString() is toHexString() — correct for BYTES, wrong for UUID. Hence the UUID branch.
Worth flagging what this line must not become: routing it through FieldSpec.DataType#toString looks tidier and handles UUID, but that method renders BIG_DECIMAL with toPlainString() while the creator uses value.toString(). new BigDecimal("1.0E-7") would then be indexed as 1.0E-7 and looked up as 0.00000010, so every BIG_DECIMAL bloom filter starts silently missing and pruning segments that do contain matches. I had it that way at one point; BloomFilterSegmentPrunerTest.testBloomFilterRoundTripsThroughCreatorRendering now builds the filter through the real BloomFilterCreator and covers DOUBLE / BIG_DECIMAL / BYTES / UUID to pin it.
There was a problem hiding this comment.
How is BOOLEAN and TIMESTAMP handled? Should we store the canonical format of BYTES here which is hex?
There was a problem hiding this comment.
BOOLEAN and TIMESTAMP go through the else branch — add(value.toString()) — because by the time the creator sees them they are already their stored types (int / long), so there is no logical-type rendering left to do. Only BYTES and UUID need an explicit branch, because both arrive as byte[] and byte[]#toString is the object identity.
On storing hex for UUID instead: I agree it would be more consistent, and it would let bloomFilterKey() collapse to _comparableValue.toString() since ByteArray#toString() is already hex. The blocker is that it changes the on-disk index. Bloom filters in existing segments were written with the canonical form, so flipping the writer makes every pre-existing UUID bloom filter miss — and a miss here prunes a segment that does contain matches, i.e. silently wrong results rather than an error. It would need a segment-format version gate to be safe.
Given UUID segments only became possible in #18870, the population of affected segments is small right now, so this is the cheapest moment to make that switch if you want it. Happy to do it as its own PR with the version gate; I would rather not fold an index-format change into this one.
There was a problem hiding this comment.
Why UUID is different from BOOLEAN and TIMESTAMP? Shouldn't it already be converted into ByteArray?
| case BYTES: | ||
| return _predicateEvaluator.applySV((byte[]) value); | ||
| case UUID: | ||
| return _predicateEvaluator.applySV(UuidUtils.toBytes(value)); |
There was a problem hiding this comment.
I could not establish that it is, which is why the polymorphic helper is there rather than a cast.
UuidUtils.toBytes(Object) accepts java.util.UUID, String, byte[] and ByteArray. The value here comes from ColumnValueExtractor.extract, which is just row[_index] off a reduce-path row, and PredicateRowMatcher is reached from HavingFilterHandler and GapfillFilterHandler. Depending on how the row was produced, a UUID column can be the internal ByteArray (what ColumnDataType.UUID#toInternal yields) or the external java.util.UUID (toExternal), so I did not want to commit to (UUID) value without being able to show it always holds.
If you know the reduce path normalises to one of these before it reaches the matcher, I will make it a direct cast to match the other branches. I would also note there is no test exercising a UUID column through HAVING, so this line is currently unverified either way — worth adding once the expected type is settled.
There was a problem hiding this comment.
This is inconsistent. The input value should be deterministic. All other types only pass single type input
ba8e60d to
050d032
Compare
| case BYTES: { | ||
| // UUID and BYTES differ only in how the literal is parsed -- canonical UUID text vs hex. The stored form, | ||
| // and therefore the lookup below, is identical. | ||
| boolean isUuid = _mainFunction.getResultMetadata().getDataType() == DataType.UUID; |
There was a problem hiding this comment.
Why is there no special handling on BOOLEAN and TIMESTAMP?
There was a problem hiding this comment.
Because the switch is on getStoredType(), and UUID is the only logical type that collides with another type there.
BOOLEAN reduces to INT and TIMESTAMP to LONG, and each is the only logical type sharing that stored type, so case INT / case LONG can parse unconditionally. UUID reduces to BYTES — which BYTES itself already occupies — and the two spell their literals differently (canonical dashed text vs hex). So case BYTES cannot pick a parser from the stored type alone; it has to ask whether the column is UUID.
One caveat I do not want to overstate: case INT parses with Integer.parseInt, which would throw on a true / false literal, since LiteralContext#getStringValue() renders a BOOLEAN literal via PinotDataType.BOOLEAN.toString. The predicate-side equivalent, BaseInPredicate#getBooleanValues(), goes through BooleanUtils.toInt and does handle it. So BOOLEAN here looks like a pre-existing gap rather than evidence that no handling is needed — I have not confirmed it end to end and it is outside this PR, but worth a look.
There was a problem hiding this comment.
I feel this inconsistency (canonical dash and hex) is a wiring bug. There should be a single canonical format reaching here
80120dc to
ce1d3d0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.
Suppressed comments (5)
pinot-spi/src/main/java/org/apache/pinot/spi/utils/ArrayCopyUtils.java:1
- These are public utility methods but the new comments use
///and Markdown-style references (e.g.[#copyFromUuid(...)]) which won’t be picked up by Javadoc tooling and won’t resolve as links. Prefer standard Javadoc (/** ... */) and{@link ArrayCopyUtils#copyFromUuid(...)}so IDEs and generated docs correctly hyperlink and surface the documentation.
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunction.java:854 - For UUID-typed CASE with a bare STRING literal branch, this allocates a fresh
byte[][]for every block evaluation. That can add noticeable GC pressure in tight loops. Consider reusing an existing per-instance buffer (e.g., initialize_bytesValuesSVand fill it) or caching the parsed UUID bytes inside theLiteralTransformFunction/CASE branch metadata so repeated calls don’t allocate a new array each time.
private byte[][] getBytesValues(TransformFunction transformFunction, ValueBlock valueBlock) {
if (_resultMetadata.getDataType() != DataType.UUID || !(transformFunction instanceof LiteralTransformFunction)) {
return transformFunction.transformToBytesValuesSV(valueBlock);
}
LiteralTransformFunction literalTransformFunction = (LiteralTransformFunction) transformFunction;
if (literalTransformFunction.isNull()
|| literalTransformFunction.getResultMetadata().getDataType() != DataType.STRING) {
return transformFunction.transformToBytesValuesSV(valueBlock);
}
int numDocs = valueBlock.getNumDocs();
byte[][] bytesValues = new byte[numDocs][];
byte[] uuidBytes = UuidUtils.toBytes(literalTransformFunction.getStringLiteral());
Arrays.fill(bytesValues, uuidBytes);
return bytesValues;
}
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CastTransformFunction.java:107
- These are user-facing query validation failures, but
Preconditions.checkState(...)throwsIllegalStateException, which can surface as an internal error depending on where it’s caught/wrapped. For query-type errors, prefer throwingBadQueryRequestException(or the project’s standard query validation exception) to keep error classification consistent and to avoid treating invalid queries as server faults.
case "UUID":
Preconditions.checkState(sourceSV, "Cannot cast from MV to UUID");
_resultMetadata = UUID_SV_NO_DICTIONARY_METADATA;
break;
case "UUID_ARRAY":
Preconditions.checkState(!sourceSV, "Cannot cast from SV to UUID_ARRAY");
_resultMetadata = UUID_MV_NO_DICTIONARY_METADATA;
break;
pinot-common/src/test/java/org/apache/pinot/common/request/context/RequestContextUtilsTest.java:103
- This test mutates the JVM-wide default
Locale, which can cause flaky failures if tests run in parallel (other tests can observe the modified default during this window). To make it robust, consider marking the test/class as single-threaded/non-parallel in TestNG, or restructuring the code under test to accept an explicitLocaleso the test doesn’t need to change global process state.
Locale originalDefault = Locale.getDefault();
Locale.setDefault(Locale.forLanguageTag("tr-TR"));
try {
FilterContext filter = compileFilter("uuidCol = CAST('" + UUID_1 + "' AS uuid)");
assertEquals(filter.getType(), FilterContext.Type.PREDICATE);
EqPredicate predicate = (EqPredicate) filter.getPredicate();
assertEquals(predicate.getValue(), UUID_1_STORED);
} finally {
Locale.setDefault(originalDefault);
}
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunction.java:223
- The newly added UUID literal validation drops the underlying exception cause, which can make debugging malformed literals harder (e.g., distinguishing parse failures vs length issues). Consider chaining the caught exception as the cause (e.g.,
new IllegalArgumentException(..., e)) so logs/error handlers retain the original failure details.
case UUID:
try {
UuidUtils.toBytes(literal);
} catch (Exception e) {
throw new IllegalArgumentException("Invalid literal: " + literal + " for UUID");
}
break;
| return _bytesValuesSV; | ||
| } | ||
|
|
||
|
|
| /// A `CAST(... AS UUID)` branch does NOT come through here: it is folded to a BINARY literal carrying the 16 | ||
| /// stored bytes (see `RequestUtils#getLiteral(Object)`), so it takes the normal path below. BOOLEAN and TIMESTAMP | ||
| /// need no equivalent because their literals keep their own type and convert themselves. | ||
| private byte[][] getBytesValues(TransformFunction transformFunction, ValueBlock valueBlock) { |
There was a problem hiding this comment.
Do you mean BOOLEAN and TIMESTAMP literal can be directly identified as their own type? What is the difference for UUID?
| case BYTES: { | ||
| // UUID and BYTES differ only in how the literal is parsed -- canonical UUID text vs hex. The stored form, | ||
| // and therefore the lookup below, is identical. | ||
| boolean isUuid = _mainFunction.getResultMetadata().getDataType() == DataType.UUID; |
There was a problem hiding this comment.
I feel this inconsistency (canonical dash and hex) is a wiring bug. There should be a single canonical format reaching here
| /// | ||
| /// 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() { |
There was a problem hiding this comment.
Why UUID is different from BOOLEAN and TIMESTAMP? Shouldn't it already be converted into ByteArray?
| case BYTES: | ||
| return _predicateEvaluator.applySV((byte[]) value); | ||
| case UUID: | ||
| return _predicateEvaluator.applySV(UuidUtils.toBytes(value)); |
61adb41 to
e3e70bd
Compare
Adds UUID handling to the predicate evaluators, so =, !=, IN, NOT IN and range predicates work against a UUID column on both the raw and the dictionary path. UUID follows the pattern TIMESTAMP already uses: a logical type whose stored type does the work. The literal is parsed to its 16-byte stored form once, when the evaluator is built, and from there the existing BYTES evaluators apply -- no per-value conversion in the scan loop. The dictionary path needs no UUID branch: Dictionary#getStoredValue returns hex for a UUID column and indexOf(String) hex-decodes, so the existing String-keyed lookup is already correct. PredicateUtils renders the literal to that hex form for those String-typed lookup APIs. Split into apache#19181 (CAST), apache#19182 (bloom filter pruning) and apache#19183 (transform functions); this PR is now just the predicate evaluators.
What
UUID handling in the predicate evaluators, so
=,!=,IN,NOT INand range predicates work against a UUID column on both the raw and the dictionary path.Approach
UUID follows the pattern
TIMESTAMPalready uses: a logical type whose stored type does the work. The literal is parsed to its 16-byte stored form once, when the evaluator is built, and from there the existing BYTES evaluators apply unchanged. There is no per-value conversion in the scan loop.Why the dictionary path needs no UUID branch
Dictionary#getStoredValuereturns hex for a UUID column, andindexOf(String)hex-decodes — so the existing String-keyed lookup is already correct, and an added UUID branch would be dead code.PredicateUtilsrenders the literal into that hex form, which is what those String-typed lookup APIs consume. (This is transport encoding at an API boundary, not the storage format — storage is the raw 16 bytes.)Testing
UuidDictionaryPredicateEvaluatorTestcovers the dictionary path across all five predicate kinds. The threeNoDictionary*PredicateEvaluatorTestclasses cover the raw path. Between them: canonical, dashless and mixed-case input forms, and rejection of malformed literals.About this PR
Part of the #18140 UUID split. Depends on
UuidUtils(#18869) and the UUID stored type (#18870), both on master.