Cosmos: Adds Binary Encoding Support for Queries and Streaming order_by - #5040
Draft
Debdatta Kunda (kundadebdatta) wants to merge 22 commits into
Draft
Cosmos: Adds Binary Encoding Support for Queries and Streaming order_by#5040Debdatta Kunda (kundadebdatta) wants to merge 22 commits into
order_by#5040Debdatta Kunda (kundadebdatta) wants to merge 22 commits into
Conversation
Queries now advertise a binary response via x-ms-cosmos-supported-serialization-formats while keeping their application/query+json request body as text. Splits the driver binary gate into request-body encoding (point item ops) and response negotiation (item ops + query), sets the header at the plan_operation choke point every query page flows through, wires binary resolution into query_items, and honors the negotiation in the in-memory emulator feed responses. Adds a driver unit test and an emulator end-to-end binary query round-trip test, and updates the binary-encoding SPEC/HLD docs.
|
Azure Pipelines: Successfully started running 1 pipeline(s). 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Adds an in-memory-emulator test that runs a full-container SELECT * over a 3-partition container with binary encoding enabled, proving the passthrough cross-partition query path round-trips binary Documents envelopes per page with no additional code beyond response negotiation.
added 13 commits
August 10, 2026 18:59
Records a per-operation comparison of Cosmos binary JSON support (request encode / response negotiate / response decode) between the Rust SDK+driver and azure-cosmos-dotnet-v3, including the ORDER BY/aggregate query-engine gap, the Delete negotiation divergence, the patch mechanism difference, and the header-value nuance, with source references on both sides.
Make the streaming ORDER BY merge binary-aware: parse_envelope_page now transcodes a binary-negotiated page to text before the envelope parse (no-op for text). Adds unit + integration coverage, extends the e2e round-trip fuzzer with single-partition and cross-partition ORDER BY query round-trips, and updates the .NET parity doc.
… gaps Addresses re-review findings #3 (correctness), #8, #9, #10. #3 — binary ORDER BY lost the native integral-Double->integer coercion that passthrough binary queries get, so a typed model with a wide integer field could round-trip through a passthrough query but fail on an ORDER BY query for the same document (the text/binary divergence, #5028). The streaming merge transcoded each binary page to text and rebuilt a *text* envelope, so the SDK decoded it with the text deserializer (which hard-fails on a service-echoed integral double for an integer field). - PageAggregator now tracks whether the source pages were binary and, when so, re-encodes the assembled envelope to Cosmos binary JSON in build_page, so the SDK's binary deserializer runs its integral-Double->integer coercion — matching passthrough. Text sources keep the zero-copy text envelope. - New unit tests prove the emitted format follows the source and that an integral-Double u64 payload now decodes (and would fail as text). - ids_in_page test helper is now format-aware (transcodes binary pages). - Live fuzzer: the typed IntProbe now also decodes through a cross-partition binary ORDER BY, exercising the merge coercion end to end. #8 — binary_cross_partition_query_round_trips could pass even if negotiation silently broke (text decodes fine). build_multi_partition_container now attaches a QueryRequestRecorder and the test asserts every fan-out page advertised a binary response with a text body. #9 — added binary_cross_partition_order_by_merges_and_round_trips: an always-run emulator test that exercises the real k-way merge over binary pages (previously only a mocked driver test + the live-only fuzzer covered it). #10 — the fuzzer's cross-partition ORDER BY fan-out (the most expensive query shape) now runs only on the binary configs, where it adds coverage; the single-partition passthrough query still runs on every config.
A - BINARY_NEGOTIATION_FORMATS doc comment was written in #4671 and never updated when this PR wired query negotiation. It claimed the constant applied 'on point operations' and that query negotiation was 'not yet wired' - the exact thing this PR ships. Rewrote it to cover point ops + query and to state explicitly why Rust forces CosmosBinary for query (vs .NET's JsonText,CosmosBinary), matching the SPEC. B - BINARY_ENCODING_SPEC.md listed 'delete' in the request-body gate in two places, but the code (supports_binary_request_body) and its unit test exclude delete. Dropped delete from both lists and noted the .NET divergence (.NET's IsPointOperationSupportedForBinaryEncoding does include delete). C - the rewritten query unit test only re-asserted two booleans already covered elsewhere and lost the behavioral guarantee its predecessor had. Replaced it with a behavioral test that drives a real query operation through apply_response_negotiation (the actual header owner) and asserts the application/query+json body stays text while the response advertises binary.
#7 - avoid resolving the binary-encoding options view twice per point op. execute_operation already resolves BinaryEncodingOptions for the request-body gate, but plan_operation -> apply_response_negotiation re-resolved the same layered view. Thread the resolved value through a private plan_operation_resolved into apply_response_negotiation; the public plan_operation (and the query path, which reaches the driver there directly) passes None and resolves lazily as before. Also corrected the now-stale comment claiming execute_operation sets the negotiation header (it no longer does after #6 - apply_response_negotiation owns it). #12 - replace the bare positional 'binary: bool' on success_feed_response / success_document_feed_response with a ResponseFormat { Text, Binary } enum, so the four read-feed/change-feed call sites read ResponseFormat::Text instead of a naked 'false' that a future edit could transpose (restores #4733's positional-creep guard). Query call sites use ResponseFormat::from( parsed.binary_response). Also documented the emulator's binary-response fidelity note in dispatch.rs (derives the flag from the header alone vs the real gateway honoring it only for Query - unreachable since Rust only advertises binary for point ops + query).
- #1 (blocking): make the binary-emit flag sticky on StreamingOrderedMerge so buffer-only pages (no backend fetch) still emit binary; a per-page flag left them text with float-widened integers that failed typed decode. + regression test. - #2: scope the BINARY_NEGOTIATION_FORMATS doc comment to note request_text_response is honored only for point ops, not queries. - #4: correct the stale into_items splitter comment (real reason it stays inert). - #11: assert query-plan requests carry no binary header instead of skipping them. - #14: move ResponseFormat below success_response_with_format to fix its orphaned doc. - #8: add CHANGELOG entries (SDK + driver) for query binary negotiation.
…ding_for_query # Conflicts: # sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs # sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/integration_tests/order_by_resume.rs # sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs
assert_query_roundtrip gained an 8th parameter (the ORDER-BY gate) that trips clippy::too_many_arguments under -Dwarnings. Bundling the args would only move the count into assert_query_hit/assert_roundtrip, so allow it on this test-only helper.
Closes the remaining review findings on binary response negotiation for queries, and covers two gaps the review surfaced. Correctness: - split_feed_envelope now handles Cosmos binary pages. OFFSET/LIMIT and TOP route through the SkipTake node, whose splitter was text-only, so those queries hard-failed with binary enabled. - A query with request_text_response no longer negotiates binary. Queries bypass the execute_operation transcode, so negotiating would hand a text-requesting caller binary pages. - Response negotiation no longer clobbers a caller-set format header. - emit_binary is promoted on the merge fill-error path and OR-assigned at the page bottom, so a sticky binary flag cannot be cleared. - build_page transcode failures are classified as SERIALIZATION_RESPONSE_BODY_INVALID (client-side re-encode) rather than a 500 service error, and carry the item ordinal. Tests: - End-to-end binary OFFSET/LIMIT and TOP round-trip against the emulator, verified load-bearing by mutation. - Byte-level query response assertions for both CosmosBinary and JsonText. - A binary-disabled query advertises no serialization format. - The sampled-corpus test now issues passthrough and ORDER BY queries per document; it previously only exercised create and read, so it was not validating query binary support at all. - Removed two vacuous assertions (empty-page format checks, unverified request bodies). Docs: - The Gateway 2.0 / thin-client text fallback is documented as a customer-visible limitation in both the SPEC and the HLD; the repeated per-document transcode is recorded as deferred work. - Added a comment-brevity convention to AGENTS.md and trimmed the verbose comment blocks this PR had accumulated.
Replace the "Deferred work" prose section with "Binary encoding support status": a per-operation table (request encode / response negotiate / response decode) and a numbered pending-work table with severity and size. Corrections: - The deferred-work note recommended slicing a document out of a binary page and re-prefixing it with 0x80. That is unsound: reference strings (STR_R1-STR_R4) resolve against absolute page offsets and the interning scope is the whole page, so a detached sub-slice mis-resolves any reference pointing outside it, silently returning wrong text. Replaced with a view-based design (refcounted page Bytes plus an offset). - The "binary feed responses" bullet claimed the feed splitter is text-only and cannot handle binary envelopes. split_feed_envelope handles them as of this branch. Rewritten as the invariant future splitters must keep. - Aggregate / GROUP BY / DISTINCT were listed as a binary gap. They are rejected cross-partition in any encoding, so they are blocked on the query engine rather than pending binary work. Restore the Rust vs .NET parity matrix (dropped in e7af8fda59) as a section here rather than a separate internal doc, updated for this branch: TOP / LIMIT / OFFSET now ship, and the Gateway 2.0 row carries the customer-visible framing instead of "still decodes".
order_by
Adds a live text-vs-binary query comparison over the sampled corpus and fixes the two real defects it caught. GUID-shaped strings decoded with their hex nibbles transposed. Each byte packs two hex characters low nibble first (the same 4-bit packing the table-string forms use), but the GUID string reader emitted the high nibble first, so every LowercaseGuidString, UppercaseGuidString, and the DoubleQuotedLowercaseGuidString form the service uses for _etag decoded to a wrong-but-well-formed GUID. The form had zero coverage: the writer never emits it, so no round-trip vector or fuzzer path reached it. The golden vectors encoded the same wrong assumption and are corrected; a service-captured _etag vector is added so this cannot regress silently. read_guid_value (marker 0xD3) is unchanged: it is a genuine .NET Guid mixed-endian layout, not the same encoding. Doubles lost up to 1 ULP when a text JSON body was parsed, which corrupted values on the binary ORDER BY path, whose merge re-encodes each payload text to binary. serde_json's default float parser is not correctly rounded, so a stored 96.182417728091792 came back as 96.1824177280918. Enabling the float_roundtrip feature makes parsing bit-exact. The comparison test seeds documents from the corpus, then runs four query shapes (passthrough, ORDER BY, ORDER BY with OFFSET/LIMIT, and TOP) under both encodings and asserts they agree item for item, also reporting page-count and RU deltas. Seeding strips service-owned _-prefixed fields and asserts a point read round-trips each document.
Binary encoding's value was being argued from intuition. This adds `binary_payload_ab`, which measures it against a live account instead. The harness installs a custom `TransportClient` via the driver's `with_mock_http_client_factory` hook, so bytes are counted at the socket boundary: request bodies after transcoding, response bodies before it. Text and binary arms are interleaved round by round so network drift and throttling hit both equally, and every arm sees byte-identical documents. Workloads cover point create/replace/upsert/delete/read plus `SELECT *`, `ORDER BY`, and a narrow projection. Created documents are deleted before the queries run, so container size is constant across arms and per-item byte counts stay comparable. Documents come from four `--profile` shapes. `corpus` samples the 29 real-world JSON files under `testdata/`, which matters because shapes chosen by the harness author would invite the objection that the result was tuned. All profiles are seeded, so runs reproduce exactly. Two subtleties the harness has to get right, both learned by getting them wrong first: document ids travel inside the request body, so mode tags are fixed-width (`m0`/`m1`/`m2`) or id length contaminates the request-byte comparison; and writes stride across the whole seeded set, since sending one fixed document measures a shape the reads never see. `BINARY_ENCODING_RESULTS.md` records two runs. Payload and RU savings reproduce tightly (queries -47% bytes, -12% to -15% RU). Latency largely does not: the `binary+text_resp` arm returns byte-identical bytes to text on queries, making it a free noise control, and it moved +18% - so most apparent latency wins sit inside the noise floor. The doc says so. The corpus itself is gitignored. It is ~500 MB of local data that the sampled integration test already documents as untracked. `binary_sampled_testdata.rs` gets a doc-comment correction: client-level binary options set the default that the seed phase writes under, and per-operation options take precedence, which is what makes the two comparison arms differ.
Addresses review feedback on the binary query encoding work. The emit format was previously inferred by sniffing each response body for the binary preamble and then latched onto a mutable flag on the merge. That flag was promoted only on the success path, so an early error return could leave a page emitted in the wrong encoding, and the inference could disagree with what was actually negotiated. The format is now read once from the operation via negotiates_binary_response and fixed at construction, which makes the inconsistent state unrepresentable rather than merely unreached. Normalization is consolidated behind normalize_page_body so every consumer shares one choke point. Text passes through as a refcount bump and binary transcodes, and a failure there is reported as SERIALIZATION_RESPONSE_BODY_INVALID with the underlying error retained as the source instead of being dropped. Integral doubles now render as integers when transcoding to text. The service emits a stored 5.0 as 5, so without this the binary ORDER BY path produced resumeFilter bytes that differed from the text path for the same query. The existing envelope test could not catch this because OrderByItem compares numerically across Number variants; the new test compares the rendered filter bytes instead. Also covers the two gaps the review called out: that request_text_response forfeits binary for queries but not for point operations, and that a binary ORDER BY resumed from a continuation stays binary and keeps its order.
Condenses over-long doc and inline comments added by the binary encoding work down to the point being made, and drops a few that restated the code beneath them. Also corrects a test doc that claimed a resumed session re-derives its emit format from backend pages, which the negotiation-derived format removed.
Fifteen findings were raised against the binary query encoding work. Fourteen held up; the fifteenth was half wrong (see below). Correctness: - `StreamingOrderedMerge::build_page` propagated its error with a bare `?`, skipping the `continuation_unsafe` latch every other error path in that function sets. By that point the emitted rows have already had their boundaries advanced and drained children evicted, so the continuation snapshot can no longer describe them -- resuming from it would silently skip rows. This is the third recurrence of the bug class the latch was introduced for. - `normalize_integral_floats` coerced `-0.0` to `0`, erasing a sign the service round-trips. - `SkipTake` decided its emitted encoding by sniffing the bytes it received while the ordered merge derived the same decision from the negotiated operation. When the service answers a binary-negotiated query in text, the two nodes disagreed. `split_feed_envelope` now takes `emit_binary` from `CosmosOperation::negotiates_binary_response`, which its own docs already claim is the authority. Test quality -- four tests could not fail: - The `build_page` binary test hand-fed a float payload straight to `PageAggregator`, bypassing the `normalize_page_body` transcode that is the thing under test. Replaced with a test that drives a real binary envelope through `parse_envelope_page`, and asserts exact bytes plus a typed `u64` decode at 2^53. - The emulator query-plan assertion sat inside a `for` loop over a vector with no non-empty guard. The vector turns out to be non-empty, so this was a latent gap rather than a live bug. - `json_equivalent` compared numbers through `as_f64`, so the wide integers the fuzzer exists to cover compared equal after losing precision. Now exact via `i128`, with a bit-compare for fractionals that also keeps `-0.0` distinct. Stale prose. Several rationale comments described code that had since changed: the `build_page` justification rested on a premise `normalize_page_body` had already made false, `skip_take_page` promised byte-for-byte identity that only the text path delivers, one comment claimed a status was "not 500" when it is exactly that, and the perf crate claimed `publish = false` isolates `__internal_mocking` when resolver v2 unifies it workspace-wide (`cargo tree --workspace -e features -i azure_data_cosmos_driver` shows the feature enabled). The CHANGELOG's GUID example had transposed nibbles, and the point-op rendering change -- an integral `Double` now transcodes as `3`, not `3.0` -- was undocumented despite being user-visible. The reviewer objected that the driver is accruing serialization duties against the schema-agnostic contract in sdk/cosmos/AGENTS.md. The contract was overstated: the driver cannot re-order documents across pages and rebuild an envelope over opaque bytes. AGENTS.md now draws the line at driver-owns-envelope / SDK-owns-item, and warns against the byte-sniffing this commit removes. That is a judgement call and is the part of this commit most worth arguing with. Two corrections to the findings themselves. The claim that a `.gitignore` change excluded `testdata/*.json` from CI is wrong -- those files were never tracked, so nothing changed. The fuzz budget concern is real but unresolved: 200 iterations is ~3400 round-trips against a leg already using ~65 of its 90 minutes, and I could not time it without a live run, so the comment now says so instead of implying the number was validated. Also folds two further A/B measurement runs into BINARY_ENCODING_RESULTS.md; the headline delta moved several points between runs, which is worth knowing before quoting it.
Two of the nine reported words were British spellings I introduced in the previous commit -- `recognised` and `neighbours`. Those are corrected to US spelling rather than added to a dictionary, since suppressing them would let the inconsistency spread. `binaryab` (a CLI default value), `subnormals` (IEEE-754) and `multibyte` are legitimate technical terms and go in the cosmos dictionary. `testdata` was already in that dictionary. The failure was a config-scope artifact: the ignore rule sat in the *root* `.gitignore`, where only the root cSpell config applies and cosmos-specific words are not in scope. Moved the rule to `sdk/cosmos/.gitignore`, which is both where a cosmos-crate artifact belongs and where the existing dictionary entry takes effect. Verified `git check-ignore` still matches the path. The repo's `Invoke-Cspell.ps1` requires Node >=22.18.0, which this machine does not have, so verification ran cspell 8 directly against `.vscode/cspell.json`: 0 issues across the six affected files. CI uses cspell 10; a version difference is unlikely to matter for plain dictionary lookups but was not exercised locally.
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.
Extends Cosmos binary JSON (the
0x80-preamble wire format) from point operations to the query path. Previously aquery_itemscall always received text pages, even with binary encoding enabled — which meant queries kept the integral-Double→ integer deserialization divergence (#5028) that binary encoding exists to fix.Opt-in and off by default; with the flag unset, behavior is byte-for-byte unchanged.
What changed
Negotiation. Queries now advertise
x-ms-cosmos-supported-serialization-formats: CosmosBinary, set once at theplan_operationchoke point that every per-page request flows through. An explicitly caller-set header is never clobbered, andrequest_text_responsestill forces text. The query request body stays text by design —application/query+jsonis a query spec, not a document.All three query pipelines handle binary pages:
into_singleparse_envelope_pagedecodes binary envelopes; merged items emitted as binaryOFFSET/LIMIT/TOPsplit_feed_envelopesplits binary pages into per-document binaryOFFSET/LIMIT/TOPwas a blocker: cross-partition skip/take failed outright on binary pages before this.Emitting merged ORDER BY items as binary is semantically load-bearing, not cosmetic. The binary deserializer coerces a service-echoed integral
Doubleinto an integer target; the text deserializer hard-fails on it. Emitting text would reintroduce the exact divergence for ORDER BY that passthrough queries no longer have.How a binary query page flows
sequenceDiagram participant SDK participant D as Driver participant S as Service SDK->>D: query_items (binary enabled) D->>S: page request + CosmosBinary header S-->>D: binary page (0x80 preamble) alt ORDER BY D->>D: decode envelope, merge on sort keys D->>D: re-encode merged items to binary else OFFSET / LIMIT / TOP D->>D: split envelope, re-encode each document standalone else passthrough D->>D: forward page unchanged end D-->>SDK: binary items SDK->>SDK: decode (auto-detect by preamble)Every producer emits each document standalone-encoded, so
into_itemsauto-detects format per item by preamble.Testing
0x80body and a text query does not