fix(topk): preserve typed count updates and migrate acceptance to registered QueryPlans - #511
Merged
Merged
Conversation
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
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.
Why
TopK binding must retain its typed count updates, and its acceptance tests must exercise the integrated architecture: register a temporal query, select a Planner DAG, install the physical QueryPlan, then execute that binding.
Before this PR
The legacy emitter dropped the selected count mode. Two heap E2Es installed only StreamingConfig and requested unregistered instantaneous
topk(3, top_endpoint_qps); both failed withNo result for query.After this PR
The evidence-aware legacy adapter retains
weight_mode=countand fails closed without membership evidence. The two serving tests are migrated to:For six series with counts alpha=100, beta=50, gamma=200, delta=75, epsilon=10 and zeta=150, both CMS-with-heap and CountSketch-with-heap return gamma=200, zeta=150 and alpha=100. Every raw sample has value 17, so a value-weighted implementation cannot accidentally pass.
How and scope
Based on #505, which already contains the shared materialization identity and canonical temporal TopK changes originally developed here. This acceptance migration is backend-local, not distributed OTLP/sketch-envelope parity. Existing
itemlabels retain canonical series identities; native label reconstruction is not claimed.Fixture certificates are deterministic test inputs, not a production evidence estimator. This PR does not close Planner #340's entire legacy acceptance scope.
Verification
Current commit
1a85748:cargo test -p data_plane --test asapquery_compatibility_process_e2e: 4 passed, including both newly migrated heap tests.cargo test -p data_plane --test e2e_controller_plans_and_backend_serves --no-run: passed.git diff --check: passed.No resultfailures.Earlier in this PR: 656 control-plane library tests and the distributed production-process test passed for the emitter/base integration. The complete legacy E2E suite is not claimed green.
Self-review, not independent approval. No merge performed.