chore(asap-query-engine): remove 3 persistence tests dependent on the removed datafusion serde - #146
Merged
Conversation
… removed datafusion serde PR #145 silenced a real futex deadlock by stubbing `EpochSource::snapshot_sealed_epoch` and `Store::query_disk_parts` to no-ops, and `#[ignore]`'d three persistence-integration tests that exercised the disk flush + read-back path: - `with_persistence_flushes_sealed_epochs_to_disk` - `query_read_through_merges_memory_and_disk_ranges` - `hard_cap_back_pressure_blocks_inserts_until_flusher_drains` The disk path those tests exercise was built on the datafusion-backed `accumulator_serde` SerDe that PR #123 removed. Rather than rebuild that SerDe for a SimpleMapStore variant that is itself slated for replacement by SketchIndex-backed persistence, the three tests are retired here. The remaining `construct_and_drop_shuts_flusher_cleanly` test stays — it only exercises the flusher's lifecycle, not the SerDe path. Test count: 807 passed (unchanged), 4 ignored (was 7). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
4 tasks
zzylol
added a commit
that referenced
this pull request
Jul 22, 2026
ASAPController#146 (CostModel::size_params) and #154 (bind::logical visibility) are both merged. Re-pin from the feat/costmodel-size-params-hook branch tip to main's current commit now that both land there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Jul 22, 2026
…#408) * feat(control_plane): adopt asap_sketch::L4Node for the L4 IR (Step B) Retires control_plane's own locally-defined, flat PhysicalExpr L4 algebra (Logical/SketchAgg/SketchEstimate/SketchMerge/ExactAgg) in favor of ASAPController's canonical L4 IR, asap_sketch::{SummaryExpr, L4Node} -- the same move Step 3 of the enum-unification made for SketchKind -> SummaryKind, one layer up. PhysicalExpr is now a thin L5 wrapper: `Committed(L4Plan)` for the common case, plus the two Phase eps.1 placement variants. L4Plan keeps only what asap_sketch genuinely doesn't have -- named LetBinding/Ref fan-in sharing (asap_sketch's own DAG sharing is structural, via Rc, but this crate's rule-firing walk still needs a name to thread a bound value across sibling calls). The 7 bind_kll_quantile/bind_ddsketch_quantile/bind_hll_cardinality/ bind_cms_count/bind_cms_topk/bind_exact_agg/bind_archive_only Rule structs are retired -- their selection/sizing policy (KLL's k rungs, DDSketch-over-KLL priority, TopK recall tiers, wire-cost tie-breaks) is preserved verbatim in the new ControlPlaneCostModel, plugged into asap_plan::bind::implement_tree_in_with via the CostModel trait (rank_candidates + the new size_params hook, ASAPController#146) instead of a bespoke dispatcher -- so schema derivation, col/by computation, and DAG construction are asap_plan::bind's, not a forked copy. Three node shapes get a small local pre-pass in sketch_algebra::lower before delegating, because asap_plan::boundary::implementation_for actively binds them to something this deployment's data plane can't (or deliberately shouldn't) serve: - AggIntent::Count{accuracy: Exact} would bind SummaryKind::Count, which has no data-plane accumulator (PR #200/#201 already established this is wrong -- reverted, stays on archive). - AggIntent::Rate would bind its own SummaryKind::Rate; this deployment represents Rate as an Increase accumulator (rate = increase / window, a query-time division, not a separate accumulator). - AggIntent::Extension (Frequency) and TopK{accuracy: Exact} both decline to bind at all (asap_plan's Extension/exact-TopK coverage gaps -- filed upstream as ASAPController#150 and #151); this is a real, accepted behavior change from the retired bind_cms_count/bind_cms_topk rules, not a bug -- see the updated tests in optimizer/rules/mod.rs and emit/mod.rs. Also fixes a real bug surfaced by the migration: PhysicalExpr now carries Rc<L4Node> (asap_sketch's own DAG-sharing mechanism), so holding a bound PhysicalExpr across an .await point made handle_plan's generated Future !Send, breaking axum::Handler. Scoped the Rc-bearing computation into a synchronous block that resolves down to Send-safe StageConfig output before the first await. Pins ASAPQuery-backend's ASAPController dependency to 12482fd77945ab5771a021c6112750fd9284f8ed (ASAPController PR #146, CostModel::size_params + bind::logical). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: re-pin ASAPController to main (post #146/#154 merge) ASAPController#146 (CostModel::size_params) and #154 (bind::logical visibility) are both merged. Re-pin from the feat/costmodel-size-params-hook branch tip to main's current commit now that both land there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Summary
PR #145 silenced a real futex deadlock by stubbing
EpochSource::snapshot_sealed_epochandStore::query_disk_partsto no-ops, and#[ignore]'d three persistence-integration tests:with_persistence_flushes_sealed_epochs_to_diskquery_read_through_merges_memory_and_disk_rangeshard_cap_back_pressure_blocks_inserts_until_flusher_drainsThe disk path those tests exercise was built on the datafusion-backed
accumulator_serdeSerDe that PR #123 removed. Rather than rebuild that SerDe for aSimpleMapStorevariant that is itself slated for replacement by SketchIndex-backed persistence (per the#[ignore]comments in #145), the three tests are retired here.The remaining
construct_and_drop_shuts_flusher_cleanlytest stays — it only exercises the flusher's construct/drop lifecycle, not the SerDe path.Test plan
cargo build --release -p query_engine_rustcleancargo test --release -p query_engine_rust --lib persistence_integration_tests— 1 passedcargo test --release -p query_engine_rust --lib— 807 passed (unchanged from baseline), 4 ignored (was 7), 2 pre-existingschema_timeline_dispatch_testsfailures unchanged🤖 Generated with Claude Code