Repository navigation
feat(sirius): lower MO exact decimals for embedded GPU execution - #29757
Conversation
…act-decimal # Conflicts: # pkg/util/metric/mometric/metric_exporter_test.go
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of base/merge-base e5dd4724f782067e1c76c38c9d2f55cebceda4a9 → head b34111b4c9e94cda034d199a2c362072fe98d808, including the follow-up to reviewed b39755ff4e and unchanged merged Sirius pin 908ffc75a58b3be496b2172795e416326416e7ec. APPROVE: both previous blocking findings are resolved; no new blocker found. No subagents or production edits.
Comment closure
- Zero DATE: the typed plan now compares against the same shifted
types.ZeroDaterepresentation used by the native reader and produces 0 for that sentinel. Normal/NULL dates retain the component path. The two casts normalize the pinned component kernel's physical INT16 result through INT32 to the declared BIGINT, avoiding the importer eliminating a same-type cast. The existing public fixture now persists zero DATE, compares 0/0/0, retains year-0001/leap-day/year-9999/NULL controls, and restores SQL mode. - CASE descriptor: IfThen is wrapped in the existing checked
mo_decimal_castannotation with the complete MO-bound result descriptor. The required-arm/nullable-condition fixture checks both branch values and nullable DECIMAL(15,2) public metadata. The previous nullable-arm and masked-error controls remain. This restores MO as the type authority rather than copying the native arm-only inference rule. - The additional ordinary integer +/-/* admission restriction correctly contains a demonstrated unchecked-consumer overflow gap. It applies to embedded post-scan work; MO-owned scan filters keep their normal executor and Flight retains its existing contract. Checked exact arithmetic and integral aggregates remain available. Read both replies and all current PR review threads; none remains substantively unresolved.
Design and complete change map
The approved semantic design (#29449, blob 42a89f09a1d168d02b9583cb3ea7b4de6dbb5634) and C/D delivery split (#29690) remain applicable. Reviewed all current changed hunks, separately accounting for implementation +825/-59 in 12 files, tests +939/-10 in 13 files, documentation +424/-0 in one file, plus one dependency-pin update. The histogram synchronization change from the earlier review is now inherited unchanged from main and is not a remaining PR delta.
The five closures remain coherent: immutable runtime capability → candidate profile; bound types/overloads/literals → exact wire/native preparation; Decimal256 reader publication → borrowed result; native status → public error/terminal evidence; merged SDK pin → supported delivery. Existing MO readers own filters, transactions and snapshots. Input credit, result leases, cancellation and query cleanup retain their established owners. No second allocator, session state machine, execution framework or storage path is introduced. Flight/default execution and the ABI remain separate existing contracts rather than partially mixed exact-decimal paths.
Q1: publication acquires credit before allocation and defers release; result vectors borrow the existing lease and batch cleanup runs after fill; native numeric failure remains primary ahead of cancellation fallout. Q2: native cancellation stays independent of data-path locking, then joins producers before destruction. Q3: existing 64 MiB windows, constant expansion accounting, bounded admission/columns/plan/heading sizes remain effective for 32-byte values; terminal schema evidence is emitted once per execution, without raw headings/values. No new shared mutable state or worker is added.
Tests prove distinct layers: descriptor/wire admission, publication/borrowing, error identity, native preparation, and public differential behavior. The new regression scenarios extend the existing GPU/public fixture; they do not add another cluster framework or weaken previous assertions. Documentation records the current contract and explicitly separates historical evidence and later campaign gates.
Adversarial checks and performance
Challenged the fixes beyond the original reproducers: nullable conditions with required arms, nested CASE, CASE consumed by arithmetic, embedded integer aggregate arithmetic versus MO-owned integer scan filters, DATE versus unsupported quarter, and exact versus legacy profiles. Traced the pinned native normalizer/evaluator, not just the new Go test assertions. Identical decimal cast annotations are removed or return the original operand without copying coefficients.
Unhinted TP and ordinary prepared execution return at the existing offload gate before any new capability/profile work. Embedded DATE extraction now does extra comparison/conditional/cast work, and exact arithmetic, wide aggregates and terminal schema hashing have real costs. This approval does not establish GPU speedup or no performance regression. The approved opt-in C scope remains intact; D's all-22 SF1/SF10, resource/lifecycle and performance campaign is still required before its acceptance or broader rollout. #28968/#28966 remain open.
Validation and limits
- Locally at this exact head: focused exact/embedded/extract tests passed (3.428s), including all 22 exporter inventory cases and the three new DATE/CASE/integer controls. A private overlay probe using the real parser/optimizer passed seven admission/serialization counterexample/control scenarios (1.123s). No source files were changed.
- Reused exact-head CI run 38033352247, completed success. Inspected its downloaded
ut-summarycheckpoint artifact: substrait, compile, siriusbridge and cnservice were all explicitly selected in the light race-test group, which terminated with status 0. Required CI is green. These CPU checks do not execute the GPU-tagged fixture. - GPU/public execution evidence is the author's recorded current-head result: all 22 native preparations (1.116s), full public normal/race runs (9.762s/11.999s), and combined bridge/coexistence (1.108s), with 34 terminal events per public run showing completed GPU work, exact profile/capability31, no fallback, healthy cleanup and zero retained credits. The review replies also record actual GPU red/green closure of both original findings. I checked the retained assertions and pinned consumer source; I did not execute CUDA locally or independently reproduce those GPU timings.
git diff --checkpassed; the isolated source worktree is clean. Native CGo inputs and the Sirius pin relevant to reused evidence remain unchanged. The original CPU oracle established the zero-date and nullable-CASE public contract; it is not represented as GPU execution.
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
Opt-in embedded MO-reader queries now lower MO exact Decimal64/128/256 values and descriptors to Sirius GPU execution. One immutable capability-scoped profile owns admission, physical types/literals, bound scalar/aggregate overloads, input publication, result reconstruction and public numeric error classes. Terminal evidence records verified descriptors and a schema digest without logging headings or values.
This implements approved PR C from #29690 and the versioned design/validation map. Physical width is preserved at narrow declared precision; MO-bound division metadata and 32-byte Decimal256 coefficients use the existing credit-before-copy and borrowed-result contracts. Embedded fetch defaults and DATE extraction use supported native wire forms.
All prerequisites are merged and inherited:
908ffc75a58b3be496b2172795e416326416e7ec, with merged importer [#4] Add a new document: CHANGELOG.md #5 at99c7ca3b6f8f3159239e119ed2982d42f98c4690.af3f7232a31879a2d6ed590785c8e9a560361c3f. Prepared SELECT metadata comes from the executed generation; incomplete saved SELECTs clear session state before reuse.Final release/native integration at executable C head
ad61b5b5d3, on merged-kernel mainaf3f7232a3, passes:mo-exact-decimal-v1, completed GPU tasks, no fallback, healthy cleanup and zero retained input/result credit.The delivered head is
b34111b4c9e94cda034d199a2c362072fe98d808, normally merged with authoritative maine5dd4724f782067e1c76c38c9d2f55cebceda4a9. Review fixes are committed inaf4f342066: typed DATE lowering preserves persisted MO zero dates; exact CASE retains the MO-bound descriptor around IfThen; ordinary embedded integer add/subtract/multiply declines before readers because the pinned consumer does not implement MO checked overflow. Flight encoding/admission, reader bytes, native ABI and the merged Sirius pin are unchanged.The installed and loaded host drivers now both report NVIDIA 615.78.08; final GPU checks run directly with the installed driver. The bounded local native fixture uses 256 MiB GPU and 2 GiB host capacity. An initial 512 MiB host fixture correctly rejected larger progress-window reservations; only the test fixture was corrected. Production windows, limits and assertions remain unchanged.
Review-resolution validation at prior head
a4e34ef05d:Ready for review; merge gates remain required CI and reviewer approval. Both incoming findings were reproduced on actual GPU/public MySQL and fixed with regressions. A separate admitted integer-overflow counterexample returned -2 instead of native MO 1690/22003; it now declines before execution. Fresh CI is pending for the delivered head, and the reviewer has been asked to reassess the fixes. Local passes do not establish required CI success.
C remains opt-in. D begins after C merges and stays one complete public campaign PR for all-22 SF1/SF10, bounded comparison, lifecycle/resource/process evidence and performance gates. E/F retain their separate recovery-readiness design. References #28968 and #28966; neither issue is closed by C.
Conflict resolution at
b34111b4c9(2026-10-10): normally merged authoritative maine5dd4724f7. The single conflict was two equivalent asynchronous histogram-test fixes; main's stopped-timer notification barrier is retained unchanged. All 28 prior C paths were checked: only this test changed, while the DATE/CASE/integer review fixes and merged native pins remain intact.Validation at this merged head passes: affected owning packages normal/race, 100 focused metrics race repetitions, fresh native generations and combined release/package, all 22 actual preparations (1.116s), full public GPU normal/race (9.762s/11.999s), and full combined bridge/coexistence (1.108s). Both public runs' 34 terminal events verify completed GPU work, exact profile/capability31, healthy cleanup, no fallback and zero retained credits. Mandatory pre-push SCA passes with zero findings; self-review has no unresolved implementation blocker. New-head required CI and human reviewer approval remain the merge gates.