Skip to content

feat: stream local scans to Sirius sidecar - #27599

Open
aunjgr wants to merge 22 commits into
matrixorigin:mainfrom
aunjgr:feature/27586-stream-input
Open

feat: stream local scans to Sirius sidecar#27599
aunjgr wants to merge 22 commits into
matrixorigin:mainfrom
aunjgr:feature/27586-stream-input

Conversation

@aunjgr

@aunjgr aunjgr commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #27586

What this PR does / why we need it:

Completes the explicit /*+ SIDECAR STREAM */ path for sending snapshot-correct local-CN scan results to Sirius and receiving results as MatrixOne-native batches.

  • exports versioned StreamRead leaves while retaining joins, aggregates, sorts, and other admitted operators in Substrait
  • executes each read through MatrixOne's native read-only snapshot scan on the current CN
  • sends bounded Batch.MarshalBinary frames over authenticated Flight DoPut
  • negotiates the exact MO result schema and decodes strict canonical MOB1 frames from DoGet
  • removes the Arrow IPC result decoder and introduces no Arrow-Go/forbidden dependency
  • accepts CHAR/VARCHAR as the TPCH string family, including Q22 substring(CHAR, 1, 2)
  • preserves single-use visibility, cancellation, timeout, disconnect, lease cleanup, and allocation-account lifecycles
  • keeps the established direct TaeRead type contract unchanged while rejecting unsupported physical types only for StreamRead
  • retires unused native producers immediately at successful result EOF, then joins server-side DoPut handlers before cleanup
  • rejects current-transaction writes, multi-CN producers, shared one-pass scan nodes, unsupported metadata, and schema/protocol mismatches

Dependencies:

Validation

  • merged newest main (merge commit, no rebase)
  • pkg/sql/compile/sidecarflight, pkg/sql/plan/substrait, and Sirius compile-path tests passed
  • sidecarflight coverage: 80.4%; Substrait coverage: 83.0%
  • focused result-EOF, blocked-acknowledgement, cancellation, and codec tests passed 100 race-stress repetitions; the full owning package passed under -race
  • Sirius/sidecar CUDA builds and all native protocol test binaries passed
  • all 22 TPC-H SF1 views matched native MatrixOne byte-for-byte through SIDECAR STREAM on one reused process

aunjgr and others added 3 commits July 1, 2026 18:08
For INNER JOIN, ON = WHERE semantically. Use WhereBinder instead
of TableBinder so scalar subqueries pass through binding, then
separate subquery conditions from join conditions, flatten via
flattenSubqueries (same as bindWhere), and wrap in a FILTER above
the join.

Before: 'subquery in JOIN condition is not yet implemented'
After: subquery is flattened into LEFT JOIN + FILTER above the
INNER JOIN, matching the native WHERE subquery path.

LEFT/RIGHT/OUTER joins unchanged — still use TableBinder.

Co-Authored-By: Claude <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@aptend aptend left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-reviewed the exact current head, the complete diff and the increment since 55caa53, all prior reviews/replies/thread states, and both pinned dependency PRs. The two previously reported blockers are genuinely closed: StreamRead-only physical type rejection no longer affects direct TaeRead, and result EOF now retires and joins pre-first-frame producers. One new blocking result-wire issue remains: the decoder accepts canonical compressed vector classes whose logical work is not bounded by the negotiated frame limit.

Comment thread pkg/sql/compile/sidecarflight/native_result.go

@aptend aptend left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-reviewed exact head 4ba883e, the increment since my prior reviewed head 758a52a, the complete diff, all historical reviews/replies/thread states, and the pinned Sirius/sidecar dependencies. The flat-only native-result validation genuinely closes the remaining blocker: the tiny 1<<30-row CONSTANT counterexample and DIST vectors are rejected before fill, while accepted canonical FLAT vectors have encoded size proportional to logical rows. The earlier direct-TaeRead eligibility and pre-first-frame producer-retirement fixes remain closed. I verified the full affected package suites (including pkg/sql/compile), full race suites for sidecarflight/vector/substrait, 100 race-enabled repetitions of the compressed-result, EOF-retirement, StreamRead, and uint32 regressions, and go vet on affected packages. I found no blocking issue.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep re-review completed at exact head 4ba883e03b54108ab9933dbe9c77dd93965f46fe.

The previously reported implementation blockers are closed: direct TaeRead keeps its existing uint32 contract, StreamRead rejects incompatible native inputs before Prepare, successful result EOF retires producers blocked before their first frame, and native results now reject compressed/constant vector classes before fill, so tiny frames cannot expand into unbounded decoded work.

Two merge blockers remain:

  1. Mandatory design gate is missing. This PR is explicitly a feature, adds well over 500 production lines, and changes a cross-repository MO ↔ sidecar ↔ Sirius authenticated wire protocol, execution/lifecycle state machine, cancellation and lease ownership, result codec, hot-path backpressure, and rollout contract. There is no approved versioned design document in the PR. Issue #27586 is an experiment brief rather than an approved design and is already materially stale: it specifies Arrow input while this revision replaces Arrow with native MOB1 batches. Please add and obtain distinct approval for a stable design covering the exact protocol/state transitions, ownership and Q1-Q3 bounds, schema/type compatibility, fallback visibility point, dependency/version negotiation, rollout/rollback, failure containment, and performance/capacity budgets.

  2. The required cross-repository delivery evidence is not closed. The exact dependencies are still open: Sirius #6 currently has multiple failed and queued build checks, and sidecar #14 has no submitted review or CI result. The MatrixOne CI cannot prove this three-repository runtime contract. Before merge, pin merge-ready dependency revisions and provide exact end-to-end evidence against them, including the issue-required slow-consumer memory bound, producer/consumer failure and cancellation cleanup, SF10 cases, GPU-path execution evidence, and the stated resource/performance measurements (or update the approved design/acceptance criteria with an explicit owner decision).

Because these are objective design and delivery gates for a distributed feature, exact-head MatrixOne CI being green is insufficient for approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature size/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants