-
Notifications
You must be signed in to change notification settings - Fork 359
Cosmos: Regression tests for feed-range-scoped query fan-out #4705
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
0efa7b8
ba795b8
6c81640
ad9e832
96390fd
b97b0cf
19bb6d4
1d74c9d
9f3e986
6a29dde
b817f11
5c9b577
9d90f1a
0d29d2e
0d3c8dc
194b406
140feda
ab07e94
952e64f
f920bdc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -380,6 +380,11 @@ async fn plan_fresh( | |
| let mut nodes: Vec<Box<dyn PipelineNode>> = Vec::new(); | ||
| for query_range in &query_plan.query_ranges { | ||
| let feed_range = query_range_to_feed_range(query_range)?; | ||
| // Restrict the query-plan range to the operation's requested scope. | ||
| // Ranges entirely outside the target contribute no request leaves. | ||
| let Some(feed_range) = clip_to_target(feed_range, operation) else { | ||
| continue; | ||
| }; | ||
| let resolved = topology_provider | ||
| .resolve_ranges(&feed_range, PartitionRoutingRefresh::UseCached) | ||
| .await?; | ||
|
|
@@ -429,6 +434,11 @@ async fn plan_resume_from_saved_snapshot( | |
|
|
||
| for query_range in &query_plan.query_ranges { | ||
| let feed_range = query_range_to_feed_range(query_range)?; | ||
| // Restrict the query-plan range to the operation's requested scope, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please also add an e2e test - simple SELECT * FROM c query with FeedScope to validate that it indeed honors the rnage (it will with your fix - but having an e2e regression test makes it much simpler to prevent regressions than unit tests which are easier to overlook in larger changes). I also disagree with the decision to scope out HPK (i would have preferred fixing it in this PR as well) - but whatever - as long as it is also fixed before next release that is fine.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @FabianMeiswinkel Added the e2e test, I've been tracking the HPK scenarios separately through more tests here: #4682 |
||
| // mirroring the fresh path. Ranges outside the target are skipped. | ||
| let Some(feed_range) = clip_to_target(feed_range, operation) else { | ||
| continue; | ||
| }; | ||
| let resolved = topology_provider | ||
| .resolve_ranges(&feed_range, PartitionRoutingRefresh::UseCached) | ||
| .await?; | ||
|
|
@@ -572,6 +582,28 @@ fn query_range_to_feed_range( | |
| FeedRange::new(min, max) | ||
| } | ||
|
|
||
| /// Clips a query-plan feed range to the operation's requested scope | ||
| /// ([`CosmosOperation::target`]). | ||
| /// | ||
| /// The backend query plan's ranges are derived from the query text alone and | ||
| /// never reflect the caller's requested [`FeedRange`] target, so the planner | ||
| /// must intersect them here. Returns `None` when the query range lies entirely | ||
| /// outside the target, meaning it contributes no request leaves. | ||
| /// | ||
| /// When the operation has no target, or targets a logical partition / prefix, | ||
| /// the range is returned unchanged: a logical-partition [`FeedRange`] collapses | ||
| /// its `[min, max)` bounds to a single effective partition key and therefore | ||
| /// cannot be intersected as an EPK range. Prefix-scoped fan-out is handled | ||
| /// elsewhere; clipping it here would incorrectly drop every range. | ||
| fn clip_to_target(feed_range: FeedRange, operation: &CosmosOperation) -> Option<FeedRange> { | ||
| match operation.target() { | ||
| Some(target) if !target.is_logical_partition() => { | ||
| intersect_feed_ranges(&feed_range, target) | ||
| } | ||
| _ => Some(feed_range), | ||
| } | ||
| } | ||
|
|
||
| /// Returns true if the union of `pieces` covers `range` end-to-end. | ||
| /// | ||
| /// Assumes pieces are subsets of `range`. The check sorts pieces by | ||
|
|
@@ -864,6 +896,18 @@ mod tests { | |
| .with_body(br#"{"query":"SELECT * FROM c"}"#.to_vec()) | ||
| } | ||
|
|
||
| /// A cross-partition query scoped to an explicit EPK feed-range target, | ||
| /// e.g. `FeedScope::range([min, max))`. | ||
| fn query_operation_with_target(min: &str, max: &str) -> CosmosOperation { | ||
| let target = FeedRange::new( | ||
| EffectivePartitionKey::from(min), | ||
| EffectivePartitionKey::from(max), | ||
| ) | ||
| .unwrap(); | ||
| CosmosOperation::query_items(test_container(), Some(target)) | ||
| .with_body(br#"{"query":"SELECT * FROM c"}"#.to_vec()) | ||
| } | ||
|
|
||
| // --- build_trivial_pipeline tests --- | ||
|
|
||
| #[test] | ||
|
|
@@ -1180,6 +1224,145 @@ mod tests { | |
| assert_drain_requests_with_partitions(pipeline, &[("20", "80", "pkrange-wide", "", "FF")]); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn restricts_fanout_to_explicit_target_range() { | ||
| // The reported bug: query plan spans the whole space `[, FF)` but the | ||
| // caller scoped the query to `[00, 80)` via `FeedScope::range`. Only | ||
| // the requested slice must be queried, not the neighbouring `[80, FF)` | ||
| // partition. The physical topology actually contains both partitions, | ||
| // so a planner that ignores the target would resolve and emit a leaf | ||
| // for `[80, FF)` as well. | ||
| let plan = plan_with_ranges(vec![qr("", "FF")]); | ||
| let op = query_operation_with_target("00", "80"); | ||
| let mut topology = PhysicalTopologyProvider::new(vec![ | ||
| rr("00", "80", "pkrange-left"), | ||
| rr("80", "FF", "pkrange-right"), | ||
| ]); | ||
|
|
||
| let pipeline = build_sequential_drain(&plan, &mut topology, &Arc::new(op), None) | ||
| .await | ||
| .unwrap(); | ||
| assert_drain_requests(pipeline, &[("00", "80", "pkrange-left")]); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn drops_query_ranges_outside_target() { | ||
| // Two disjoint query-plan ranges; the target only overlaps the first. | ||
| // The second range must contribute no request leaves (and its topology | ||
| // must never be resolved). | ||
| let plan = plan_with_ranges(vec![qr("", "40"), qr("80", "FF")]); | ||
| let op = query_operation_with_target("", "40"); | ||
| let mut topology = MockTopologyProvider::new(vec![Ok(vec![rr("", "40", "pkrange-A")])]); | ||
|
|
||
| let pipeline = build_sequential_drain(&plan, &mut topology, &Arc::new(op), None) | ||
| .await | ||
| .unwrap(); | ||
| assert_drain_requests(pipeline, &[("", "40", "pkrange-A")]); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn clips_query_range_to_partial_target_overlap() { | ||
| // The target `[20, 60)` partially overlaps a single query-plan range | ||
| // spanning several partitions. Only partitions within the target, | ||
| // clipped to its bounds, may be queried. | ||
| let plan = plan_with_ranges(vec![qr("", "FF")]); | ||
| let op = query_operation_with_target("20", "60"); | ||
| let mut topology = MockTopologyProvider::new(vec![Ok(vec![ | ||
| rr("00", "40", "pkrange-1"), | ||
| rr("40", "80", "pkrange-2"), | ||
| ])]); | ||
|
|
||
| let pipeline = build_sequential_drain(&plan, &mut topology, &Arc::new(op), None) | ||
| .await | ||
| .unwrap(); | ||
| assert_drain_requests_with_partitions( | ||
| pipeline, | ||
| &[ | ||
| ("20", "40", "pkrange-1", "00", "40"), | ||
| ("40", "60", "pkrange-2", "40", "80"), | ||
| ], | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn drops_query_range_touching_target_boundary() { | ||
| // A query-plan range that only *touches* the target's exclusive upper | ||
| // bound (target `[, 40)`, range `[40, FF)`) shares no EPKs with the | ||
| // target and must be dropped, not queried. Exercises the exact | ||
| // boundary case where `intersect_feed_ranges` collapses to empty. | ||
| let plan = plan_with_ranges(vec![qr("", "40"), qr("40", "FF")]); | ||
| let op = query_operation_with_target("", "40"); | ||
| let mut topology = PhysicalTopologyProvider::new(vec![ | ||
| rr("", "40", "pkrange-A"), | ||
| rr("40", "FF", "pkrange-B"), | ||
| ]); | ||
|
|
||
| let pipeline = build_sequential_drain(&plan, &mut topology, &Arc::new(op), None) | ||
| .await | ||
| .unwrap(); | ||
| assert_drain_requests(pipeline, &[("", "40", "pkrange-A")]); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn logical_partition_target_is_not_clipped() { | ||
| // A logical-partition (prefix) target must NOT be clipped: its | ||
| // `[min, max)` bounds collapse to a single EPK, so intersecting the | ||
| // query-plan ranges against it would drop everything. The guard in | ||
| // `clip_to_target` leaves such ranges untouched, letting the query | ||
| // fan out across the resolved topology. This also documents that | ||
| // prefix-scoped over-scan is handled elsewhere (see follow-up). | ||
| let plan = plan_with_ranges(vec![qr("", "FF")]); | ||
| let target = | ||
| FeedRange::for_partition(PartitionKey::from("pk1"), &test_partition_key_definition()); | ||
| assert!(target.is_logical_partition()); | ||
| let op = CosmosOperation::query_items(test_container(), Some(target)) | ||
| .with_body(br#"{"query":"SELECT * FROM c"}"#.to_vec()); | ||
| let mut topology = PhysicalTopologyProvider::new(vec![ | ||
| rr("00", "80", "pkrange-left"), | ||
| rr("80", "FF", "pkrange-right"), | ||
| ]); | ||
|
|
||
| let pipeline = build_sequential_drain(&plan, &mut topology, &Arc::new(op), None) | ||
| .await | ||
| .unwrap(); | ||
| assert_drain_requests( | ||
| pipeline, | ||
| &[("00", "80", "pkrange-left"), ("80", "FF", "pkrange-right")], | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn resume_restricts_fanout_to_target_range() { | ||
| // Resuming a target-scoped query must also honour the target: the | ||
| // `[80, FF)` partition lies outside `[00, 80)` and must not be queried | ||
| // even though the query plan spans `[, FF)` and that partition exists | ||
| // in the physical topology. An unclipped resume would emit a second, | ||
| // fresh-start leaf for `[80, FF)`. | ||
| let plan = plan_with_ranges(vec![qr("", "FF")]); | ||
| let op = query_operation_with_target("00", "80"); | ||
| let mut topology = PhysicalTopologyProvider::new(vec![ | ||
| rr("00", "80", "pkrange-left"), | ||
| rr("80", "FF", "pkrange-right"), | ||
| ]); | ||
|
|
||
| let resume = saved_drain(vec![("00", "80", saved_request(Some("server-token-xyz")))]); | ||
|
|
||
| let pipeline = build_sequential_drain(&plan, &mut topology, &Arc::new(op), Some(resume)) | ||
| .await | ||
| .unwrap(); | ||
| assert_drain_requests_with_partitions_and_continuation( | ||
| pipeline, | ||
| &[( | ||
| "00", | ||
| "80", | ||
| "pkrange-left", | ||
| "00", | ||
| "80", | ||
| Some("server-token-xyz"), | ||
| )], | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn rejects_query_plan_with_top() { | ||
| let plan = QueryPlan { | ||
|
|
@@ -1346,6 +1529,27 @@ mod tests { | |
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn rejects_target_disjoint_from_query_ranges() { | ||
| // A target that shares no EPKs with any query-plan range clips every | ||
| // range away, leaving zero request leaves. On the fresh path this is | ||
| // reported as the same hard error as an empty query plan — the clip | ||
| // does not silently swallow the query. `NoopTopologyProvider` asserts | ||
| // no partition is ever resolved (the clip skips before resolution). | ||
| let plan = plan_with_ranges(vec![qr("80", "FF")]); | ||
| let op = query_operation_with_target("00", "40"); | ||
| let mut topology = NoopTopologyProvider; | ||
|
|
||
| let err = build_sequential_drain(&plan, &mut topology, &Arc::new(op), None) | ||
| .await | ||
| .unwrap_err(); | ||
| let rendered = err.to_string(); | ||
| assert!( | ||
| rendered.ends_with("query plan produced no partition ranges to query"), | ||
| "unexpected: {rendered}" | ||
| ); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn propagates_topology_resolution_error() { | ||
| let plan = plan_with_ranges(vec![qr("", "FF")]); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.