Repository navigation
Make IDSET and IN_ID_SET work in the multi-stage engine - #19757
Conversation
yashmayya
added
multi-stage
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19757 +/- ##
============================================
+ Coverage 68.33% 68.39% +0.06%
Complexity 1470 1470
============================================
Files 3520 3522 +2
Lines 228960 229102 +142
Branches 36308 36361 +53
============================================
+ Hits 156459 156698 +239
+ Misses 60232 60136 -96
+ Partials 12269 12268 -1
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:
|
gortiz
left a comment
gortiz
left a comment
There was a problem hiding this comment.
LGTM, thanks for the thorough tests.
One suggestion for IdSets.toInputStream: instead of Jackson's ByteBufferBackedInputStream, we can use our own stream classes from pinot-segment-spi (org.apache.pinot.segment.spi.memory). pinot-core already has them on the classpath. Something like this would do:
static InputStream toInputStream(ByteBuffer byteBuffer) {
return new DataBufferPinotInputStream(PinotByteBuffer.slice(byteBuffer));
}PinotByteBuffer.slice wraps any ByteBuffer (heap, direct or read-only) without moving the caller's position, so the hasArray() branch goes away and every buffer type takes the same path. DataBufferPinotInputStream supports bulk reads and also implements DataInput, so Roaring64NavigableMapIdSet could pass it straight to deserialize(DataInput) without the extra DataInputStream. It is also the same buffer abstraction the multi-stage engine already uses for mailbox payloads (DataBlockUtils and BaseDataBlock wrap them with PinotByteBuffer), and it keeps a JSON library out of plain IO code. The new IdSetTest cases for heap, read-only, direct and offset buffers would cover the change.
Not a blocker, so approving as is.
| /// instead, as before this function existed. The flag also marks the function VOLATILE, so ingestion transforms | ||
| /// cannot use it. | ||
| @Nullable | ||
| @ScalarFunction(nullableParameters = true, isDeterministic = false) |
There was a problem hiding this comment.
isDeterministic = false works around literal typing (BigDecimal for an INT literal), but it also marks the function volatile. That is harmless for planning today only because PinotOperatorTable registers TransformFunctionType.IN_ID_SET before the scalar functions, so Calcite sees the transform's operator, which is deterministic.
If that operator ever goes away, the planner picks this scalar's PinotSqlFunction instead. Then PinotRuleUtils.isRelocatable() returns false and PinotFilterJoinRule stops pushing IN_ID_SET below joins. Results stay correct, but the leaf transform path is lost silently.
Could we add a comment here that names this coupling, and a plan test (EXPLAIN) that pins a filter over a join down to the leaf? Another option is to map BigDecimal literals to the set's type in getStoredType and keep the function deterministic.
There was a problem hiding this comment.
Done in 021374b. The comment is in IdSetFunctions and at TransformFunctionType.IN_ID_SET, and new cases in JoinPlans.json and PhysicalOptimizerPlans.json pin the push-down. When I made the IN_ID_SET operator volatile, both cases failed, with the filter above the join.
I kept isDeterministic = false instead of mapping BigDecimal to the set's type. A Bloom set does not record whether it was built from INT or FLOAT values (or LONG or DOUBLE), so the mapping could look up the wrong bits. The root cause is that PinotEvaluateLiteralRule passes raw Calcite values (BigDecimal for every exact number) to Object parameters, where the runtime passes Integer or Long. From the code, this also changes fold-time results of other Object-parameter functions, such as toThetaSketch. So I think the fix belongs in the rule, in a separate PR.
| @Nullable | ||
| @Override | ||
| public RoaringBitmap getNullBitmap(ValueBlock valueBlock) { | ||
| return _idSet.getType() == IdSet.Type.EMPTY ? null : super.getNullBitmap(valueBlock); |
There was a problem hiding this comment.
This changes existing results. With null handling on, IN_ID_SET(nullableCol, <empty set>) used to be NULL for NULL rows and is now FALSE, so = 0 or NOT IN_ID_SET(...) now keeps those rows. The single-stage IN_SUBQUERY changes too when the subquery matches no rows.
The PR description explains it, but I think it needs the backward-incompat label and a release note, plus a line in the IN_ID_SET / IN_SUBQUERY docs. Also, only the multi-stage leaf case in IdSet.json covers this today. A single-stage case in IdSetQueriesTest (empty set, nullable column, null handling on) would lock it in for that engine as well.
There was a problem hiding this comment.
Done in 021374b. I added the backward-incompat label, a release note in the description, and IdSetQueriesTest.testInIdSetNullHandling (single-stage, nullable column, null handling on, empty and non-empty sets). The docs line needs a separate PR in the docs repo.
One note on IN_SUBQUERY: its subquery takes null handling from the broker config or the request options, not from the outer query's SET. If the subquery runs with null handling on and matches no rows, IDSET returns NULL, and the rewrite already fails with a NullPointerException today. I listed that under Known limits.
yashmayya
added
the
backward-incompat
|
@gortiz Thanks! Done in 021374b. IdSets now read through One addition: Bloom filter reads go through a |
Docs follow-up for apache/pinot#19757. Corrects the single-stage-only guidance, describes multi-stage IDSET/IN_ID_SET support, empty-set null behavior, and the current subquery-form limit.\n\nValidation: full docs validator passed (existing non-fatal anchor warnings only); git diff --check passed. Co-authored-by: Xiang Fu <xiangfu@Xiang-mac-mtv-2.local>
|
Docs follow-up: pinot-contrib/pinot-docs#1088 (merged). The IdSet guide now covers multi-stage IDSET/IN_ID_SET support, empty-set null behavior, and current subquery-form limits. |
4 participants
PR flow
Flow for IN_ID_SET in MSE non-leaf stages: scalar function deserializes, validates, checks containment.
AI-generated · Green: added · Yellow: modified · Red: removed · Gray: existing
Diff evidence
Problem
IDSETandIN_ID_SETonly partly work in the multi-stage engine (MSE):IDSETworks on INT columns only. Every other type, and every Bloom filter set, fails withCannot deserialize BloomFilter from ByteBuffer not backed by an accessible byte array(or the same error forRoaring64NavigableMap). MSE hands intermediate results over in read-only buffers, which have no accessible array.IN_ID_SETworks only when the planner pushes it into a leaf stage. In a filter over a join, inHAVINGor in a join condition it fails withUnsupported function: INIDSET, because it has no scalar implementation.ClassCastException.Changes
BloomFilterIdSetandRoaring64NavigableMapIdSetread from anyByteBuffer, through Pinot'sDataBufferPinotInputStreamoverPinotByteBuffer.slice. Heap, direct and read-only buffers take the same path, andRoaring64NavigableMapreads the stream as aDataInputdirectly. Bloom filter reads go through aBufferedInputStream, because Guava reads 8 bytes at a time.inIdSetscalar function, for MSE stages that are not leaf stages. Leaf stages keepInIdSetTransformFunction, because transform functions take precedence. Each call site gets its own instance, which caches the deserialized set.IdSets.validateValueTypechecks the value type against the set once, in both the transform function and the scalar function. A mismatch now fails with, for example,Cannot look up LONG values in an IdSet built from INT values.IN_ID_SETis now typed as nullable in the planner, because it returns NULL for a NULL value. With the old NOT NULL type, Calcite removedIS NOT TRUEandCOALESCEaround it, which gave wrong results over outer joins.INover an empty subquery does.IDSETreturns NULL over zero rows when null handling is on. This changes existing results in both engines: with null handling on,IN_ID_SET(nullableCol, <empty set>) = 0(orNOT IN_ID_SET(...)) now keeps the NULL rows, asx NOT IN (empty subquery)does. This includes the single-stageIN_SUBQUERYwhen its subquery returns an empty set.isDeterministic = false). The planner passes literals in types the lookup cannot use, such as BigDecimal for an INT literal, so an all-literal call is left to the runtime, as before. The planner still pushesIN_ID_SETbelow joins, because it uses the deterministic operator of theIN_ID_SETtransform function. A plan test pins this.IN_ID_SETafter aggregation, for example inHAVING. Aggregations such asMAXreturn DOUBLE there, so they must be cast to the type the set was built from.No change to the serialized format or the wire protocol.
Release notes
Behavior change, with null handling on:
IN_ID_SET(col, <empty IdSet>)is now FALSE for a NULLcol, not NULL, ascol IN (<empty subquery>)is. SoIN_ID_SET(col, <empty IdSet>) = 0andNOT IN_ID_SET(...)now keep the rows wherecolis NULL. This applies to both engines, and to single-stageIN_SUBQUERYwhen its subquery returns an empty IdSet. With a non-empty IdSet, a NULL value still gives NULL.Known limits
IDSETsubquery, which has one set per group) deserializes the set on every change.IN_SUBQUERYfails with aNullPointerExceptionwhen its subquery runs with null handling on and matches no rows.IDSETreturns NULL there, and the broker cannot turn NULL into the set literal. This is not new.Next phases
This PR is phase 1 of 3, plus an optional phase 4. Each phase is a separate PR.
QueryServerandDispatchClienthardcode 64 MB, and the broker gRPC endpoint uses the gRPC default of 4 MB.IN_SUBQUERY(expr, 'sql')and uncorrelatedIN_ID_SET(expr, (SELECT IDSET(...) ...)). The broker runs the subquery first, so the set never goes through the parser or the client.IDSET, and shorter literals in EXPLAIN (Explain plan should elide function parameters #8009).Tests
IdSetTest: every set type from heap, read-only, direct and offset buffers, and the value type matrix.IdSetFunctionsTest: every value type (including BOOLEAN, TIMESTAMP and UUID, through their stored types), NULL and empty sets, malformed sets, cache reuse and registration.IdSetQueriesTest:IN_ID_SETon every single-value type in a leaf, inHAVING, the mismatch error, and NULL values with empty and non-empty sets under null handling.JoinPlans.jsonandPhysicalOptimizerPlans.json: anIN_ID_SETfilter on one side of a join is pushed into the leaf stage, in both planners.IdSet.json(MSE, both planners):IDSETon every type, as a scalar subquery, a CTE and a correlated subquery.IN_ID_SETin leaf filters, post-join filters,HAVINGand join conditions.IDSETresults compared byte for byte with sets built directly from the values.IdSetIntegrationTest(2 servers): both engines return the sameIDSETbytes, and a set built by either engine filters the same rows in both.Without the main changes, 56 of the first 66
IdSet.jsoncases fail.