[Enhancement] Eventize the spillable aggregation operators#74588
[Enhancement] Eventize the spillable aggregation operators#74588eshishki wants to merge 1 commit into
Conversation
|
@codex review |
|
No new undocumented parameters detected by the param-drift check. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d2c2763ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| CLEANUP { | ||
| function: update_be_config("enable_spill_agg_events", "false") | ||
| } END CLEANUP | ||
| select count(*), sum(sl), max(sl), min(sl) from (select c0, sum(c1) sl from t0 group by c0) tb; |
There was a problem hiding this comment.
In validate mode the SQL framework executes the R/ file, not the matching T/ file (ChooseCase.get_cases selects self.r when not recording), so this first “ON” query currently runs without the preceding function: update_be_config("enable_spill_agg_events", "true") that exists only in T/test_spill_agg_events. As a result this test exercises the default false path for both query groups and would not catch regressions in the new event-scheduler path; add the missing update call before this first query in the recorded file.
Useful? React with 👍 / 👎.
| RETURN_IF(!guard.scoped_begin(), Status::Cancelled("cancelled")); | ||
| DEFER_GUARD_END(guard); | ||
| // Completion body, then guard.scoped_end(). It is cancelled on a yield-resubmit (the task lives on | ||
| // and keeps its in-flight increment), so the completion fires exactly once, when the task truly | ||
| // finishes, while scoped_end still balances this invocation's scoped_begin. | ||
| auto guarded = GuardedCompletion( |
There was a problem hiding this comment.
When a queued spill IO task starts after the query has been cancelled and guard.scoped_begin() returns false, this completion guard is never constructed, even though _spiller->increase_in_flight_io() and _running_flush_tasks++ have already happened before submit. That leaves the new _in_flight_io counter permanently positive, so the new pending_finish() paths that wait on has_running_io_tasks() can keep the driver in pending finish forever; the restore and partitioned-flush task bodies repeat the same pattern, so the failed-scoped_begin path needs the same completion/compensation as normal task exit.
Useful? React with 👍 / 👎.
| _remote_disk_bytes_used.get()); | ||
|
|
||
| _notify_count_total = std::make_unique<IntCounter>(MetricUnit::OPERATIONS); | ||
| registry->register_metric("spill_notify_count_total", _notify_count_total.get()); |
There was a problem hiding this comment.
This registers new user-visible BE metrics, but the repo-wide AGENTS.md invariant requires user-facing config or metric changes to update matching public docs in docs/en/ and docs/zh/ when applicable. I searched for spill_notify_count_total, spill_notify_duration_ns_total, and spill_parked_with_uncovered_reason_total under docs/ and found no entries, so operators will see these metrics without documentation for their meaning or alertability.
Useful? React with 👍 / 👎.
🌎 Translation Required?✅ All translation files are up to date.
|
eshishki
force-pushed
the
enhance/spill-event-agg
branch
7 times, most recently
from
116ef34 to
16eb38d
Compare
|
@codex review |
github-actions
Bot
added
the
docs-maintainer
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16eb38d4fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ## `spill_parked_with_uncovered_reason_total` | ||
|
|
||
| - Unit: Count | ||
| - Description: Number of times a pipeline driver was parked on a spill wait whose declared reason is not covered by any wakeup subscription. Always 0 in a healthy system; a non-zero value means a parked driver had nobody to wake it (a wakeup-table bug; the query may stall until it is cancelled) and is worth alerting on. Each occurrence also emits a rate-limited WARNING in the BE log. |
There was a problem hiding this comment.
This adds the new spill_parked_with_uncovered_reason_total metric to the English and Chinese metric reference pages, but docs/ja/administration/management/monitoring/metric_details/s.md exists and was not updated. The docs guide requires corresponding pages in all language directories to stay in sync when editing docs, so Japanese users will not see this new metric in the reference.
Useful? React with 👍 / 👎.
eshishki
force-pushed
the
enhance/spill-event-agg
branch
10 times, most recently
from
4426f3a to
9efd7cc
Compare
eshishki
force-pushed
the
enhance/spill-event-agg
branch
10 times, most recently
from
13fc73f to
bd8424c
Compare
Move the spillable aggregation operators (blocking, distinct-blocking, and the partition-wise agg/distinct variants) onto the pipeline event scheduler from the base change. Each operator's factory opts into the scheduler when enable_spill_agg_events is set; with the flag off (default) they stay on the poller, unchanged from upstream. - Sink edges fold need_input()/block_reason()/pending_finish() through the shared SpillableFlatSinkMixin; the simple blocking/distinct sources through the new SpillableSimpleRestoreSourceMixin; the two partition-wise sources through the new two-axis SpillablePartitionWiseSourceMixin. - block_reason() reasons (WAIT_FLUSH / WAIT_RESTORE / WAIT_CHANNEL) route through the named<> compile guard against each operator's covered_wakeups() mask. - Tests: agg event subscription/gating, observer fan-out wakeups, a lockstep oracle binding has_output() to block_reason() per agg family, and the agg wakeup table + mask coverage (block_reason_agg_test); plus force-spill ON/OFF result-parity e2e. - Documents enable_spill_agg_events (en/zh/ja). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Evgeniy Shishkin <eshishki@gmail.com>
eshishki
force-pushed
the
enhance/spill-event-agg
branch
from
bd8424c to
c6e74aa
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[BE Incremental Coverage Report]❌ fail : 0 / 156 (00.00%) file detail
|
2 participants
Why I'm doing:
Spillable aggregation blocks on spill IO but subscribes to no wakeup, so the busy-poller spin-polls it (the cost is in #74586, the shared base in #74587). This PR opts the aggregation family into the event scheduler.
What I'm doing:
Stacked on #74587 (base).
Each spillable aggregation operator subscribes its parked driver to the spiller's event lists in
prepare()and returns a typedBlockReason, so a spill IO completion wakes exactly that driver instead of the poller finding it on a spin:OUTPUT_FULLon a full writer (WAIT_FLUSH) or a busy spill-process channel (WAIT_CHANNEL); they subscribe to the spiller's sink list and shareSpillableFlatSinkMixin. Woken by the flush-completion defer and the channel handshake.INPUT_EMPTYon restore IO (WAIT_RESTORE) and subscribe to the spiller's source list viaSpillableSimpleRestoreSourceMixin. Woken by restore completion.SpillablePartitionWiseSourceMixin:WAIT_FLUSHwhile the sink drains its final flush,WAIT_RESTOREwhile a per-partition restore is in flight. Every transient per-partition reader completes on the one parent spiller, so a single source-list subscription covers them all.Gated on
enable_spill_agg_events(default false, mutable): off, the agg fragment runs on the poller as before; on, the agg drivers are woken by spill events instead of polled. Query results are unchanged.Part of #74586
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: