[GLUTEN-12597][CORE] Migrate WriteRel to Substrait 0.98 (relocate bucket_spec; add create_mode/advanced_extension) - #12746
Open
nielspardon wants to merge 2 commits into
Open
Conversation
…ket_spec; add create_mode/advanced_extension) Substrait 0.98 extended WriteRel: field 7 became `RelCommon common`, and it gained `CreateMode create_mode = 8` and `substrait.extensions.AdvancedExtension advanced_extension = 9`, while the `OutputMode` value `OUTPUT_MODE_MODIFIED_TUPLES` was renamed to `OUTPUT_MODE_MODIFIED_RECORDS`. Gluten's local `bucket_spec` graft (PR apache#8386) sat on field 7, exactly where 0.98 puts `common`. This vendors the 0.98 WriteRel body verbatim and relocates the graft off the collision, adopting a "Gluten-local fields start at 1000" convention that keeps grafts clear of the range upstream allocates from, so a future upstream field cannot collide with them again. The enclosing `Rel.write` oneof tag stays at 18; reconciling the whole `Rel` oneof to upstream's numbers is a separate change. All producer/consumer accessors are name-based (`setBucketSpec`, `has_common`), so the field renumber needs no source change. The one source edit is a guard on the Velox consumer. - Proto: add `common=7`, `create_mode=8`, `advanced_extension=9` and the `CreateMode` enum (vendored verbatim from 0.98), rename the `OutputMode` value to `OUTPUT_MODE_MODIFIED_RECORDS`, and relocate `BucketSpec bucket_spec` from field 7 to 1000 (nested `BucketSpec` message unchanged). - Velox: reject a `common.emit` mapping in the WriteRel handler, which the writer does not apply, mirroring the existing guard on other rels. - Docs: record the WriteRel rebase and the 1000+ numbering convention in SubstraitModifications.md, noting that `WriteRel.common` uses Gluten's pre-0.98 `RelCommon` copy. - Add WriteRelProtoSuite pinning the producer contract, the 0.98 field numbers (including `bucket_spec=1000`), and the `OutputMode` rename (a renumber or rename is invisible to a round trip through the shared schema). Part of apache#12597 Generated-by: Claude Code (Claude Opus 4.8)
|
Run Gluten Clickhouse CI on x86 |
19 tasks
nielspardon
marked this pull request as ready for review
August 11, 2026 06:01
JkSelf
reviewed
Aug 12, 2026
WriteRelNode never sets WriteRel.common, so has_common() is always false for Gluten-produced plans and the guard is dead code. Remove it per review; it can be re-introduced if a common producer path is added later. Part of apache#12597 Generated-by: Claude Code (Claude Opus 4.8)
|
Run Gluten Clickhouse CI on x86 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
Substrait 0.98 extended
WriteRel: field 7 becameRelCommon common, and it gainedCreateMode create_mode = 8andsubstrait.extensions.AdvancedExtension advanced_extension = 9, while theOutputModevalueOUTPUT_MODE_MODIFIED_TUPLESwas renamed toOUTPUT_MODE_MODIFIED_RECORDS. Gluten's localbucket_specgraft (#8386) sat on field 7 — exactly where 0.98 putscommon. This migrates Gluten's vendoredWriteRelto the 0.98 body and relocates the graft off the collision, as one step of the Substraitv0.23.0→0.98.0proto rebase (#12597).The graft moves to field 1000, adopting a "Gluten-local fields start at 1000" convention that keeps grafts clear of the range upstream allocates from, so a future upstream field cannot collide with them again. The enclosing
Rel.writeoneof tag stays at 18; reconciling the wholeReloneof to upstream's numbers is a separate follow-up. All producer/consumer accessors are name-based (setBucketSpec), so the field renumber needs no source change; no producer or consumer source is touched.WriteRelbody verbatim (common = 7,create_mode = 8,advanced_extension = 9, theCreateModeenum,OUTPUT_MODE_MODIFIED_RECORDS) and relocateBucketSpec bucket_specfrom field 7 to 1000. The nestedBucketSpecmessage is unchanged.SubstraitModifications.md, noting thatWriteRel.commonstill uses Gluten's pre-0.98RelCommoncopy and so cannot carry a full 0.98commonpayload.Gluten's
WriteRelproducer sets onlytable_schema,named_table(withadvanced_extension),input, andbucket_spec; it never setsop,output, orcommon. The Velox and ClickHouse consumers readinput,table_schema,bucket_spec, andnamed_table.advanced_extension— all by name — and ignore the renamed enum value (it appears only in generated code). So no producer or consumer source needs to change for the proto edits.How was this patch tested?
WriteRelProtoSuitepins the producer contract (named table, explicit schema, relocated bucket spec, and the never-set 0.98 fields at their defaults) and asserts the 0.98 field numbers on the descriptor — includingbucket_spec = 1000— plus theOUTPUT_MODE_MODIFIED_RECORDSrename. A renumber or enum-value rename is invisible to a round trip through the shared schema, so the descriptor assertions are the real guard.protocdup-field check (all imports resolve, no duplicate numbers);gluten-substraitbuilds with the proto codegen regenerated and the new suite passes (mvn -Pspark-3.5); scalastyle + spotless clean.WriteRelconsumer body is unchanged; a full locallibveloxlink is currently blocked by an unrelated Velox-EP skew inConfigExtractor.cc, so the end-to-end bucketed-write path (Velox) and the ClickHouseWriteRelparser are exercised by CI, not locally. A green local run does not imply full backend coverage here.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)
🤖 Generated with AI