nielspardon opened a new pull request, #12728: URL: https://github.com/apache/gluten/pull/12728
## What changes are proposed in this pull request? Substrait 0.98 redesigned `TopNRel`: the scalar `int64 n = 3` became `Expression count = 5`, `sorts` moved to field 3, and it gained `Expression offset = 4` and `FetchMode mode = 6`. This migrates Gluten's vendored proto, the JVM producer, and the Velox consumer/validator in lockstep, as one step of the Substrait `v0.23.0` → `0.98.0` proto rebase (#12597). Gluten's only `TopNRel` producer (`TopNTransformer`, Spark's `TakeOrderedAndProject`) always supplies a literal `Long` limit with no offset and no ties, so the producer wraps the limit into an i64-literal `Expression` and sets `FETCH_MODE_ROWS_ONLY` explicitly (`FETCH_MODE_UNSPECIFIED` is merely the proto3 default and not a valid producer choice). `TopNRel` stays at Rel-oneof field 19, so there is no oneof renumbering. - **Proto:** replace `int64 n = 3` / `sorts = 4` / `advanced_extension = 10` with the 0.98 body (`sorts = 3`, `offset = 4`, `count = 5`, `mode = 6`, `advanced_extension = 7`) and add the top-level `FetchMode` enum (vendored verbatim from 0.98). - **Producer:** `TopNNode` wraps the limit via `ExpressionBuilder.makeLongLiteral`, sets `FETCH_MODE_ROWS_ONLY`, and now attaches `RelCommon` (previously it was built but never set). - **Velox:** `SubstraitToVeloxPlan` and `SubstraitToVeloxPlanValidator` extract the count via a new `SubstraitParser::getRowCount` helper — which rejects a count that is not a positive i64 literal in `[1, INT32_MAX]` — and reject an `offset` or any `mode` other than `FETCH_MODE_ROWS_ONLY`. The converter mirrors the validator so a plan reaching it with native validation disabled (or via the JSON-plan/benchmark paths) fails loudly instead of silently dropping OFFSET / WITH TIES semantics. The validator also rejects an empty sort list, which `core::TopNNode` asserts against. ClickHouse has no `TopNRel` parser, and the Velox → Substrait reverse converter maps a Velox `TopNNode` to a `FetchRel` (not a `TopNRel`), so neither is affected. ## How was this patch tested? - New `TopNRelProtoSuite` pins the producer contract (the count lands as an i64-literal expression, `FETCH_MODE_ROWS_ONLY`, no offset, `RelCommon` direct) and asserts the 0.98 field numbers on the descriptor (a same-schema round trip cannot catch a renumber). - Locally verified: `protoc` dup-field check; `gluten-substrait` builds and the new suite passes (`mvn -Pspark-3.5`); the Velox native backend compiles and links (consumer, validator, and the new `getRowCount` helper); `clang-format` (v15) clean on all changed C++ files. - Existing end-to-end `TakeOrderedAndProject` suites exercise the consumer extraction on Velox. ## Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 4.8) 🤖 Generated with AI -- 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]
