Fix HPK prefix and cross-partition query bugs#4729
Conversation
Both bugs share a single root cause: the driver emitted the point key-type `EffectivePartitionKey` in `x-ms-read-key-type` while sending `x-ms-start-epk`/`x-ms-end-epk`, which describe an EPK *range*. The gateway rejects that combination with `400 "One of the input values is invalid"`. The correct value is `EffectivePartitionKeyRange` (validated against a live Cosmos account for both query and change-feed paths; no api-version bump needed). - azure_data_cosmos_driver/operation_pipeline.rs: emit `EffectivePartitionKeyRange` when EPK bounds are present; update the apply_headers unit test. - azure_data_cosmos_driver/planner.rs: prefix EPK-range clipping (prefix_epk_range / clip_to_prefix) so a `FeedScope::partition` prefix fans out over `[prefix_epk, prefix_epk + "FF")` instead of scanning whole physical partitions. - azure_data_cosmos/feed/query.rs: document prefix support on `FeedScope::partition`. - azure_data_cosmos in-memory emulator tests: end-to-end HPK prefix and cross-partition coverage through the public SDK. - CHANGELOG entries in both crates. Fixes #4680 Fixes #4681 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reference PR #4729 in the azure_data_cosmos and azure_data_cosmos_driver CHANGELOG entries for the HPK prefix and cross-partition query fixes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes two hierarchical-partition-key (HPK / MultiHash) query bugs that were deferred out of #4705. Both share a single root cause: when the driver scopes a data-plane request to an effective-partition-key (EPK) range (emitting x-ms-start-epk/x-ms-end-epk), it was sending the point key-type x-ms-read-key-type: EffectivePartitionKey, which the gateway rejects with 400. The fix emits EffectivePartitionKeyRange and adds prefix→EPK-range clipping so a partial HPK key (FeedScope::partition prefix) fans out over — and is EPK-filtered within — only the physical partitions its completions can occupy.
Changes:
- Emit
x-ms-read-key-type: EffectivePartitionKeyRangefor EPK range-scoped requests (fixes both #4680 filtered-prefix queries and #4681 cross-partition 400s). - Add
prefix_epk_range/clip_to_prefixin the planner to scope prefix fan-out to[prefix_epk, prefix_epk + "FF"), applied in both fresh and resume leaf builders. - Add CI-runnable SDK-level HPK tests plus driver unit tests, document prefix support on
FeedScope::partition, and add CHANGELOG entries.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
azure_data_cosmos_driver/src/driver/pipeline/operation_pipeline.rs |
Sends EffectivePartitionKeyRange key type for EPK range-scoped requests; updates the corresponding unit test. |
azure_data_cosmos_driver/src/driver/dataflow/planner.rs |
Adds prefix EPK-range computation and clipping into both fan-out leaf builders, with three new unit tests. |
azure_data_cosmos/src/feed/query.rs |
Documents that FeedScope::partition now supports HPK prefixes as filtered cross-partition queries. |
azure_data_cosmos/tests/in_memory_emulator_tests/hpk.rs |
New SDK-level tests covering prefix filtering, no-match, cross-partition full-container, and full-key cases. |
azure_data_cosmos/tests/in_memory_emulator_tests/mod.rs |
Registers the new hpk test module. |
azure_data_cosmos/CHANGELOG.md |
Adds a Bugs Fixed entry for #4680/#4681. |
azure_data_cosmos_driver/CHANGELOG.md |
Adds a Bugs Fixed entry describing the x-ms-read-key-type fix. |
FabianMeiswinkel
left a comment
There was a problem hiding this comment.
LGTM except for few NITs
Address review feedback on the HPK query fixes (#4680, #4681), aligning edge-case behavior with the .NET SDK (which routes a prefix scope directly to its effective-partition-key range and lets the server filter, rather than intersecting with query-plan ranges). Behavioral changes (planner.rs): - A fresh prefix-scoped fan-out whose query ranges fall entirely outside the prefix EPK range now drains to an empty page instead of raising CLIENT_QUERY_PLAN_PRODUCED_EMPTY_RANGES, matching .NET. - On resume, a saved continuation range that lies outside the current prefix scope is treated as already satisfied (clipped to the prefix before the coverage guard) instead of hard-failing with an unhonored- range error. - prefix_epk_range debug_asserts the incomplete-key invariant while still degrading gracefully in release. Regression hardening: - The in-memory emulator now rejects an EPK-range-scoped read (x-ms-start-epk/x-ms-end-epk) that declares the point key type instead of EffectivePartitionKeyRange with 400 "One of the input values is invalid", so the emulator-backed HPK tests fail if the driver's key-type value regresses. - Strengthen hpk_query_prefix_correctness_guard with an explicit count so an empty result cannot pass the anti-leak loop. - Add planner tests for a three-level prefix, a disjoint fresh prefix, a query range skipped outside the prefix, and resume outside the prefix; assert the split point in the multi-partition prefix test; guard the FF/0x3F EPK sentinel invariant. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolve conflicts after main independently landed the Gateway V2 / routing / RNTBD refactor, which also reimplemented the HPK-prefix fan-out mechanism (compute_range-based FeedRange::for_partition, target-intersect clipping in the planner, and partial-PK emission in cosmos_driver). That work supersedes this branch's WIP prefix_clip planner code, so planner.rs is taken wholesale from main. The essential HPK fix survives unchanged: the driver still emitted the point key type `EffectivePartitionKey` alongside x-ms-start-epk / x-ms-end-epk for range-scoped requests, which the gateway rejects with 400 "One of the input values is invalid" (#4680, #4681). It now emits `EffectivePartitionKeyRange`. Conflict resolutions: - operation_pipeline.rs: keep main's unconditional to_hex() EPK-bounds block; correct the read-key-type value to EffectivePartitionKeyRange and update the apply_headers unit test assertion. - planner.rs: take origin/main (redundant prefix_clip code/tests dropped). - effective_partition_key.rs: port the compute_range nibble test to the new binary Cow<[u8]> / to_hex() API (as_str() was removed on main). Also carry the in-memory emulator validation support added on this branch: an InvalidInput guard in dispatch.rs (400 when EPK bounds are paired with a non-range read-key-type) and a partial-PK fix in operations.rs::collect_item_documents so a prefix PK header is scoped by the start/end-epk range instead of an exact point EPK (which dropped every full-key item). All six in-memory HPK tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Hi @simorenoh — nice work here. The EffectivePartitionKeyRange header correction in this PR is exactly the fix I'd landed independently in #4726, and your version carries the HPK context plus .NET validation, so I'm closing #4726 in favor of this PR and @tvaron3's #4319. One artifact from #4726 that I don't think is covered elsewhere: a standard-gateway (non-GW2.0, single-hash It's // Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
//! End-to-end regression test for the cross-partition query **scope bug**: the
//! planner used to ignore the caller's `FeedScope::range(..)` feed range.
//!
//! Before the fix, `plan_fresh` built its request ranges purely from the query
//! plan (`query_plan.query_ranges`) and never intersected them with the
//! operation's scope feed range (`operation.target()`). For a plain `SELECT *`
//! the query plan reports the whole container, so a `FeedScope::range([X, Y))`
//! window was silently dropped and the query did a **full scan** — returning
//! documents outside the requested window.
//!
//! This test seeds one document per partition key, computes each document's
//! effective partition key locally, and issues cross-partition queries scoped
//! to interior EPK windows. It asserts the result set is exactly the windowed
//! subset. Without the planner fix these assertions fail with
//! `SCOPE BUG` (too many documents returned); with the fix — plus the
//! `x-ms-read-key-type: EffectivePartitionKeyRange` header correction that lets
//! the gateway accept the now-emitted interior EPK window — they pass.
//!
//! Run against the emulator:
//! RUSTFLAGS='--cfg test_category="emulator"' \
//! cargo test -p azure_data_cosmos --test emulator \
//! --features "key_auth fault_injection" feed_range_scope
use super::framework;
use std::borrow::Cow;
use std::collections::BTreeSet;
use std::error::Error;
use azure_data_cosmos::clients::ContainerClient;
use azure_data_cosmos::feed::{FeedRange, FeedScope};
use azure_data_cosmos::models::{
ContainerProperties, EffectivePartitionKey, PartitionKeyDefinition,
};
use azure_data_cosmos::{PartitionKey, Query};
use futures::TryStreamExt;
use serde::{Deserialize, Serialize};
use framework::{TestClient, TestOptions};
const DOC_COUNT: usize = 40;
#[derive(Clone, Debug, Serialize, Deserialize)]
struct Doc {
id: String,
pk: String,
}
fn pk_definition() -> PartitionKeyDefinition {
PartitionKeyDefinition::new(vec![Cow::Borrowed("/pk")])
}
/// Runs a feed-range-scoped `SELECT *` and returns the set of returned ids.
async fn drain_ids(
container: &ContainerClient,
scope: FeedScope,
) -> Result<BTreeSet<String>, Box<dyn Error>> {
let mut iter = container
.query_items::<Doc>(Query::from("SELECT * FROM c"), scope, None)
.await?;
let mut got = BTreeSet::new();
while let Some(doc) = iter.try_next().await? {
got.insert(doc.id);
}
Ok(got)
}
#[tokio::test]
#[cfg_attr(
not(any(test_category = "emulator", test_category = "emulator_vnext")),
ignore = "requires test_category 'emulator' or 'emulator_vnext'"
)]
pub async fn feed_range_scope_restricts_cross_partition_query() -> Result<(), Box<dyn Error>> {
TestClient::run_with_unique_db(
async |run_context, db_client| {
let properties = ContainerProperties::new("ScopeBugContainer", pk_definition());
let container = run_context
.create_container(db_client, properties, None)
.await?;
// Seed DOC_COUNT documents, each on its own partition key.
let mut docs: Vec<Doc> = Vec::with_capacity(DOC_COUNT);
for i in 0..DOC_COUNT {
let id = format!("doc-{i:03}");
let doc = Doc {
id: id.clone(),
pk: id.clone(),
};
container
.create_item(PartitionKey::from(doc.pk.clone()), &id, doc.clone(), None)
.await?;
docs.push(doc);
}
// Compute each document's effective partition key locally (same hash
// the service uses), then sort ascending by EPK.
let pk_def = pk_definition();
let mut points: Vec<(String, FeedRange)> = docs
.iter()
.map(|d| {
let fr = FeedRange::for_partition(PartitionKey::from(d.pk.clone()), &pk_def);
(d.id.clone(), fr)
})
.collect();
points.sort_by(|a, b| a.1.min_inclusive().cmp(b.1.min_inclusive()));
let k = points.len();
// Control: a full-container scan must return every seeded document.
let all: BTreeSet<String> = points.iter().map(|(id, _)| id.clone()).collect();
let control = drain_ids(&container, FeedScope::full_container()).await?;
assert_eq!(
control, all,
"control full-container scan should return all {DOC_COUNT} docs"
);
// Test B: a WIDE interior window [X1, X_{k-1}) must exclude the
// globally smallest and largest EPKs => expect k-2 documents.
let window_b = FeedRange::new(
points[1].1.min_inclusive().clone(),
points[k - 1].1.min_inclusive().clone(),
)?;
let expected_b: BTreeSet<String> =
points[1..k - 1].iter().map(|(id, _)| id.clone()).collect();
let got_b = drain_ids(&container, FeedScope::range(window_b)).await?;
assert_eq!(
got_b,
expected_b,
"SCOPE BUG (wide window): FeedScope::range was ignored and the \
query fell back to a full scan. got {} docs, expected {}. \
unexpected(outside window)={:?}",
got_b.len(),
expected_b.len(),
got_b.difference(&expected_b).collect::<Vec<_>>()
);
// Test C: the TIGHTEST interior window [X_mid, X_{mid+1}) must
// return exactly one document.
let mid = k / 2;
let window_c = FeedRange::new(
points[mid].1.min_inclusive().clone(),
points[mid + 1].1.min_inclusive().clone(),
)?;
let expected_c: BTreeSet<String> = std::iter::once(points[mid].0.clone()).collect();
let got_c = drain_ids(&container, FeedScope::range(window_c)).await?;
assert_eq!(
got_c,
expected_c,
"SCOPE BUG (tight window): FeedScope::range was ignored. \
got {} docs, expected 1. unexpected(outside window)={:?}",
got_c.len(),
got_c.difference(&expected_c).collect::<Vec<_>>()
);
// Test D: a window that lies ENTIRELY in the gap between two adjacent
// EPKs must return zero documents. Appending a whole byte (`80`) to
// `X_mid`'s hex keeps `X_mid` as a byte prefix, so the result is
// strictly greater than `X_mid` and — because adjacent single-hash
// EPKs differ within their leading bytes — strictly less than
// `X_{mid+1}`. So `[X_mid || 0x80, X_{mid+1})` contains no doc's EPK.
// (A single hex nibble would be dropped by the byte-wise hex parser,
// so a full byte is required.) This guards against an off-by-one
// where the lower bound is treated as inclusive of `X_mid`.
let gap_start =
EffectivePartitionKey::from(format!("{}80", points[mid].1.min_inclusive().to_hex()));
let window_d = FeedRange::new(gap_start, points[mid + 1].1.min_inclusive().clone())?;
let got_d = drain_ids(&container, FeedScope::range(window_d)).await?;
assert!(
got_d.is_empty(),
"SCOPE BUG (empty gap window): a window between adjacent EPKs \
returned {} docs, expected 0. unexpected={:?}",
got_d.len(),
got_d.iter().collect::<Vec<_>>()
);
Ok(())
},
Some(TestOptions::for_emulator()),
)
.await
}Happy to discuss any of this. |
Resolve the `clippy::large-futures` CI failures in the in-memory HPK tests by boxing the query pipeline futures inside the `query_scope` helper, so callers awaiting it stay under the lint's stack-size threshold (one fix instead of six `Box::pin` call sites). Address a review nit (Fabian): hoist the `EffectivePartitionKeyRange` `x-ms-read-key-type` value into a shared `request_header_names::READ_FEED_KEY_TYPE_EPK_RANGE` constant and use it in the driver's `apply_headers`, its unit test, and the in-memory emulator dispatch guard instead of repeating the string literal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fold Ananth's standard-gateway (single-hash `/pk`) end-to-end regression test into cosmos_feed_ranges.rs rather than adding a separate file. It seeds one document per partition key, computes each doc's effective partition key locally, and asserts that FeedScope::range(interior_window) returns exactly the windowed subset across wide, tight, and empty-gap windows. This exercises a path not covered by the in-memory HPK tests or the GW2.0 query tests: it fails on pre-fix main with "SCOPE BUG" (full-scan fallback) and passes once both the planner scope-clip and the x-ms-read-key-type: EffectivePartitionKeyRange header fix are in. drain_ids drains page-by-page via into_pages() instead of item-by-item so the query future stays under the clippy::large-futures threshold. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks @ananth7592 — appreciate you closing #4726 in favor of this, and the regression test is a great addition. It exercises a path neither the in-memory HPK tests nor the GW2.0 query tests cover, so I've folded it straight in rather than taking it as a follow-up (least noise for everyone). It now lives in |
Now that #4729 (merged from main) fixes the HPK prefix (#4680) and cross-partition (#4681) query bugs, flip the ignores the PR deferred to this coverage branch: - hpk_query_cross_partition_full_container (#4681): enabled on both the classic and vnext emulators. Verified passing against the classic emulator locally. - hpk_query_prefix_level1/level2/no_match/correctness_guard (#4680): gated to run only on the vnext emulator (test_category=emulator_vnext). The classic Cosmos emulator rejects the prefix EPK-range query that #4729 emits with 400 BadRequest, so these stay ignored on the blocking classic legs. The prefix-filtering fix is already covered deterministically in CI by in_memory_emulator_tests::hpk. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add EffectivePartitionKey::normalized_successor(len), which zero-extends a trailing-zero-trimmed EPK to the partition key definition's full width (16 * paths().len() for V2 hash/MultiHash) before the big-endian increment, matching the .NET SDK. successor() is refactored onto a shared increment_be() helper and keeps its width-preserving (Java-parity) behavior. Add normalized_epk_len(&PartitionKeyDefinition) and wire the planner's closed non-point range upper bound to normalize before becoming exclusive. Equality/IN points still route whole-partition (Option A) on this branch; wiring the Option B [X, successor(X)) window is deferred until Azure#4729's EffectivePartitionKeyRange read-key-type header lands here (otherwise the window would 400). Adds 7 unit tests; driver lib tests pass, clippy clean. Relates to Azure#4574 / Azure#4638. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ow (Option B, Azure#4574/Azure#4638) Normalize an equality/IN query-plan point [X, X] to the half-open window [X, successor(X)) in query_range_to_feed_range and let it flow through the general half-open routing path (scope intersection, topology resolve, per-partition start/end-epk emit with x-ms-read-key-type: EffectivePartitionKeyRange), instead of special-casing points to whole-partition routing. Matches the .NET normalize-to-EPK-range model endorsed by Fabian/Ashley and relies on the Azure#4729 read-key-type header fix. Updates the 6 point-routing tests to assert per-value EPK windows (fresh + resume; IN continuations are now per-window) and adds a CHANGELOG Bugs Fixed entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add EffectivePartitionKey::normalized_successor(len), which zero-extends a trailing-zero-trimmed EPK to the partition key definition's full width (16 * paths().len() for V2 hash/MultiHash) before the big-endian increment, matching the .NET SDK. successor() is refactored onto a shared increment_be() helper and keeps its width-preserving (Java-parity) behavior. Add normalized_epk_len(&PartitionKeyDefinition) and wire the planner's closed non-point range upper bound to normalize before becoming exclusive. Equality/IN points still route whole-partition (Option A) on this branch; wiring the Option B [X, successor(X)) window is deferred until Azure#4729's EffectivePartitionKeyRange read-key-type header lands here (otherwise the window would 400). Adds 7 unit tests; driver lib tests pass, clippy clean. Relates to Azure#4574 / Azure#4638. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ow (Option B, Azure#4574/Azure#4638) Normalize an equality/IN query-plan point [X, X] to the half-open window [X, successor(X)) in query_range_to_feed_range and let it flow through the general half-open routing path (scope intersection, topology resolve, per-partition start/end-epk emit with x-ms-read-key-type: EffectivePartitionKeyRange), instead of special-casing points to whole-partition routing. Matches the .NET normalize-to-EPK-range model endorsed by Fabian/Ashley and relies on the Azure#4729 read-key-type header fix. Updates the 6 point-routing tests to assert per-value EPK windows (fresh + resume; IN continuations are now per-window) and adds a CHANGELOG Bugs Fixed entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add EffectivePartitionKey::normalized_successor(len), which zero-extends a trailing-zero-trimmed EPK to the partition key definition's full width (16 * paths().len() for V2 hash/MultiHash) before the big-endian increment, matching the .NET SDK. successor() is refactored onto a shared increment_be() helper and keeps its width-preserving (Java-parity) behavior. Add normalized_epk_len(&PartitionKeyDefinition) and wire the planner's closed non-point range upper bound to normalize before becoming exclusive. Equality/IN points still route whole-partition (Option A) on this branch; wiring the Option B [X, successor(X)) window is deferred until Azure#4729's EffectivePartitionKeyRange read-key-type header lands here (otherwise the window would 400). Adds 7 unit tests; driver lib tests pass, clippy clean. Relates to Azure#4574 / Azure#4638. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ow (Option B, Azure#4574/Azure#4638) Normalize an equality/IN query-plan point [X, X] to the half-open window [X, successor(X)) in query_range_to_feed_range and let it flow through the general half-open routing path (scope intersection, topology resolve, per-partition start/end-epk emit with x-ms-read-key-type: EffectivePartitionKeyRange), instead of special-casing points to whole-partition routing. Matches the .NET normalize-to-EPK-range model endorsed by Fabian/Ashley and relies on the Azure#4729 read-key-type header fix. Updates the 6 point-routing tests to assert per-value EPK windows (fresh + resume; IN continuations are now per-window) and adds a CHANGELOG Bugs Fixed entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Fixes two hierarchical-partition-key (HPK / MultiHash) query bugs that were deferred out of #4705:
FeedScope::partitionscope with only a prefix of the key hierarchy (e.g.("USA", "CA")on a/country/state/citycontainer) returned every item in the physical partition instead of filtering to the prefix.400 Bad Request("One of the input values is invalid") and an emptyPartitionKeyRangeId.Root cause (unified)
Both bugs share a single defect. When the driver scopes a data-plane request to an effective-partition-key (EPK) range, it emits
x-ms-start-epk/x-ms-end-epkalongsidex-ms-read-key-type. The value was hardcoded to the point key-typeEffectivePartitionKey, which the gateway rejects for range-scoped requests. It must be the range key-typeEffectivePartitionKeyRange.This was validated end-to-end against a live Cosmos account (raw HTTP + the Python
azure-cosmosSDK) and cross-checked against the .NET SDK (azure-cosmos-dotnet-v3), whoseRequestInvokerHandlerand Gateway 2.0 thin-client path emit the identical header set.Changes
azure_data_cosmos_driver/src/driver/pipeline/operation_pipeline.rs— emitx-ms-read-key-type: EffectivePartitionKeyRangefor EPK range-scoped requests; updated the corresponding unit test.azure_data_cosmos_driver/src/driver/dataflow/planner.rs— scope a prefix HPK fan-out to[prefix_epk, prefix_epk + "FF")(prefix_epk_range/clip_to_prefix) so a prefix query only fans out over — and is EPK-filtered within — the physical partitions its completions can live in, instead of scanning whole partitions.azure_data_cosmos/src/feed/query.rs— documented prefix support onFeedScope::partition.azure_data_cosmos/tests/in_memory_emulator_tests/hpk.rs(new) — CI-runnable SDK-level tests covering both bugs.azure_data_cosmosandazure_data_cosmos_driver.Validation
cargo fmt+cargo clippyclean.[epk, epk+"FF")range with the0x3F-masked-nibbleFFsentinel, exact-match pkrangeid collapse, multi-partition prefix fan-out, and change-feed sharing the same path all match).Note
The canonical
#[ignore]d emulator tests incosmos_hpk.rsland via the separate #4155 coverage branch; flipping those ignores is deferred to that branch's merge to avoid duplication.