Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #19311 +/- ##
=============================================
- Coverage 39.94% 0.00% -39.95%
=============================================
Files 3520 3 -3517
Lines 228940 6 -228934
Branches 36313 0 -36313
=============================================
- Hits 91453 0 -91453
+ Misses 129215 6 -129209
+ Partials 8272 0 -8272
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:
|
xiangfu0
added
feature
ededa2b to
3435400
Compare
7ebcfdc to
5f27d70
Compare
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Review of current head 5f27d70c.
The correctness, rolling-upgrade, and resource-behavior issues below should be addressed before merge. I did not duplicate the two existing unresolved threads about v2/lite rejection and _closedPartitionKeys cardinality.
Process follow-ups:
- This is a 56-file, 7.5K-line change spanning protobuf, planning, execution, configuration, and integration. Please either link the reviewed design/maintainer agreement for keeping it as one vertical slice, or split it into reviewable stacked PRs.
- Please link the sender-side-sorting TODO to a tracking issue.
- Both commits contain AI
Co-Authored-Bytrailers; repository guidance says to omit those, so please rewrite the commit messages before merge.
9623e5c to
ccb8dea
Compare
xiangfu0
added
design-review
ccb8dea to
b612fab
Compare
|
Addressed the process follow-ups on head
Local validation passed: the 342-test focused suite, a 32-test matcher/operator rerun after the final retained-state changes, 13/13 two-server integration cases, the full 63-module |
e25bf6f to
c05a54b
Compare
|
The rebased PR head is now Coverage added in this follow-up:
The comparison remains pinned to Trino Validation:
Fresh hosted CI is complete: 12/12 checks passed for |
6575e4c to
3034c6d
Compare
b6ed53c to
d252451
Compare
Adds row pattern recognition to the multi-stage query engine, following the same shape as the UNNEST support added in apache#17168: a new plan node, an exchange-insertion rule, and an intermediate-stage operator. Pinot's parser already accepted the full MATCH_RECOGNIZE grammar (Parser.jj carries Calcite's Babel production). What was missing was operator-table registration, validation, a plan node, and all of execution. Front end - Register PREV/NEXT/FIRST/LAST/CLASSIFIER/MATCH_NUMBER/RUNNING/FINAL in PinotOperatorTable, which is a strict allow-list. - New MatchRecognizeValidator runs on the SqlNode tree before conversion. It rewrites an OMITTED AFTER MATCH clause to SKIP PAST LAST ROW: Calcite substitutes SKIP TO NEXT ROW, but SQL:2016, Trino, Snowflake and Oracle all default to SKIP PAST LAST ROW. The two differ in whether matches overlap, so a query ported from another engine would otherwise silently return different rows. Conversion erases the omitted-vs-explicit distinction, so the rewrite has to happen here. - Deferred constructs are rejected at planning time with actionable messages rather than silently mis-executing: ALL ROWS PER MATCH, SUBSET, PERMUTE, pattern exclusions, WITHIN, aggregates in DEFINE, NULLS FIRST/LAST, and ORDER BY / PARTITION BY on expressions (the last of which otherwise fail inside SqlToRelConverter with AssertionError or ClassCastException). Plan and wire format - MatchNode at plan.proto tag 19, encoding the pattern as a self-contained recursive RowPattern with a symbol table rather than a RexCall tree, with field numbers reserved for SUBSET and WITHIN. - PatternFieldRef in expressions.proto, so RexPatternFieldRef can no longer degrade to a plain InputRef and produce wrong-but-type-correct results. - PinotMatchExchangeNodeInsertRule hash-distributes on the PARTITION BY keys and prepends them to the sort collation, so rows arrive clustered and the operator can match and flush one partition at a time. A missing PARTITION BY is rejected by default, since it collapses the table onto one worker. Runtime - PatternToNfaCompiler builds an NFA with prioritized transitions (alternation in source order; greedy takes the loop edge first, reluctant the exit edge; {n,m} via counter registers rather than state unrolling), so depth-first traversal yields the SQL:2016 preferred match first. - MatchOperator evaluates DEFINE predicates over a classifier tape supporting PREV/NEXT/FIRST/LAST/CLASSIFIER/MATCH_NUMBER, emits MEASURES, and advances per the AFTER MATCH SKIP mode. - Guardrails throw rather than truncate, since a truncated pattern result is a silently wrong one: maxRowsInMatch, maxStepsPerMatchAttempt, and an empty-cycle guard. v1 covers PARTITION BY, mandatory ORDER BY, MEASURES, ONE ROW PER MATCH, all four AFTER MATCH SKIP modes, the full pattern algebra including reluctant quantifiers and anchors, and single-variable aggregates in MEASURES. MATCH_RECOGNIZE is not yet supported under the v2 physical optimizer or lite mode; queries there are covered by ignore flags rather than silently wrong results. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Upstream now enforces `///` markdown doc comments (JEP 467) over `/** */`
Javadoc via a checkstyle RegexpCheck, and this feature branch predates that
rule. Converts all 168 Javadoc blocks across the 23 MATCH_RECOGNIZE files, and
removes an `org.apache.calcite.sql.SqlLiteral` import that the feature commit
added without ever using (UnusedImports flags it independently).
Formatting only: `{@link X}` becomes `[X]`, `{@link X label}` becomes
`[label][X]`, `{@code x}` becomes a backtick span, `<p>` becomes an empty ///
line, and the remaining HTML becomes its markdown equivalent. No documentation
text was reworded or dropped, and the removed import is the only non-comment
line changed.
Verified: checkstyle reports 0 violations in each of pinot-spi, pinot-common,
pinot-query-planner, pinot-query-runtime and pinot-integration-tests (run
per-module, since a combined reactor stops at the first failure and hides the
rest); `javadoc -Xdoclint:reference,syntax` is clean over the changed sources;
pinot-query-planner 1598 tests and pinot-query-runtime 4611 tests still pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d252451 to
80f2387
Compare
Harden planner validation, configuration precedence, wire compatibility, partition ordering, matcher resource bounds, cancellation, and output buffering. Add focused and two-server integration coverage for null semantics, global execution, legacy plans, numeric precision, and retained matcher state. (cherry picked from commit 6aa6d26)
(cherry picked from commit b6ed53c)
|
Recovered the two reviewed MATCH hardening/configuration commits that had been dropped from this branch, and integrated current master Current upstream also needs a direct managed, provided JetBrains annotations dependency for segment-local compilation: zstd-jni's provided annotation dependency is not transitive. This changes compile-time dependency declaration only. Validation on this source: 726 targeted tests and all 21 real two-server MATCH integration cases passed with zero failures, errors, or skips. The affected-module/root style and license checks and independent source/artifact review passed. Fresh hosted CI also passed on this exact head: both unit suites, both integration suites, linter, Quickstart, binary compatibility, all four compatibility regressions, Java 11 client compatibility, Docker and Trivy. The PR description records these current results separately from historical results. |
2 participants
PR flow
Adds MATCH_RECOGNIZE validation, planning, exchange insertion, serialization, deserialization, and runtime execution with limits and tape-based matching.
AI-generated · Green: added · Yellow: modified · Red: removed · Gray: existing
Partial evidence: 29 file patches omitted; 2 truncated.
Diff evidence
Adds SQL:2016 row pattern recognition to Pinot's multi-stage query engine: validation, logical/wire plans, exchange planning, and an intermediate-stage matcher.
Pinot's parser already accepts the
MATCH_RECOGNIZEgrammar through Calcite's Babel parser. This PR adds the missing supported-subset validation and execution path.Design and delivery boundary
The parser validation,
MatchNodewire representation, exchange rule, runtime operator, and expression semantics form one end-to-end query operator and must land atomically. Splitting those layers would either expose syntax that cannot execute or dispatch plans that servers cannot interpret. Existing non-MATCH_RECOGNIZEqueries keep their existing plan and execution behavior; unsupportedMATCH_RECOGNIZEmodes fail closed with actionable validation errors. The PR remains labeleddesign-reviewfor maintainer sign-off before landing.Rollback is likewise atomic: revert the feature commits and stop issuing
MATCH_RECOGNIZEqueries. There is no persisted-data or table-schema migration.Front end and planner
PREV/NEXT/FIRST/LAST/CLASSIFIER/MATCH_NUMBER/RUNNING/FINALin the strictPinotOperatorTableallow-list.MatchRecognizeValidator, including SQL:2016's omittedAFTER MATCHdefault (SKIP PAST LAST ROW) and actionable rejection of deferred constructs such asALL ROWS PER MATCH,SUBSET,PERMUTE, exclusions,WITHIN, aggregates inDEFINE, explicit null ordering, expression partition/order keys, and multi-value partition or aggregate inputs.RUNNINGinDEFINEas the clause's intrinsic mode and rejects unsupportedFINALsemantics there.CLASSIFIER()as nullable so Calcite preservesCOUNT(classifier_measure)instead of incorrectly rewriting it toCOUNT(*)for empty matches.MatchNodeand a self-contained recursiveRowPattern;PatternFieldRefpreserves pattern-variable identity instead of degrading to a plain input reference.PARTITION BY, then sorts by the exchange's actual partition-key order plus the requested order keys. Queries withoutPARTITION BYrequire the explicitallowMatchRecognizeWithoutPartitionByquery option and execute as one global partition.SET usePhysicalOptimizer = falseselects the supported path.Runtime and resource safety
DEFINEpredicates andMEASURESover a classifier tape, including navigation aroundCLASSIFIER(), bounded navigation, and exact decimal/integer aggregation behavior.Resource-limit precedence is node hint, query option, server config, then default:
maxRowsInMatchPartitionmax_rows_in_match_partitionpinot.query.match.max.rows.per.partitionmaxStepsPerMatchAttemptmax_steps_per_match_attemptpinot.query.match.max.steps.per.attemptScope
v1 covers
PARTITION BY, mandatoryORDER BY,MEASURES,ONE ROW PER MATCH, all four supportedAFTER MATCH SKIPmodes, pattern alternation/concatenation/quantifiers/anchors, navigation and classifier functions, and single-variable aggregates in measures.Trino comparison
The supported v1 behavior and tests were compared against Trino at commit
19b4cebac145, especially its row-pattern query suite, aggregation suite, analyzer suite, and feature documentation.Coverage added from that comparison includes:
CLASSIFIER()nullability, andCOUNT(classifier_measure)versusCOUNT(*)semantics;FIRST/LAST/PREV/NEXTnavigation aroundCLASSIFIER(), including out-of-match nulls and composition of non-zero logical and physical offsets;DEFINEexpressions and rejection ofFINALinDEFINE;DESC/ASCkeys.This comparison targets Pinot's explicitly supported v1 subset and does not claim full Trino feature parity.
ALL ROWS PER MATCH,SUBSET,PERMUTE, exclusions,WITHIN, aggregates inDEFINE, optional measures/order, and row-pattern window frames remain deferred and fail validation where applicable.Rolling-upgrade boundary
MatchNodeis plan oneof field 19. Older servers preserve that protobuf field as unknown but see the node oneof as unset (NODE_NOT_SET), so they cannot execute a dispatchedMATCH_RECOGNIZEstage. Upgrade all servers before issuing these queries; there is no mixed-version fallback. Existing query node kinds remain wire-compatible and are unaffected.Testing
35692e0aad54, 726 targeted tests across 20 common, planner, runtime, segment, upsert, and compression classes passed with zero failures, errors, or skips. This includes the restored query-option validation, MATCH expression/limit tests, plan merge/equivalence tests, and the current upstream string-function behavior.MatchRecognizeIntegrationTest: 21/21 passed against a real two-server cluster using the coherently compiled and installed artifacts from this source.git diff --checkpassed for the affected modules and root on JDK 25.35692e0aad54: both unit suites, both integration suites, linter, Quickstart, binary compatibility, all four compatibility regressions, Java 11 client compatibility, Docker and Trivy. Earlier results from80f23878ef6dremain historical.