From c8078d53cd7fc57f7c4c7c71be92dff5a7e6884a Mon Sep 17 00:00:00 2001 From: kundadebdatta Date: Mon, 10 Aug 2026 14:26:08 -0700 Subject: [PATCH 01/13] feat(cosmos): negotiate binary response for queries 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. --- .../src/clients/container_client.rs | 12 +- .../binary_round_trip.rs | 165 ++++++++++++ .../docs/BINARY_ENCODING_HLD.md | 7 +- .../docs/BINARY_ENCODING_SPEC.md | 22 +- .../src/driver/cosmos_driver.rs | 237 ++++++++++++++---- .../src/in_memory_emulator/operations.rs | 15 +- .../src/models/mod.rs | 29 ++- 7 files changed, 427 insertions(+), 60 deletions(-) diff --git a/sdk/cosmos/azure_data_cosmos/src/clients/container_client.rs b/sdk/cosmos/azure_data_cosmos/src/clients/container_client.rs index 6bbd9c0628..52b8b5d883 100644 --- a/sdk/cosmos/azure_data_cosmos/src/clients/container_client.rs +++ b/sdk/cosmos/azure_data_cosmos/src/clients/container_client.rs @@ -868,6 +868,14 @@ impl ContainerClient { let options = options.unwrap_or_default(); let query = query.into(); + // Resolve binary encoding so the driver advertises a binary *response* + // via the negotiation header. Unlike point item writes, the query + // request body stays text (`application/query+json` is a query spec, + // not a document), so we do not touch body serialization here — the + // driver's request-body gate excludes query. + let (operation_options, _binary) = + resolve_binary_encoding(options.operation, &self.context.binary_encoding); + let container_ref = self.container_ref.clone(); // The first operation to execute in the query items flow. @@ -894,7 +902,7 @@ impl ContainerClient { .driver .plan_operation( initial_operation, - &options.operation, + &operation_options, options.feed.continuation_token.as_ref(), &options.feed.to_plan_options(), ) @@ -903,7 +911,7 @@ impl ContainerClient { self.context.driver.clone(), Some(self.container_ref.clone()), plan, - options.operation, + operation_options, self.context.diagnostics_handlers.clone(), self.operation_context("query_items"), )) diff --git a/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs b/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs index 8db61fc8da..53f9c0d2ac 100644 --- a/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs +++ b/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs @@ -19,11 +19,13 @@ use azure_data_cosmos::{ RoutingStrategy, }, AccountEndpoint, AccountReference, ContainerClient, CosmosClientBuilder, CosmosRuntimeBuilder, + FeedScope, Query, }; use azure_data_cosmos_driver::in_memory_emulator::{ ConsistencyLevel, ContainerConfig, InMemoryEmulatorHttpClient, RequestObserver, VirtualAccountConfig, VirtualRegion, }; +use futures::TryStreamExt; use serde::{Deserialize, Serialize}; use std::sync::{Arc, Mutex}; @@ -345,3 +347,166 @@ async fn request_text_response_keeps_wire_binary_and_returns_data() { ); } } + +/// A [`RequestObserver`] that records, for each query request (`Content-Type: +/// application/query+json`), the advertised negotiation header and whether the +/// request body was Cosmos binary JSON (first byte `0x80`). Lets a test assert +/// that a query advertises a binary *response* while keeping its request body +/// text. +#[derive(Debug, Default)] +struct QueryRequestRecorder { + negotiation_formats: Mutex>>, + body_is_binary: Mutex>, +} + +impl RequestObserver for QueryRequestRecorder { + fn on_request(&self, request: &azure_core::http::Request) { + let content_type = request + .headers() + .get_optional_str(&azure_core::http::headers::HeaderName::from_static( + "content-type", + )) + .map(|s| s.to_string()); + if content_type.as_deref() != Some("application/query+json") { + return; + } + // The query-plan request shares the `application/query+json` content type + // but is metadata (no data negotiation); skip it so only the data query + // is asserted on. + if request + .headers() + .get_optional_str(&azure_core::http::headers::HeaderName::from_static( + "x-ms-cosmos-is-query-plan-request", + )) + .is_some() + { + return; + } + let formats = request + .headers() + .get_optional_str(&azure_core::http::headers::HeaderName::from_static( + "x-ms-cosmos-supported-serialization-formats", + )) + .map(|s| s.to_string()); + self.negotiation_formats.lock().unwrap().push(formats); + + let is_binary = match request.body() { + azure_core::http::request::Body::Bytes(bytes) => bytes.first() == Some(&0x80), + _ => false, + }; + self.body_is_binary.lock().unwrap().push(is_binary); + } +} + +/// With binary enabled, a `query_items` call advertises a binary **response** +/// (`x-ms-cosmos-supported-serialization-formats: CosmosBinary`) while keeping +/// its `application/query+json` request body as text; the emulator honors the +/// negotiation and replies with a binary feed body, which the SDK auto-detects +/// and decodes — so the queried documents round-trip intact. +#[tokio::test] +async fn binary_query_negotiates_response_and_round_trips() { + let config = VirtualAccountConfig::new(vec![VirtualRegion::new( + "East US", + azure_core::http::Url::parse(EMULATOR_GATEWAY_URL).unwrap(), + )]) + .unwrap() + .with_consistency(ConsistencyLevel::Session); + + let recorder = Arc::new(QueryRequestRecorder::default()); + let emulator = Arc::new( + InMemoryEmulatorHttpClient::new(config) + .with_request_observer(Arc::clone(&recorder) as Arc), + ); + let store = emulator.store(); + store.create_database("bin-query"); + store.create_container_with_config( + "bin-query", + "items", + serde_json::from_value(serde_json::json!({ + "paths": ["/pk"], + "kind": "Hash", + "version": 2 + })) + .unwrap(), + ContainerConfig::new() + .with_partition_count(1) + .with_throughput(400) + .build() + .unwrap(), + ); + + let account = AccountReference::with_authentication_key( + EMULATOR_GATEWAY_URL.parse::().unwrap(), + azure_core::credentials::Secret::new("dGVzdGtleQ=="), + ); + let client = CosmosClientBuilder::new() + .with_binary_encoding_options(BinaryEncodingOptions::new().with_enabled(true)) + .with_runtime( + CosmosRuntimeBuilder::from(emulator.runtime_builder()) + .build() + .await + .unwrap(), + ) + .build(account, RoutingStrategy::ProximityTo(Region::EAST_US)) + .await + .unwrap(); + let container = client + .database_client("bin-query") + .container_client("items") + .await + .unwrap(); + + let items = vec![ + TestItem { + id: "q-1".into(), + pk: "pk1".into(), + value: 10, + note: "café ☃".into(), + }, + TestItem { + id: "q-2".into(), + pk: "pk1".into(), + value: 20, + note: "second".into(), + }, + ]; + for item in &items { + container + .create_item("pk1", &item.id, item, Some(write_options_with_content())) + .await + .unwrap(); + } + + let iter = Box::pin(container.query_items( + Query::from("SELECT * FROM c ORDER BY c.value"), + FeedScope::partition("pk1"), + None, + )) + .await + .unwrap(); + let mut results: Vec = Box::pin(iter.try_collect()).await.unwrap(); + results.sort_by_key(|d| d.value); + + assert_eq!( + results, items, + "query results must round-trip through binary" + ); + + // The query advertised a binary response but kept its body text. + let formats = recorder.negotiation_formats.lock().unwrap(); + assert!(!formats.is_empty(), "expected at least one query request"); + for value in formats.iter() { + assert_eq!( + value.as_deref(), + Some("CosmosBinary"), + "query must advertise a binary response", + ); + } + let body_is_binary = recorder.body_is_binary.lock().unwrap(); + for is_binary in body_is_binary.iter() { + assert!( + !is_binary, + "query request body must stay text (application/query+json is a spec, not a document)", + ); + } +} diff --git a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_HLD.md b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_HLD.md index 678862f086..722967ef1c 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_HLD.md +++ b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_HLD.md @@ -22,7 +22,7 @@ Because the option lives on the driver and is schema-agnostic, the driver perfor A self-contained, in-tree **end-to-end validation loop** is included via the in-memory emulator (no Docker, no live account, no external test vectors). -> **Scope:** item operations (`create` / `replace` / `upsert` / `read`). Query, patch, transactional batch, and bulk are intentionally deferred (see [Deferred work](#deferred-work)). +> **Scope:** item operations (`create` / `replace` / `upsert` / `read`) encode request bodies and decode responses; `query` negotiates a binary response (its `application/query+json` request body stays text). Patch, transactional batch, and bulk are intentionally deferred (see [Deferred work](#deferred-work)). --- @@ -394,8 +394,9 @@ opts.binary_encoding_request_text_response = 2; /* 2 = true */ ## Deferred work -* **Query binary negotiation** — the request-body encoding + negotiation for query pages is still deferred. Binary encoding now lives on `OperationOptions.binary_encoding`, so a driver-minted page operation *can* carry it; what remains is confirming query-body semantics (`application/query+json`) and the native cross-partition query engine's handling of binary item bytes. The query *response* decode already works via the shared choke point. -* **Binary feed responses** — the feed splitter scans **text** JSON, so binary `Documents` envelopes cannot be sliced yet; making it binary-aware is a prerequisite for any feed/query binary negotiation. +* **Query response negotiation** — **done.** A `query_items` call now advertises a binary response via `x-ms-cosmos-supported-serialization-formats` (set once at the `plan_operation` choke point that every per-page request flows through). The query request body is a `application/query+json` spec, not a document, so it intentionally stays text — there is no query request-body encoding. The query *response* decodes via the shared choke point. +* **Cross-partition / ORDER BY merge on binary bytes** — the SDK's `parse_envelope_page` merge path parses page envelopes with text-only `serde_json` + `RawValue`, so cross-partition ordering over *binary* item bytes is not yet covered. Single-partition and `into_single` query drains already round-trip binary end to end. +* **Binary feed responses** — the `into_items` feed splitter scans **text** JSON, so binary `Documents` envelopes cannot be sliced by that splitter yet. (The SDK query path uses `into_single`, which is unaffected.) `ReadFeed` / change feed is excluded from binary negotiation: the backend does not honor the header for it. * **`patch`** — excluded from binary encoding for now (the driver's request-side encode intentionally skips patch); transactional `batch` / `bulk` are deferred by spec. * **Cross-implementation vectors** — validate against captured real .NET / Java binary output. diff --git a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_SPEC.md b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_SPEC.md index 462001d07c..66e90af99a 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_SPEC.md +++ b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_SPEC.md @@ -68,13 +68,16 @@ text-equivalent results. | ------------------------------------------- | :----------: | :-----------: | --------- | | `read_item` | — | decode | ✅ done | | `create_item` / `upsert_item` / `replace_item` | encode | decode | ✅ done | -| `query_items` | deferred | decode | response done; request-encode deferred | +| `query_items` | — (text spec) | decode | ✅ done (response negotiated) | | `delete_item` | — | — | n/a | | `patch_item` | deferred | deferred | deferred | | transactional batch / bulk | deferred | deferred | deferred | The response-decode boundary is shared, so `query_items` already decodes binary -response envelopes; only its *request-body* encoding + negotiation header remain. +response envelopes. A query now also **advertises** a binary response via the +`x-ms-cosmos-supported-serialization-formats` header; its request body is a +`application/query+json` query spec (not a document) and intentionally stays +text — there is no query request-body encoding to do. ## 3. Background: the .NET reference @@ -438,11 +441,16 @@ it through. It sets the option via a `with_binary_encoding` helper on **Option resolution + operation-type guard.** `execute_operation` resolves `binary_encoding` through the same runtime → account → operation layered view (`operation_options_view`) as every other option, so a default set at the -runtime/account layer is honored. Binary encoding is honored **only for point -item operations** (`OperationType::supports_binary_encoding`: create, read, -replace, upsert, delete); query, feed, batch, and stored-procedure operations -are ignored even if a caller (e.g. an FFI host) sets the flag, since those paths -remain deferred. Patch is dispatched to its own handler before this check and is +runtime/account layer is honored. Two independent gates apply. Request-body +transcoding is honored **only for point item operations** +(`OperationType::supports_binary_encoding`: create, read, replace, upsert, +delete). Response negotiation (the `x-ms-cosmos-supported-serialization-formats` +header) covers the same point item ops **plus query** +(`OperationType::supports_binary_response`) — a query advertises a binary +response while keeping its `application/query+json` request body text. Feed +(`ReadFeed` / change feed), batch, and stored-procedure operations are ignored +even if a caller (e.g. an FFI host) sets the flag, since those paths remain +deferred. Patch is dispatched to its own handler before this check and is likewise excluded. This matches the guidance that the **driver** (not the backend) performs the diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs index 6ca5de9d6c..051d862b90 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs @@ -2369,21 +2369,30 @@ impl CosmosDriver { } // Resolve binary encoding through the same layered view as every other - // option, and only honor it for point **item** operations (the resource - // must be a `Document`; query/feed/batch and every control-plane - // resource are deferred per the binary-encoding spec). - let binary = - if Self::binary_encoding_applies(operation.resource_type(), operation.operation_type()) - { - self.operation_options_view(&options) - .binary_encoding() - .cloned() - .unwrap_or_default() - } else { - crate::options::BinaryEncodingOptions::default() - }; + // option. Two independent gates apply: + // * request-body transcoding — only point **item** operations whose + // body is a document (create/read/replace/upsert on a `Document`); + // * response negotiation — the same point item ops **plus** query, + // which advertises a binary response but keeps a text request body. + // The options are resolved whenever either gate could fire. + let resource_type = operation.resource_type(); + let operation_type = operation.operation_type(); + let encodes_request_body = Self::binary_encodes_request_body(resource_type, operation_type); + let negotiates_response = Self::binary_negotiates_response(resource_type, operation_type); + let binary = if encodes_request_body || negotiates_response { + self.operation_options_view(&options) + .binary_encoding() + .cloned() + .unwrap_or_default() + } else { + crate::options::BinaryEncodingOptions::default() + }; let operation = if binary.enabled { - Self::apply_request_binary_encoding(operation)? + Self::apply_request_binary_encoding( + operation, + encodes_request_body, + negotiates_response, + )? } else { operation }; @@ -2412,13 +2421,15 @@ impl CosmosDriver { Ok(response) } - /// Whether binary encoding applies to an operation. + /// Whether an operation's **request body** should be transcoded to Cosmos + /// binary JSON. /// /// Honored only for point item operations: the resource must be a /// [`ResourceType::Document`] and the operation one of create/read/replace/ /// upsert. Control-plane resources share those operation types but must - /// never be binary encoded (some carry JSON bodies). - fn binary_encoding_applies( + /// never be binary encoded (some carry JSON bodies). Query is excluded — a + /// query body is a `application/query+json` spec, not a document. + fn binary_encodes_request_body( resource_type: crate::models::ResourceType, operation_type: crate::models::OperationType, ) -> bool { @@ -2426,39 +2437,101 @@ impl CosmosDriver { && operation_type.supports_binary_encoding() } - /// Applies request-side binary encoding to an operation: transcodes a text - /// request body to Cosmos binary JSON (an already-binary or empty body is - /// passed through) and advertises binary responses via the + /// Whether an operation may advertise a binary **response** via the /// `x-ms-cosmos-supported-serialization-formats` header. /// + /// This is a superset of [`binary_encodes_request_body`]: the point item + /// ops plus `Query` / `SqlQuery`. Change feed (`ReadFeed`) is excluded — the + /// backend does not honor the negotiation header for it. + /// + /// [`binary_encodes_request_body`]: CosmosDriver::binary_encodes_request_body + fn binary_negotiates_response( + resource_type: crate::models::ResourceType, + operation_type: crate::models::OperationType, + ) -> bool { + resource_type == crate::models::ResourceType::Document + && operation_type.supports_binary_response() + } + + /// Advertises a binary response via the + /// `x-ms-cosmos-supported-serialization-formats` header when the operation + /// negotiates one ([`binary_negotiates_response`]) and binary encoding is + /// enabled. The request body is untouched — this is response negotiation + /// only, so it is safe for query (whose text `application/query+json` body + /// must never be transcoded). + /// + /// [`binary_negotiates_response`]: CosmosDriver::binary_negotiates_response + fn apply_response_negotiation( + &self, + operation: CosmosOperation, + options: &OperationOptions, + ) -> CosmosOperation { + if !Self::binary_negotiates_response(operation.resource_type(), operation.operation_type()) + { + return operation; + } + let binary = self + .operation_options_view(options) + .binary_encoding() + .cloned() + .unwrap_or_default(); + if binary.enabled { + operation.with_supported_serialization_formats(BINARY_NEGOTIATION_FORMATS) + } else { + operation + } + } + + /// Applies request-side binary encoding to an operation as two independent + /// steps: + /// * when `encodes_request_body`, transcodes a text request body to + /// Cosmos binary JSON (an already-binary or empty body is passed + /// through); + /// * when `negotiates_response`, advertises binary responses via the + /// `x-ms-cosmos-supported-serialization-formats` header. + /// + /// A query op negotiates a binary response without transcoding its body. + /// /// This is schema-agnostic — it operates on the raw body bytes — so a /// caller that deals only in text JSON gets a binary wire without encoding /// anything itself. fn apply_request_binary_encoding( operation: CosmosOperation, + encodes_request_body: bool, + negotiates_response: bool, ) -> crate::error::Result { // Transcode a non-empty *text* body to binary. A body that is already // binary (the SDK's typed fast path) or empty is left in place — no // clone — so only genuinely text bodies pay the conversion. - let transcoded = match operation.body() { - Some(body) if !body.is_empty() && !crate::binary_json::is_binary(body) => { - Some(crate::binary_json::transcode_to_binary(body).map_err(|e| { - crate::error::CosmosError::builder() - .with_status(crate::error::CosmosStatus::SERIALIZATION_REQUEST_BODY_INVALID) - .with_message(format!( - "failed to transcode text request body to Cosmos binary JSON: {e}" - )) - .with_source(e) - .build() - })?) + let transcoded = if encodes_request_body { + match operation.body() { + Some(body) if !body.is_empty() && !crate::binary_json::is_binary(body) => { + Some(crate::binary_json::transcode_to_binary(body).map_err(|e| { + crate::error::CosmosError::builder() + .with_status( + crate::error::CosmosStatus::SERIALIZATION_REQUEST_BODY_INVALID, + ) + .with_message(format!( + "failed to transcode text request body to Cosmos binary JSON: {e}" + )) + .with_source(e) + .build() + })?) + } + _ => None, } - _ => None, + } else { + None }; let operation = match transcoded { Some(bytes) => operation.with_body(bytes), None => operation, }; - Ok(operation.with_supported_serialization_formats(BINARY_NEGOTIATION_FORMATS)) + if negotiates_response { + Ok(operation.with_supported_serialization_formats(BINARY_NEGOTIATION_FORMATS)) + } else { + Ok(operation) + } } /// Executes a singleton operation (operations which return only a single result). @@ -2963,6 +3036,13 @@ impl CosmosDriver { tracing::debug!(operation_type = ?operation.operation_type(), resource_type = ?operation.resource_type(), resource_reference = ?operation.resource_reference(), "planning operation"); + // Advertise a binary response when negotiation applies (point item ops + // and query). Point ops also set this in `execute_operation`, but query + // reaches the driver through `plan_operation` directly, so this is the + // single choke point that covers every per-page request built from the + // resulting plan. The header set is idempotent. + let operation = self.apply_response_negotiation(operation, options); + // Share the operation across every Request node in the resulting plan. // Per-Request differences are layered on at execution time via // OperationOverrides; the operation itself is never mutated. @@ -5894,7 +5974,8 @@ mod tests { fn binary_encoding_applies_only_to_document_item_ops() { use crate::models::{OperationType, ResourceType}; - // Point item ops on `Document` are the only combinations that qualify. + // Point item ops on `Document` are the only combinations whose request + // body qualifies for binary encoding. for op in [ OperationType::Create, OperationType::Read, @@ -5902,12 +5983,13 @@ mod tests { OperationType::Upsert, ] { assert!( - CosmosDriver::binary_encoding_applies(ResourceType::Document, op), + CosmosDriver::binary_encodes_request_body(ResourceType::Document, op), "Document + {op:?} should be binary-encodable", ); } - // Non-item operation types on `Document` are excluded (query/feed/delete/patch). + // Non-item operation types on `Document` are excluded from body + // encoding (query/feed/delete/patch). for op in [ OperationType::Delete, OperationType::Query, @@ -5915,8 +5997,8 @@ mod tests { OperationType::Patch, ] { assert!( - !CosmosDriver::binary_encoding_applies(ResourceType::Document, op), - "Document + {op:?} must not be binary-encoded", + !CosmosDriver::binary_encodes_request_body(ResourceType::Document, op), + "Document + {op:?} must not have its body binary-encoded", ); } @@ -5937,13 +6019,55 @@ mod tests { OperationType::Upsert, ] { assert!( - !CosmosDriver::binary_encoding_applies(rt, op), + !CosmosDriver::binary_encodes_request_body(rt, op), "{rt:?} + {op:?} must not be binary-encoded (control plane)", ); } } } + #[test] + fn binary_negotiates_response_covers_item_ops_and_query() { + use crate::models::{OperationType, ResourceType}; + + // Point item ops plus query/sql-query on `Document` advertise a binary + // response. + for op in [ + OperationType::Create, + OperationType::Read, + OperationType::Replace, + OperationType::Upsert, + OperationType::Query, + OperationType::SqlQuery, + ] { + assert!( + CosmosDriver::binary_negotiates_response(ResourceType::Document, op), + "Document + {op:?} should negotiate a binary response", + ); + } + + // Change feed (`ReadFeed`) is excluded — the backend does not honor the + // negotiation header for it. + for op in [ + OperationType::ReadFeed, + OperationType::Delete, + OperationType::Patch, + ] { + assert!( + !CosmosDriver::binary_negotiates_response(ResourceType::Document, op), + "Document + {op:?} must not negotiate a binary response", + ); + } + + // Control-plane resources never negotiate binary responses. + for op in [OperationType::Query, OperationType::Read] { + assert!( + !CosmosDriver::binary_negotiates_response(ResourceType::Database, op), + "Database + {op:?} must not negotiate a binary response (control plane)", + ); + } + } + fn binary_encoding_test_operation(body: Vec) -> CosmosOperation { let container = epk_test_container(r#"{"paths":["/pk"],"version":2}"#); let item = @@ -5960,7 +6084,7 @@ mod tests { assert!(!crate::binary_json::is_binary(&text)); let op = binary_encoding_test_operation(text); - let op = CosmosDriver::apply_request_binary_encoding(op).unwrap(); + let op = CosmosDriver::apply_request_binary_encoding(op, true, true).unwrap(); let body = op.body().expect("body present"); assert!( @@ -5988,7 +6112,7 @@ mod tests { // through unchanged. let binary = crate::binary_json::encode(&serde_json::json!({ "id": "doc1", "n": 7 })); let op = binary_encoding_test_operation(binary.clone()); - let op = CosmosDriver::apply_request_binary_encoding(op).unwrap(); + let op = CosmosDriver::apply_request_binary_encoding(op, true, true).unwrap(); assert_eq!(op.body().unwrap(), binary.as_slice()); assert_eq!( @@ -6004,10 +6128,39 @@ mod tests { // A body that is neither binary nor valid JSON surfaces as a // request-body serialization error. let op = binary_encoding_test_operation(b"{not json".to_vec()); - let err = CosmosDriver::apply_request_binary_encoding(op).unwrap_err(); + let err = CosmosDriver::apply_request_binary_encoding(op, true, true).unwrap_err(); assert_eq!( err.status().sub_status(), Some(crate::error::SubStatusCode::SERIALIZATION_REQUEST_BODY_INVALID), ); } + + #[test] + fn apply_request_binary_encoding_query_negotiates_response_without_transcoding_body() { + // A query op negotiates a binary *response* but must leave its + // `application/query+json` body untouched (text), because the body is a + // query spec, not a document. + let container = epk_test_container(r#"{"paths":["/pk"],"version":2}"#); + let query_body = + serde_json::to_vec(&serde_json::json!({ "query": "SELECT * FROM c" })).unwrap(); + let op = CosmosOperation::query_items(container, Some(FeedRange::full())) + .with_body(query_body.clone()); + + // encodes_request_body = false (query), negotiates_response = true. + let op = CosmosDriver::apply_request_binary_encoding(op, false, true).unwrap(); + + // Body is unchanged text — never transcoded to binary. + assert_eq!(op.body().unwrap(), query_body.as_slice()); + assert!( + !crate::binary_json::is_binary(op.body().unwrap()), + "query body must remain text on the wire", + ); + // Still advertises a binary response. + assert_eq!( + op.request_headers() + .supported_serialization_formats + .as_deref(), + Some("CosmosBinary"), + ); + } } diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/operations.rs b/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/operations.rs index eb23ae9582..c81d49393e 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/operations.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/operations.rs @@ -2323,6 +2323,7 @@ fn success_feed_response( items: Vec, page_options: FeedPageOptions<'_>, feed_headers: FeedResponseHeaders, + binary: bool, start: Instant, ) -> AsyncRawResponse { let (page, next) = match paginate_values( @@ -2336,9 +2337,10 @@ fn success_feed_response( }; let item_count = page.len() as u32; let body = feed_to_json(envelope_name, page, rid); - let mut builder = success_response( + let mut builder = success_response_with_format( StatusCode::Ok, &body, + binary, 1.0, &feed_headers.session_token, start, @@ -2365,6 +2367,7 @@ fn success_document_feed_response( items: Vec, page_options: FeedPageOptions<'_>, feed_headers: FeedResponseHeaders, + binary: bool, start: Instant, ) -> AsyncRawResponse { let (page, next) = match paginate_document_feed_items( @@ -2378,9 +2381,10 @@ fn success_document_feed_response( }; let item_count = page.len() as u32; let body = feed_to_json(envelope_name, page, rid); - let mut builder = success_response( + let mut builder = success_response_with_format( StatusCode::Ok, &body, + binary, 1.0, &feed_headers.session_token, start, @@ -2484,6 +2488,7 @@ fn execute_query_feed( results, FeedPageOptions::from_request(parsed), feed_headers, + parsed.binary_response, start, ) } @@ -2508,6 +2513,7 @@ fn execute_document_query_feed( results, FeedPageOptions::from_request(parsed), feed_headers, + parsed.binary_response, start, ), Ok(None) => { @@ -2533,6 +2539,7 @@ fn execute_document_query_feed( results, FeedPageOptions::from_request(parsed), feed_headers, + parsed.binary_response, start, ) } @@ -2711,6 +2718,7 @@ fn handle_read_feed_databases( databases, FeedPageOptions::from_request(parsed), FeedResponseHeaders::none(), + false, start, ) } @@ -2776,6 +2784,7 @@ fn handle_read_feed_containers( containers, FeedPageOptions::from_request(parsed), FeedResponseHeaders::none(), + false, start, ) } @@ -2837,6 +2846,7 @@ fn handle_read_feed_offers( offers, FeedPageOptions::from_request(parsed), FeedResponseHeaders::none(), + false, start, ) } @@ -3178,6 +3188,7 @@ fn handle_read_feed_items( docs, FeedPageOptions::from_request(parsed), headers, + false, start, ) } diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/models/mod.rs b/sdk/cosmos/azure_data_cosmos_driver/src/models/mod.rs index 260f920811..a06b3cf598 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/models/mod.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/models/mod.rs @@ -636,10 +636,17 @@ impl OperationType { ) } - /// True for the point item ops (create/read/replace/upsert) eligible for - /// binary encoding. Necessary but not sufficient: the full gate also - /// requires [`ResourceType::Document`] (see - /// `CosmosDriver::binary_encoding_applies`). + /// True for the point item ops (create/read/replace/upsert) whose **request + /// body** is eligible for Cosmos binary encoding. Necessary but not + /// sufficient: the full gate also requires [`ResourceType::Document`] (see + /// `CosmosDriver::binary_encodes_request_body`). + /// + /// Query is intentionally excluded: a query request body is a + /// `{"query":..., "parameters":[...]}` spec sent as `application/query+json`, + /// not a document, so it must not be transcoded to binary. Query still + /// negotiates a binary *response* — see [`supports_binary_response`]. + /// + /// [`supports_binary_response`]: OperationType::supports_binary_response pub(crate) fn supports_binary_encoding(self) -> bool { matches!( self, @@ -650,6 +657,20 @@ impl OperationType { ) } + /// True for the ops that may negotiate a binary **response** via the + /// `x-ms-cosmos-supported-serialization-formats` header. This is a superset + /// of [`supports_binary_encoding`](OperationType::supports_binary_encoding): + /// the point item ops plus `Query` / `SqlQuery`. + /// + /// `ReadFeed` / change feed is intentionally excluded: the backend only + /// honors the negotiation header for `Query`, and returns a binary response + /// for a `ReadFeed`-with-partition-key request as a known bug — so change + /// feed must never advertise binary. + pub(crate) fn supports_binary_response(self) -> bool { + self.supports_binary_encoding() + || matches!(self, OperationType::Query | OperationType::SqlQuery) + } + /// Returns the HTTP method for this operation type. pub fn http_method(self) -> azure_core::http::Method { use azure_core::http::Method; From 63b6951d03a2abd88d6aa3f1ce4406f9cad00b9e Mon Sep 17 00:00:00 2001 From: kundadebdatta Date: Mon, 10 Aug 2026 17:35:06 -0700 Subject: [PATCH 02/13] test(cosmos): cover passthrough cross-partition binary query 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. --- .../binary_round_trip.rs | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs b/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs index 53f9c0d2ac..508e72d8d5 100644 --- a/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs +++ b/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs @@ -101,6 +101,59 @@ async fn build_container(db_name: &str, binary: bool) -> ContainerClient { .unwrap() } +/// Like [`build_container`], but provisions `partition_count` physical +/// partitions so a full-container query fans out across ranges (the passthrough +/// cross-partition path). Binary encoding is enabled. +async fn build_multi_partition_container(db_name: &str, partition_count: u32) -> ContainerClient { + let config = VirtualAccountConfig::new(vec![VirtualRegion::new( + "East US", + azure_core::http::Url::parse(EMULATOR_GATEWAY_URL).unwrap(), + )]) + .unwrap() + .with_consistency(ConsistencyLevel::Session); + + let emulator = std::sync::Arc::new(InMemoryEmulatorHttpClient::new(config)); + let store = emulator.store(); + store.create_database(db_name); + store.create_container_with_config( + db_name, + "items", + serde_json::from_value(serde_json::json!({ + "paths": ["/pk"], + "kind": "Hash", + "version": 2 + })) + .unwrap(), + ContainerConfig::new() + .with_partition_count(partition_count) + .with_throughput(400) + .build() + .unwrap(), + ); + + let account = AccountReference::with_authentication_key( + EMULATOR_GATEWAY_URL.parse::().unwrap(), + azure_core::credentials::Secret::new("dGVzdGtleQ=="), + ); + let client = CosmosClientBuilder::new() + .with_binary_encoding_options(BinaryEncodingOptions::new().with_enabled(true)) + .with_runtime( + CosmosRuntimeBuilder::from(emulator.runtime_builder()) + .build() + .await + .unwrap(), + ) + .build(account, RoutingStrategy::ProximityTo(Region::EAST_US)) + .await + .unwrap(); + + client + .database_client(db_name) + .container_client("items") + .await + .unwrap() +} + /// With binary enabled, an item written through the SDK is binary-encoded on the /// wire, decoded + stored by the emulator, returned as binary, and decoded back /// — and the value survives every hop unchanged. @@ -510,3 +563,46 @@ async fn binary_query_negotiates_response_and_round_trips() { ); } } + +/// Passthrough **cross-partition** binary query: a full-container `SELECT *` +/// fans out across multiple physical partitions. Each partition's page is +/// returned as an independent binary `Documents` envelope, decoded per page +/// through the shared choke point — so every item round-trips regardless of +/// which partition served it. +#[tokio::test] +async fn binary_cross_partition_query_round_trips() { + let container = build_multi_partition_container("bin-xpart-query", 3).await; + + // Spread items across several partition keys so the fan-out spans ranges. + let items: Vec = (0..12) + .map(|i| TestItem { + id: format!("x-{i}"), + pk: format!("pk{}", i % 4), + value: i, + note: format!("café ☃ {i}"), + }) + .collect(); + for item in &items { + container + .create_item(&item.pk, &item.id, item, Some(write_options_with_content())) + .await + .unwrap(); + } + + let iter = Box::pin(container.query_items( + Query::from("SELECT * FROM c"), + FeedScope::full_container(), + None, + )) + .await + .unwrap(); + let mut results: Vec = Box::pin(iter.try_collect()).await.unwrap(); + results.sort_by_key(|d| d.value); + + let mut expected = items; + expected.sort_by_key(|d| d.value); + assert_eq!( + results, expected, + "cross-partition query results must round-trip through binary", + ); +} From b815ba4bb147bc25372233efdca2b7f85659b839 Mon Sep 17 00:00:00 2001 From: kundadebdatta Date: Mon, 10 Aug 2026 18:59:58 -0700 Subject: [PATCH 03/13] docs(cosmos): add Rust vs .NET binary-encoding parity matrix 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. --- .../docs/BINARY_ENCODING_DOTNET_PARITY.md | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_DOTNET_PARITY.md diff --git a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_DOTNET_PARITY.md b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_DOTNET_PARITY.md new file mode 100644 index 0000000000..6c3c36e14b --- /dev/null +++ b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_DOTNET_PARITY.md @@ -0,0 +1,95 @@ +# Binary Encoding — Rust vs .NET Parity (per operation) + +Tracks how Cosmos **binary JSON** encoding behaves across every operation type in +the Rust SDK/driver (`azure_data_cosmos` + `azure_data_cosmos_driver`) compared to +the .NET SDK (`Azure/azure-cosmos-dotnet-v3`). + +Cosmos binary JSON is the `0x80`-preamble wire format. "Binary encoding" spans +three independent concerns: + +- **Req-encode** — the request body is serialized as Cosmos binary JSON. +- **Resp-negotiate** — the request advertises + `x-ms-cosmos-supported-serialization-formats: CosmosBinary`, asking the service + to return a binary body. +- **Resp-decode** — the client decodes a binary response body (auto-detected by + the `0x80` first byte). + +> Last verified: 2026-08-10, against `azure-cosmos-dotnet-v3` `main`. + +## Enablement model + +| | .NET | Rust | +|---|---|---| +| Opt-in gate | `ConfigurationManager.IsBinaryEncodingEnabled()` (env var) + `ItemRequestOptions.EnableBinaryResponseOnPointOperations` | `BinaryEncodingOptions` (client default + per-op override) | +| Suppressed with custom serializer | Yes — `GetTargetResponseSerializationFormat` returns `Text` | N/A (SDK owns serde) | +| Response decode | Format-agnostic `JsonNavigator` (first-byte detect) | Shared `deserialize_response` / `is_binary` choke point | +| Status | Preview / opt-in | Preview / opt-in | + +## Per-operation matrix + +Legend: ✅ supported · ❌ not · — not applicable. + +| Operation | .NET Req-encode | .NET Resp-negotiate | Rust Req-encode | Rust Resp-negotiate | Rust Resp-decode | Parity | +|---|:--:|:--:|:--:|:--:|:--:|---| +| Create item | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ match | +| Read item | — (no body) | ✅ | ✅ (no-op) | ✅ | ✅ | ✅ match | +| Replace item | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ match | +| Upsert item | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ match | +| Delete item | — | ✅ (in point-binary set) | ❌ | ❌ | — | ⚠️ diff | +| Query — single-partition | text body | ✅ (Query only) | text body | ✅ | ✅ | ✅ match | +| Query — passthrough cross-partition | text body | ✅ | text body | ✅ | ✅ | ✅ match | +| Query — ORDER BY / aggregate / GROUP BY / DISTINCT / TOP / LIMIT | ✅ | ✅ | ❌ (engine absent) | ❌ | ❌ | ⚠️ diff | +| Change feed / ReadFeed | ❌ (backend bug) | ❌ | ❌ | ❌ | (capable, unused) | ✅ match | +| Patch | ❌ (real server op) | ❌ | ✅ via internal Read+Replace | ✅ (sub-ops) | ✅ | ⚠️ diff (mechanism) | +| Transactional batch | ❌ (HybridRow, not binary JSON) | ❌ | ❌ | ❌ | — | ✅ match | +| Bulk | ❌ | ❌ | ❌ | ❌ | — | ✅ match | +| Stored procedure (Execute) | ❌ | ❌ | ❌ | ❌ | — | ✅ match | +| Control-plane (db / container / offer) | ❌ | ❌ | ❌ | ❌ | — | ✅ match | + +## Differences that matter + +| # | Difference | Detail | Severity | +|---|---|---|---| +| 1 | ORDER BY / aggregate / GROUP BY / DISTINCT / TOP / LIMIT cross-partition | .NET runs them; its cross-partition merge is on the format-agnostic `CosmosElement` model, so binary works for free. Rust's `validate_query_info` (`dataflow/planner.rs`) **rejects them in any encoding** — the ordered/aggregate merge engine does not exist yet, and `SUPPORTED_QUERY_FEATURES = "None"`. | Real capability gap (not binary-specific) | +| 2 | Delete negotiation | .NET's `IsPointOperationSupportedForBinaryEncoding` includes `Delete`; Rust's `supports_binary_encoding` / `supports_binary_response` exclude it. Low impact (delete carries no request body and typically no response body), but Rust does not advertise binary for delete. | Minor | +| 3 | Patch mechanism | .NET Patch is a real server op and is **not** binary-negotiated. Rust Patch is a client-side Read-Modify-Write, so its internal Read/Replace **are** binary-encoded when enabled. Different architecture; both functionally correct. | Cosmetic / architectural | +| 4 | Negotiation header value | .NET query default = `"JsonText,CosmosBinary"`; .NET point ops = `"CosmosBinary"`. Rust = `"CosmosBinary"` for all paths (`BINARY_NEGOTIATION_FORMATS`). Rust always forces binary rather than advertising "either". | Minor wire diff | + +## Similarities (matched by design) + +- Point item ops (create/read/replace/upsert) encode requests and decode responses identically. +- Single-partition and passthrough cross-partition queries: text request body, negotiated binary response, per-page binary decode. +- Query request body always stays text (`application/query+json` is a query spec, not a document). +- Change feed / ReadFeed excluded from binary negotiation (the backend returns binary for ReadFeed-with-partition-key as a known bug). +- Batch / bulk / stored procedures / control-plane resources never use binary JSON. +- Response decode is a single format-agnostic choke point on both sides (first-byte `0x80` detection). + +## Bottom line + +For point operations, single-partition queries, and passthrough cross-partition +queries, Rust and .NET are **functionally equivalent** on binary encoding. + +Actionable divergences: + +- **#1** is the only user-facing gap, and it is a missing query engine, not a + binary issue — deferred. When that engine is built, use a format-agnostic value + model (like .NET's `CosmosElement`) so binary support is inherent, not retrofitted. +- **#2 (Delete)** is a genuine small binary-scope divergence: add `Delete` to the + Rust predicates to match .NET's point-operation binary scope exactly. +- **#3, #4** are mechanism / wire nuances with no functional impact. + +## Key source references + +Rust: + +- `src/models/mod.rs` — `OperationType::supports_binary_encoding` / `supports_binary_response`. +- `src/driver/cosmos_driver.rs` — `binary_encodes_request_body`, `binary_negotiates_response`, `apply_request_binary_encoding`, `apply_response_negotiation`, `BINARY_NEGOTIATION_FORMATS`. +- `src/models/response_body.rs` — `deserialize_response`, `into_single`, `into_items` (text-only splitter note). +- `src/driver/dataflow/planner.rs` — `validate_query_info` (rejects ORDER BY / aggregate / etc.). + +.NET (`Azure/azure-cosmos-dotnet-v3`): + +- `src/Handler/RequestInvokerHandler.cs` — `IsPointOperationSupportedForBinaryEncoding` (create/replace/delete/read/upsert), sets `SupportedSerializationFormats = CosmosBinary` for point ops. +- `src/RequestOptions/QueryRequestOptions.cs` — `PopulateRequestOptions` sets the header for `OperationType.Query` only (with the ReadFeed backend-bug comment). +- `src/Query/v2Query/DocumentQueryExecutionContextBase.cs` — `DefaultSupportedSerializationFormats = "JsonText,CosmosBinary"`. +- `src/Resource/Container/ContainerCore.Items.cs` — `GetTargetRequestSerializationFormat` / `GetTargetResponseSerializationFormat`. From cacfd7feee8b746d0b90f51a7f006e2fbc88779a Mon Sep 17 00:00:00 2001 From: kundadebdatta Date: Tue, 11 Aug 2026 17:22:03 -0700 Subject: [PATCH 04/13] Cosmos: support binary encoding for streaming ORDER BY queries 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. --- .../tests/binary_roundtrip_fuzzer.rs | 99 ++++++++++++++++++- .../docs/BINARY_ENCODING_DOTNET_PARITY.md | 15 ++- .../integration_tests/order_by_resume.rs | 74 ++++++++++++++ .../src/driver/dataflow/query_response.rs | 24 +++++ 4 files changed, 205 insertions(+), 7 deletions(-) diff --git a/sdk/cosmos/azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs b/sdk/cosmos/azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs index c270314603..dd29880fad 100644 --- a/sdk/cosmos/azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs +++ b/sdk/cosmos/azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs @@ -45,9 +45,11 @@ use azure_data_cosmos::options::{ OperationOptions, Region, ServerCertificateValidation, }; use azure_data_cosmos::{ - AccountEndpoint, AccountReference, CosmosClient, CosmosRuntime, RoutingStrategy, SubStatusCode, + AccountEndpoint, AccountReference, CosmosClient, CosmosRuntime, FeedScope, Query, + RoutingStrategy, SubStatusCode, }; use azure_data_cosmos_driver::models::ConnectionString; +use futures::TryStreamExt; use serde::{Deserialize, Serialize}; use serde_json::{Map, Number, Value}; use sha2::{Digest, Sha256}; @@ -1969,6 +1971,83 @@ where } } +/// Queries the just-written item back and asserts it round-trips, covering the +/// query binary-response decode path (which point ops do not exercise). Runs +/// two shapes so both query pipelines are hit under binary encoding: +/// +/// * a single-partition query (`WHERE c.id = @id` scoped to the item's +/// partition) — the passthrough decode path, and +/// * a cross-partition streaming `ORDER BY` query (`... ORDER BY c.id` over the +/// full container) — the k-way merge, whose per-page envelope decode is the +/// binary path added for query support. +/// +/// Both filter on the unique `id`, so each returns exactly this item. +async fn assert_query_roundtrip( + container: &ContainerClient, + pk: &str, + id: &str, + sent_canon: &str, + sent_hash: &[u8; 32], + doc: &Map, + context: &str, +) -> Result<(), Box> { + let single_partition = with_transient_retry("query", context, || async { + let query = Query::from("SELECT * FROM c WHERE c.id = @id").with_parameter("@id", id)?; + let iter = container + .query_items::(query, FeedScope::partition(pk.to_string()), None) + .await?; + iter.try_collect::>().await + }) + .await?; + assert_query_hit( + &single_partition, + doc, + sent_canon, + sent_hash, + context, + "query", + ); + + let order_by = with_transient_retry("query-order-by", context, || async { + let query = Query::from("SELECT * FROM c WHERE c.id = @id ORDER BY c.id") + .with_parameter("@id", id)?; + let iter = container + .query_items::(query, FeedScope::full_container(), None) + .await?; + iter.try_collect::>().await + }) + .await?; + assert_query_hit( + &order_by, + doc, + sent_canon, + sent_hash, + context, + "query-order-by", + ); + + Ok(()) +} + +/// Asserts a query returned exactly the one expected item and that it +/// round-trips against the sent canonical form. +fn assert_query_hit( + results: &[Value], + doc: &Map, + sent_canon: &str, + sent_hash: &[u8; 32], + context: &str, + phase: &str, +) { + assert_eq!( + results.len(), + 1, + "{context}: {phase} expected exactly one item, got {}", + results.len() + ); + assert_roundtrip(doc, &results[0], sent_canon, sent_hash, context, phase); +} + // ───────────────────────────────────────────────────────────────────────────── // The fuzzer test // ───────────────────────────────────────────────────────────────────────────── @@ -2176,6 +2255,22 @@ async fn binary_encoding_roundtrip_fuzz() -> Result<(), Box> { // Four point-op round-trips this config: create, read, replace, upsert. checked += 4; + // QUERY the item back (single-partition + cross-partition ORDER BY). + // Covers the query binary-response decode path — including the + // streaming ORDER BY per-page envelope decode — which the point ops + // above do not exercise. + assert_query_roundtrip( + &container, + &pk, + &id, + &sent_canon, + &sent_hash, + &doc, + &context, + ) + .await?; + checked += 2; + // The four ops above decode into `serde_json::Value` (→ // `deserialize_any`), so they do NOT cover the native typed-integer // path (`deserialize_integer`) this PR ships. A typed probe covers it @@ -2194,7 +2289,7 @@ async fn binary_encoding_roundtrip_fuzz() -> Result<(), Box> { } println!( - "binary_roundtrip_fuzzer: DONE — {} documents × {} configs × 4 point ops = {checked} round-trips, all canonical-equal (seed={})", + "binary_roundtrip_fuzzer: DONE — {} documents × {} configs × (4 point ops + 2 queries) = {checked} round-trips, all canonical-equal (seed={})", cfg.iterations, configs.len(), cfg.seed diff --git a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_DOTNET_PARITY.md b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_DOTNET_PARITY.md index 6c3c36e14b..31ec4c047c 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_DOTNET_PARITY.md +++ b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_DOTNET_PARITY.md @@ -38,7 +38,8 @@ Legend: ✅ supported · ❌ not · — not applicable. | Delete item | — | ✅ (in point-binary set) | ❌ | ❌ | — | ⚠️ diff | | Query — single-partition | text body | ✅ (Query only) | text body | ✅ | ✅ | ✅ match | | Query — passthrough cross-partition | text body | ✅ | text body | ✅ | ✅ | ✅ match | -| Query — ORDER BY / aggregate / GROUP BY / DISTINCT / TOP / LIMIT | ✅ | ✅ | ❌ (engine absent) | ❌ | ❌ | ⚠️ diff | +| Query — streaming ORDER BY (scalar sort keys) | text body | ✅ | text body | ✅ | ✅ | ✅ match (PR #4800) | +| Query — aggregate / GROUP BY / DISTINCT / TOP / LIMIT | text body | ✅ | ❌ (engine absent) | ❌ | ❌ | ⚠️ diff | | Change feed / ReadFeed | ❌ (backend bug) | ❌ | ❌ | ❌ | (capable, unused) | ✅ match | | Patch | ❌ (real server op) | ❌ | ✅ via internal Read+Replace | ✅ (sub-ops) | ✅ | ⚠️ diff (mechanism) | | Transactional batch | ❌ (HybridRow, not binary JSON) | ❌ | ❌ | ❌ | — | ✅ match | @@ -50,7 +51,7 @@ Legend: ✅ supported · ❌ not · — not applicable. | # | Difference | Detail | Severity | |---|---|---|---| -| 1 | ORDER BY / aggregate / GROUP BY / DISTINCT / TOP / LIMIT cross-partition | .NET runs them; its cross-partition merge is on the format-agnostic `CosmosElement` model, so binary works for free. Rust's `validate_query_info` (`dataflow/planner.rs`) **rejects them in any encoding** — the ordered/aggregate merge engine does not exist yet, and `SUPPORTED_QUERY_FEATURES = "None"`. | Real capability gap (not binary-specific) | +| 1 | aggregate / GROUP BY / DISTINCT / TOP / LIMIT cross-partition | .NET runs them; its cross-partition merge is on the format-agnostic `CosmosElement` model, so binary works for free. Rust's `validate_query_info` (`dataflow/planner.rs`) still **rejects these in any encoding** — the aggregate/GROUP BY/DISTINCT merge engine does not exist yet. (Streaming ORDER BY with scalar sort keys is now supported — see PR #4800 — and binary works for it because response decode is format-agnostic.) | Real capability gap (not binary-specific) | | 2 | Delete negotiation | .NET's `IsPointOperationSupportedForBinaryEncoding` includes `Delete`; Rust's `supports_binary_encoding` / `supports_binary_response` exclude it. Low impact (delete carries no request body and typically no response body), but Rust does not advertise binary for delete. | Minor | | 3 | Patch mechanism | .NET Patch is a real server op and is **not** binary-negotiated. Rust Patch is a client-side Read-Modify-Write, so its internal Read/Replace **are** binary-encoded when enabled. Different architecture; both functionally correct. | Cosmetic / architectural | | 4 | Negotiation header value | .NET query default = `"JsonText,CosmosBinary"`; .NET point ops = `"CosmosBinary"`. Rust = `"CosmosBinary"` for all paths (`BINARY_NEGOTIATION_FORMATS`). Rust always forces binary rather than advertising "either". | Minor wire diff | @@ -72,8 +73,10 @@ queries, Rust and .NET are **functionally equivalent** on binary encoding. Actionable divergences: - **#1** is the only user-facing gap, and it is a missing query engine, not a - binary issue — deferred. When that engine is built, use a format-agnostic value - model (like .NET's `CosmosElement`) so binary support is inherent, not retrofitted. + binary issue — deferred. Streaming ORDER BY (scalar sort keys) already landed via + PR #4800 with binary working for free; the remaining aggregate/GROUP BY/DISTINCT + engine should likewise use a format-agnostic value model (like .NET's + `CosmosElement`) so binary support is inherent, not retrofitted. - **#2 (Delete)** is a genuine small binary-scope divergence: add `Delete` to the Rust predicates to match .NET's point-operation binary scope exactly. - **#3, #4** are mechanism / wire nuances with no functional impact. @@ -85,7 +88,9 @@ Rust: - `src/models/mod.rs` — `OperationType::supports_binary_encoding` / `supports_binary_response`. - `src/driver/cosmos_driver.rs` — `binary_encodes_request_body`, `binary_negotiates_response`, `apply_request_binary_encoding`, `apply_response_negotiation`, `BINARY_NEGOTIATION_FORMATS`. - `src/models/response_body.rs` — `deserialize_response`, `into_single`, `into_items` (text-only splitter note). -- `src/driver/dataflow/planner.rs` — `validate_query_info` (rejects ORDER BY / aggregate / etc.). +- `src/driver/dataflow/planner.rs` — `validate_query_info` (rejects aggregate / GROUP BY / DISTINCT / TOP / LIMIT). +- `src/driver/dataflow/order_by.rs`, `src/driver/dataflow/streaming_ordered_merge.rs` — streaming ORDER BY merge (PR #4800). +- `src/driver/dataflow/query_response.rs` — `parse_envelope_page` transcodes a binary ORDER BY page to text (`binary_json::transcode_to_text`) before the envelope parse, so the merge is format-agnostic like the point/passthrough decode path. .NET (`Azure/azure-cosmos-dotnet-v3`): diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/integration_tests/order_by_resume.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/integration_tests/order_by_resume.rs index 95ab97eec2..1a6553ebfb 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/integration_tests/order_by_resume.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/integration_tests/order_by_resume.rs @@ -214,6 +214,43 @@ fn envelope_page(rows: &[(&str, i64)], continuation: Option<&str>) -> CosmosResp ) } +/// Like [`envelope_page`] but encodes the feed body as Cosmos **binary** JSON, +/// exercising `parse_envelope_page`'s transcode path through the merge. +fn binary_envelope_page(rows: &[(&str, i64)], continuation: Option<&str>) -> CosmosResponse { + let documents: Vec = rows + .iter() + .map(|(rid, rank)| { + serde_json::json!({ + "_rid": label_rid(rid), + "orderByItems": [{"item": rank}], + "payload": {"id": rid, "rank": rank}, + }) + }) + .collect(); + let body = serde_json::json!({ + "_rid": "", + "Documents": documents, + "_count": documents.len(), + }); + let text = serde_json::to_vec(&body).unwrap(); + let binary = crate::binary_json::transcode_to_binary(&text).unwrap(); + assert!(crate::binary_json::is_binary(&binary)); + let mut diagnostics = DiagnosticsContextBuilder::new( + ActivityId::new_uuid(), + Arc::new(DiagnosticsOptions::default()), + ); + diagnostics.set_operation_status(azure_core::http::StatusCode::Ok, None); + let mut headers = CosmosResponseHeaders::new(); + headers.continuation = continuation.map(str::to_owned); + headers.request_charge = Some(crate::models::RequestCharge::new(1.5)); + CosmosResponse::new( + binary, + headers, + CosmosStatus::new(azure_core::http::StatusCode::Ok), + Arc::new(diagnostics.complete()), + ) +} + /// Like [`envelope_page`] but JOIN-shaped: `(rid, rank, id)` rows where /// several rows may share one `_rid` (a single document expanded by a JOIN) /// while carrying distinct payload `id`s. Drives the `skip_count` resume path. @@ -479,6 +516,43 @@ async fn merges_two_partitions_into_global_order() { ); } +/// Binary-negotiated ORDER BY pages must merge into the same global order as +/// the text path — proof the merge is format-agnostic. +#[tokio::test] +async fn merges_two_binary_partitions_into_global_order() { + let op = order_by_operation(); + let plan = order_by_plan(); + + let mut topology = MockTopologyProvider::new(vec![Ok(vec![ + resolved("", "80", "pk-left"), + resolved("80", "FF", "pk-right"), + ])]); + let mut executor = MockRequestExecutor::new(vec![ + Ok(binary_envelope_page( + &[("l1", 1), ("l2", 3), ("l3", 5)], + None, + )), + Ok(binary_envelope_page( + &[("r1", 2), ("r2", 4), ("r3", 6)], + None, + )), + ]); + + let mut pipeline = build_streaming_ordered_merge(&plan, &mut topology, &op, None) + .await + .unwrap(); + let ids = drain_all(&mut pipeline, &mut executor).await; + + assert_eq!( + ids, + vec!["l1", "r1", "l2", "r2", "l3", "r3"] + .into_iter() + .map(str::to_owned) + .collect::>(), + "binary ORDER BY pages must interleave in the same global order as text" + ); +} + /// A single partition, single page: the trivial case must still flow /// through the merge machinery correctly (no fan-out needed). #[tokio::test] diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs index 954663aad5..8e5bf867c0 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs @@ -306,6 +306,13 @@ pub(crate) fn parse_envelope_page( )); } }; + // Binary-negotiated ORDER BY pages arrive as Cosmos binary JSON; the + // envelope parse below is text-only, so transcode first (no-op for text). + let bytes = crate::binary_json::transcode_to_text(&bytes).map_err(|e| { + envelope_error(format!( + "failed to transcode binary ORDER BY envelope page to text: {e}" + )) + })?; let feed: RawFeedBody = serde_json::from_slice(&bytes).map_err(|e| { body_error( "failed to parse rewritten-query backend page as a feed body", @@ -753,6 +760,23 @@ mod tests { assert_eq!(rows[1].keys, vec![OrderByItem::Undefined]); } + #[test] + fn parse_envelope_page_decodes_binary_envelope() { + // A binary-encoded page must parse into the same rows as its text form. + let text = br#"{"_rid":"abc","Documents":[{"_rid":"r1","orderByItems":[{"item":1}],"payload":{"id":"d1"}},{"_rid":"r2","orderByItems":[{}],"payload":{"id":"d2"}}],"_count":2}"#; + let binary = crate::binary_json::transcode_to_binary(text).unwrap(); + assert!(crate::binary_json::is_binary(&binary)); + + let rows = parse_envelope_page(&ResponseBody::from_bytes(binary), 1).unwrap(); + assert_eq!(rows.len(), 2); + assert_eq!(rows[0].rid, "r1"); + assert_eq!(rows[0].keys, vec![OrderByItem::Number(1.0.into())]); + assert_eq!(rows[0].payload.get(), r#"{"id":"d1"}"#); + assert_eq!(rows[1].rid, "r2"); + assert_eq!(rows[1].keys, vec![OrderByItem::Undefined]); + assert_eq!(rows[1].payload.get(), r#"{"id":"d2"}"#); + } + #[test] fn parse_envelope_page_empty_body_yields_no_rows() { let rows = parse_envelope_page(&ResponseBody::NoPayload, 1).unwrap(); From b38af06767dd10a4ec285c1c354ba2a9de694236 Mon Sep 17 00:00:00 2001 From: kundadebdatta Date: Wed, 12 Aug 2026 15:48:52 -0700 Subject: [PATCH 05/13] Address local review findings --- .../binary_round_trip.rs | 4 +- .../docs/BINARY_ENCODING_DOTNET_PARITY.md | 100 ------------- .../docs/BINARY_ENCODING_HLD.md | 19 +-- .../docs/BINARY_ENCODING_SPEC.md | 14 +- .../src/driver/cosmos_driver.rs | 133 +++++++----------- .../src/driver/dataflow/query_response.rs | 33 +++-- .../src/models/mod.rs | 15 +- 7 files changed, 106 insertions(+), 212 deletions(-) delete mode 100644 sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_DOTNET_PARITY.md diff --git a/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs b/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs index 508e72d8d5..d62a7a9377 100644 --- a/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs +++ b/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs @@ -444,7 +444,9 @@ impl RequestObserver for QueryRequestRecorder { self.negotiation_formats.lock().unwrap().push(formats); let is_binary = match request.body() { - azure_core::http::request::Body::Bytes(bytes) => bytes.first() == Some(&0x80), + azure_core::http::request::Body::Bytes(bytes) => { + azure_data_cosmos_driver::binary_json::is_binary(bytes) + } _ => false, }; self.body_is_binary.lock().unwrap().push(is_binary); diff --git a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_DOTNET_PARITY.md b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_DOTNET_PARITY.md deleted file mode 100644 index 31ec4c047c..0000000000 --- a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_DOTNET_PARITY.md +++ /dev/null @@ -1,100 +0,0 @@ -# Binary Encoding — Rust vs .NET Parity (per operation) - -Tracks how Cosmos **binary JSON** encoding behaves across every operation type in -the Rust SDK/driver (`azure_data_cosmos` + `azure_data_cosmos_driver`) compared to -the .NET SDK (`Azure/azure-cosmos-dotnet-v3`). - -Cosmos binary JSON is the `0x80`-preamble wire format. "Binary encoding" spans -three independent concerns: - -- **Req-encode** — the request body is serialized as Cosmos binary JSON. -- **Resp-negotiate** — the request advertises - `x-ms-cosmos-supported-serialization-formats: CosmosBinary`, asking the service - to return a binary body. -- **Resp-decode** — the client decodes a binary response body (auto-detected by - the `0x80` first byte). - -> Last verified: 2026-08-10, against `azure-cosmos-dotnet-v3` `main`. - -## Enablement model - -| | .NET | Rust | -|---|---|---| -| Opt-in gate | `ConfigurationManager.IsBinaryEncodingEnabled()` (env var) + `ItemRequestOptions.EnableBinaryResponseOnPointOperations` | `BinaryEncodingOptions` (client default + per-op override) | -| Suppressed with custom serializer | Yes — `GetTargetResponseSerializationFormat` returns `Text` | N/A (SDK owns serde) | -| Response decode | Format-agnostic `JsonNavigator` (first-byte detect) | Shared `deserialize_response` / `is_binary` choke point | -| Status | Preview / opt-in | Preview / opt-in | - -## Per-operation matrix - -Legend: ✅ supported · ❌ not · — not applicable. - -| Operation | .NET Req-encode | .NET Resp-negotiate | Rust Req-encode | Rust Resp-negotiate | Rust Resp-decode | Parity | -|---|:--:|:--:|:--:|:--:|:--:|---| -| Create item | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ match | -| Read item | — (no body) | ✅ | ✅ (no-op) | ✅ | ✅ | ✅ match | -| Replace item | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ match | -| Upsert item | ✅ | ✅ | ✅ | ✅ | ✅ | ✅ match | -| Delete item | — | ✅ (in point-binary set) | ❌ | ❌ | — | ⚠️ diff | -| Query — single-partition | text body | ✅ (Query only) | text body | ✅ | ✅ | ✅ match | -| Query — passthrough cross-partition | text body | ✅ | text body | ✅ | ✅ | ✅ match | -| Query — streaming ORDER BY (scalar sort keys) | text body | ✅ | text body | ✅ | ✅ | ✅ match (PR #4800) | -| Query — aggregate / GROUP BY / DISTINCT / TOP / LIMIT | text body | ✅ | ❌ (engine absent) | ❌ | ❌ | ⚠️ diff | -| Change feed / ReadFeed | ❌ (backend bug) | ❌ | ❌ | ❌ | (capable, unused) | ✅ match | -| Patch | ❌ (real server op) | ❌ | ✅ via internal Read+Replace | ✅ (sub-ops) | ✅ | ⚠️ diff (mechanism) | -| Transactional batch | ❌ (HybridRow, not binary JSON) | ❌ | ❌ | ❌ | — | ✅ match | -| Bulk | ❌ | ❌ | ❌ | ❌ | — | ✅ match | -| Stored procedure (Execute) | ❌ | ❌ | ❌ | ❌ | — | ✅ match | -| Control-plane (db / container / offer) | ❌ | ❌ | ❌ | ❌ | — | ✅ match | - -## Differences that matter - -| # | Difference | Detail | Severity | -|---|---|---|---| -| 1 | aggregate / GROUP BY / DISTINCT / TOP / LIMIT cross-partition | .NET runs them; its cross-partition merge is on the format-agnostic `CosmosElement` model, so binary works for free. Rust's `validate_query_info` (`dataflow/planner.rs`) still **rejects these in any encoding** — the aggregate/GROUP BY/DISTINCT merge engine does not exist yet. (Streaming ORDER BY with scalar sort keys is now supported — see PR #4800 — and binary works for it because response decode is format-agnostic.) | Real capability gap (not binary-specific) | -| 2 | Delete negotiation | .NET's `IsPointOperationSupportedForBinaryEncoding` includes `Delete`; Rust's `supports_binary_encoding` / `supports_binary_response` exclude it. Low impact (delete carries no request body and typically no response body), but Rust does not advertise binary for delete. | Minor | -| 3 | Patch mechanism | .NET Patch is a real server op and is **not** binary-negotiated. Rust Patch is a client-side Read-Modify-Write, so its internal Read/Replace **are** binary-encoded when enabled. Different architecture; both functionally correct. | Cosmetic / architectural | -| 4 | Negotiation header value | .NET query default = `"JsonText,CosmosBinary"`; .NET point ops = `"CosmosBinary"`. Rust = `"CosmosBinary"` for all paths (`BINARY_NEGOTIATION_FORMATS`). Rust always forces binary rather than advertising "either". | Minor wire diff | - -## Similarities (matched by design) - -- Point item ops (create/read/replace/upsert) encode requests and decode responses identically. -- Single-partition and passthrough cross-partition queries: text request body, negotiated binary response, per-page binary decode. -- Query request body always stays text (`application/query+json` is a query spec, not a document). -- Change feed / ReadFeed excluded from binary negotiation (the backend returns binary for ReadFeed-with-partition-key as a known bug). -- Batch / bulk / stored procedures / control-plane resources never use binary JSON. -- Response decode is a single format-agnostic choke point on both sides (first-byte `0x80` detection). - -## Bottom line - -For point operations, single-partition queries, and passthrough cross-partition -queries, Rust and .NET are **functionally equivalent** on binary encoding. - -Actionable divergences: - -- **#1** is the only user-facing gap, and it is a missing query engine, not a - binary issue — deferred. Streaming ORDER BY (scalar sort keys) already landed via - PR #4800 with binary working for free; the remaining aggregate/GROUP BY/DISTINCT - engine should likewise use a format-agnostic value model (like .NET's - `CosmosElement`) so binary support is inherent, not retrofitted. -- **#2 (Delete)** is a genuine small binary-scope divergence: add `Delete` to the - Rust predicates to match .NET's point-operation binary scope exactly. -- **#3, #4** are mechanism / wire nuances with no functional impact. - -## Key source references - -Rust: - -- `src/models/mod.rs` — `OperationType::supports_binary_encoding` / `supports_binary_response`. -- `src/driver/cosmos_driver.rs` — `binary_encodes_request_body`, `binary_negotiates_response`, `apply_request_binary_encoding`, `apply_response_negotiation`, `BINARY_NEGOTIATION_FORMATS`. -- `src/models/response_body.rs` — `deserialize_response`, `into_single`, `into_items` (text-only splitter note). -- `src/driver/dataflow/planner.rs` — `validate_query_info` (rejects aggregate / GROUP BY / DISTINCT / TOP / LIMIT). -- `src/driver/dataflow/order_by.rs`, `src/driver/dataflow/streaming_ordered_merge.rs` — streaming ORDER BY merge (PR #4800). -- `src/driver/dataflow/query_response.rs` — `parse_envelope_page` transcodes a binary ORDER BY page to text (`binary_json::transcode_to_text`) before the envelope parse, so the merge is format-agnostic like the point/passthrough decode path. - -.NET (`Azure/azure-cosmos-dotnet-v3`): - -- `src/Handler/RequestInvokerHandler.cs` — `IsPointOperationSupportedForBinaryEncoding` (create/replace/delete/read/upsert), sets `SupportedSerializationFormats = CosmosBinary` for point ops. -- `src/RequestOptions/QueryRequestOptions.cs` — `PopulateRequestOptions` sets the header for `OperationType.Query` only (with the ReadFeed backend-bug comment). -- `src/Query/v2Query/DocumentQueryExecutionContextBase.cs` — `DefaultSupportedSerializationFormats = "JsonText,CosmosBinary"`. -- `src/Resource/Container/ContainerCore.Items.cs` — `GetTargetRequestSerializationFormat` / `GetTargetResponseSerializationFormat`. diff --git a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_HLD.md b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_HLD.md index 722967ef1c..e08d37a6b4 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_HLD.md +++ b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_HLD.md @@ -115,15 +115,18 @@ flowchart TD subgraph DRIVER_REQ["Driver — request side (schema-agnostic)"] DRV["execute_operation"] - DRV --> GATE{"binary_encoding_applies?
(Document + point op)
AND binary.enabled"} - GATE -->|no| WIRE - GATE -->|yes| APPLY["apply_request_binary_encoding"] + DRV --> GATE{"binary_encodes_request_body?
(Document + point op)
AND binary.enabled"} + GATE -->|no| PLAN + GATE -->|yes| APPLY["apply_request_binary_encoding
(body transcode only)"] APPLY --> CHK{"body already binary?"} CHK -->|yes / empty| PASS["pass through unchanged"] CHK -->|no = text| TRANS["transcode_to_binary(bytes)"] - PASS --> HDR - TRANS --> HDR["advertise CosmosBinary
(supported-serialization-formats header)"] - HDR --> WIRE["send request to Cosmos"] + PASS --> PLAN + TRANS --> PLAN["plan_operation →
apply_response_negotiation"] + PLAN --> NEG{"binary_negotiates_response?
(point op + query)
AND binary.enabled"} + NEG -->|no| WIRE["send request to Cosmos"] + NEG -->|yes| HDR["advertise CosmosBinary
(supported-serialization-formats header)"] + HDR --> WIRE end WIRE --> COSMOS[("Cosmos DB service")] @@ -394,8 +397,8 @@ opts.binary_encoding_request_text_response = 2; /* 2 = true */ ## Deferred work -* **Query response negotiation** — **done.** A `query_items` call now advertises a binary response via `x-ms-cosmos-supported-serialization-formats` (set once at the `plan_operation` choke point that every per-page request flows through). The query request body is a `application/query+json` spec, not a document, so it intentionally stays text — there is no query request-body encoding. The query *response* decodes via the shared choke point. -* **Cross-partition / ORDER BY merge on binary bytes** — the SDK's `parse_envelope_page` merge path parses page envelopes with text-only `serde_json` + `RawValue`, so cross-partition ordering over *binary* item bytes is not yet covered. Single-partition and `into_single` query drains already round-trip binary end to end. +* **Query response negotiation** — **done.** A `query_items` call now advertises a binary response via `x-ms-cosmos-supported-serialization-formats` (set once at the `plan_operation` choke point that every per-page request flows through). The query request body is a `application/query+json` spec, not a document, so it intentionally stays text — there is no query request-body encoding. The query *response* decodes via the shared choke point. **Standard-gateway path only:** the Gateway 2.0 / thin-client path re-encodes the request as an RNTBD metadata token list that has no `SupportedSerializationFormats` token, so the header is dropped there and the service returns text (which still decodes). Adding the RNTBD token is a **follow-up**. +* **Cross-partition / ORDER BY merge on binary bytes** — **scalar-key streaming ORDER BY now works on binary.** `parse_envelope_page` transcodes a binary-negotiated page to text before its text-only `serde_json` + `RawValue` envelope parse, so cross-partition ORDER BY over binary item bytes round-trips end to end. Single-partition and `into_single` query drains already round-trip binary directly. Remaining gap: **aggregate / GROUP BY / DISTINCT** merges over binary (and a binary-aware envelope parse that avoids the transcode copy — see the architecture note). * **Binary feed responses** — the `into_items` feed splitter scans **text** JSON, so binary `Documents` envelopes cannot be sliced by that splitter yet. (The SDK query path uses `into_single`, which is unaffected.) `ReadFeed` / change feed is excluded from binary negotiation: the backend does not honor the header for it. * **`patch`** — excluded from binary encoding for now (the driver's request-side encode intentionally skips patch); transactional `batch` / `bulk` are deferred by spec. * **Cross-implementation vectors** — validate against captured real .NET / Java binary output. diff --git a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_SPEC.md b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_SPEC.md index 66e90af99a..f1e9b72c00 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_SPEC.md +++ b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_SPEC.md @@ -68,7 +68,7 @@ text-equivalent results. | ------------------------------------------- | :----------: | :-----------: | --------- | | `read_item` | — | decode | ✅ done | | `create_item` / `upsert_item` / `replace_item` | encode | decode | ✅ done | -| `query_items` | — (text spec) | decode | ✅ done (response negotiated) | +| `query_items` | — (text spec) | decode | ✅ done (response negotiated, standard gateway) | | `delete_item` | — | — | n/a | | `patch_item` | deferred | deferred | deferred | | transactional batch / bulk | deferred | deferred | deferred | @@ -79,6 +79,14 @@ response envelopes. A query now also **advertises** a binary response via the `application/query+json` query spec (not a document) and intentionally stays text — there is no query request-body encoding to do. +> **Gateway 2.0 limitation (follow-up).** Response negotiation is honored on the +> **standard gateway** path only. On the Gateway 2.0 / thin-client path the +> request is re-encoded as an RNTBD metadata token list, and there is no +> `SupportedSerializationFormats` token, so the header cannot survive the +> thin-client wrapping — a query against a Gateway 2.0 account silently returns +> text (which still decodes correctly). Adding the RNTBD token is tracked as a +> follow-up. + ## 3. Background: the .NET reference .NET PR [#4652](https://github.com/Azure/azure-cosmos-dotnet-v3/pull/4652) @@ -443,7 +451,7 @@ it through. It sets the option via a `with_binary_encoding` helper on (`operation_options_view`) as every other option, so a default set at the runtime/account layer is honored. Two independent gates apply. Request-body transcoding is honored **only for point item operations** -(`OperationType::supports_binary_encoding`: create, read, replace, upsert, +(`OperationType::supports_binary_request_body`: create, read, replace, upsert, delete). Response negotiation (the `x-ms-cosmos-supported-serialization-formats` header) covers the same point item ops **plus query** (`OperationType::supports_binary_response`) — a query advertises a binary @@ -538,7 +546,7 @@ rare forms is a possible future optimization.) `CosmosResponse::transcode_body_to_text` applies the conversion to the assembled response body. - `azure_data_cosmos_driver/src/models/mod.rs`: - `OperationType::supports_binary_encoding` gates binary encoding to point item + `OperationType::supports_binary_request_body` gates binary encoding to point item operations (create/read/replace/upsert/delete). - `azure_data_cosmos_driver/src/driver/cosmos_driver.rs`: `execute_operation` resolves `binary_encoding` via the layered `operation_options_view`, applies diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs index 2d9d87dbad..2938ee02f7 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs @@ -2620,12 +2620,8 @@ impl CosmosDriver { } else { crate::options::BinaryEncodingOptions::default() }; - let operation = if binary.enabled { - Self::apply_request_binary_encoding( - operation, - encodes_request_body, - negotiates_response, - )? + let operation = if binary.enabled && encodes_request_body { + Self::apply_request_binary_encoding(operation)? } else { operation }; @@ -2667,7 +2663,7 @@ impl CosmosDriver { operation_type: crate::models::OperationType, ) -> bool { resource_type == crate::models::ResourceType::Document - && operation_type.supports_binary_encoding() + && operation_type.supports_binary_request_body() } /// Whether an operation may advertise a binary **response** via the @@ -2715,56 +2711,43 @@ impl CosmosDriver { } } - /// Applies request-side binary encoding to an operation as two independent - /// steps: - /// * when `encodes_request_body`, transcodes a text request body to - /// Cosmos binary JSON (an already-binary or empty body is passed - /// through); - /// * when `negotiates_response`, advertises binary responses via the - /// `x-ms-cosmos-supported-serialization-formats` header. + /// Transcodes an operation's **text** request body to Cosmos binary JSON. /// - /// A query op negotiates a binary response without transcoding its body. + /// Only the request body is touched — response negotiation (the + /// `x-ms-cosmos-supported-serialization-formats` header) is owned solely by + /// [`apply_response_negotiation`], which every operation reaches through + /// `plan_operation`. A body that is already binary (the SDK's typed fast + /// path) or empty is left in place — no clone. /// /// This is schema-agnostic — it operates on the raw body bytes — so a /// caller that deals only in text JSON gets a binary wire without encoding /// anything itself. + /// + /// [`apply_response_negotiation`]: CosmosDriver::apply_response_negotiation fn apply_request_binary_encoding( operation: CosmosOperation, - encodes_request_body: bool, - negotiates_response: bool, ) -> crate::error::Result { // Transcode a non-empty *text* body to binary. A body that is already // binary (the SDK's typed fast path) or empty is left in place — no // clone — so only genuinely text bodies pay the conversion. - let transcoded = if encodes_request_body { - match operation.body() { - Some(body) if !body.is_empty() && !crate::binary_json::is_binary(body) => { - Some(crate::binary_json::transcode_to_binary(body).map_err(|e| { - crate::error::CosmosError::builder() - .with_status( - crate::error::CosmosStatus::SERIALIZATION_REQUEST_BODY_INVALID, - ) - .with_message(format!( - "failed to transcode text request body to Cosmos binary JSON: {e}" - )) - .with_source(e) - .build() - })?) - } - _ => None, + let transcoded = match operation.body() { + Some(body) if !body.is_empty() && !crate::binary_json::is_binary(body) => { + Some(crate::binary_json::transcode_to_binary(body).map_err(|e| { + crate::error::CosmosError::builder() + .with_status(crate::error::CosmosStatus::SERIALIZATION_REQUEST_BODY_INVALID) + .with_message(format!( + "failed to transcode text request body to Cosmos binary JSON: {e}" + )) + .with_source(e) + .build() + })?) } - } else { - None + _ => None, }; - let operation = match transcoded { + Ok(match transcoded { Some(bytes) => operation.with_body(bytes), None => operation, - }; - if negotiates_response { - Ok(operation.with_supported_serialization_formats(BINARY_NEGOTIATION_FORMATS)) - } else { - Ok(operation) - } + }) } /// Executes a singleton operation (operations which return only a single result). @@ -6348,7 +6331,7 @@ mod tests { // ── apply_request_binary_encoding (schema-agnostic request-side encode) ── #[test] - fn binary_encoding_applies_only_to_document_item_ops() { + fn binary_encodes_request_body_only_for_document_item_ops() { use crate::models::{OperationType, ResourceType}; // Point item ops on `Document` are the only combinations whose request @@ -6455,13 +6438,13 @@ mod tests { #[test] fn apply_request_binary_encoding_transcodes_text_body_to_binary() { // A caller (e.g. FFI) hands a TEXT JSON body; the driver transcodes it - // to Cosmos binary JSON and advertises binary responses. The caller - // never encoded binary itself. + // to Cosmos binary JSON. The caller never encoded binary itself. Response + // negotiation is a separate concern owned by `apply_response_negotiation`. let text = serde_json::to_vec(&serde_json::json!({ "id": "doc1", "n": 7 })).unwrap(); assert!(!crate::binary_json::is_binary(&text)); let op = binary_encoding_test_operation(text); - let op = CosmosDriver::apply_request_binary_encoding(op, true, true).unwrap(); + let op = CosmosDriver::apply_request_binary_encoding(op).unwrap(); let body = op.body().expect("body present"); assert!( @@ -6473,13 +6456,6 @@ mod tests { crate::binary_json::decode(body).unwrap(), serde_json::json!({ "id": "doc1", "n": 7 }), ); - // Advertises binary responses. - assert_eq!( - op.request_headers() - .supported_serialization_formats - .as_deref(), - Some("CosmosBinary"), - ); } #[test] @@ -6489,15 +6465,9 @@ mod tests { // through unchanged. let binary = crate::binary_json::encode(&serde_json::json!({ "id": "doc1", "n": 7 })); let op = binary_encoding_test_operation(binary.clone()); - let op = CosmosDriver::apply_request_binary_encoding(op, true, true).unwrap(); + let op = CosmosDriver::apply_request_binary_encoding(op).unwrap(); assert_eq!(op.body().unwrap(), binary.as_slice()); - assert_eq!( - op.request_headers() - .supported_serialization_formats - .as_deref(), - Some("CosmosBinary"), - ); } #[test] @@ -6505,7 +6475,7 @@ mod tests { // A body that is neither binary nor valid JSON surfaces as a // request-body serialization error. let op = binary_encoding_test_operation(b"{not json".to_vec()); - let err = CosmosDriver::apply_request_binary_encoding(op, true, true).unwrap_err(); + let err = CosmosDriver::apply_request_binary_encoding(op).unwrap_err(); assert_eq!( err.status().sub_status(), Some(crate::error::SubStatusCode::SERIALIZATION_REQUEST_BODY_INVALID), @@ -6513,31 +6483,26 @@ mod tests { } #[test] - fn apply_request_binary_encoding_query_negotiates_response_without_transcoding_body() { - // A query op negotiates a binary *response* but must leave its - // `application/query+json` body untouched (text), because the body is a - // query spec, not a document. - let container = epk_test_container(r#"{"paths":["/pk"],"version":2}"#); - let query_body = - serde_json::to_vec(&serde_json::json!({ "query": "SELECT * FROM c" })).unwrap(); - let op = CosmosOperation::query_items(container, Some(FeedRange::full())) - .with_body(query_body.clone()); - - // encodes_request_body = false (query), negotiates_response = true. - let op = CosmosDriver::apply_request_binary_encoding(op, false, true).unwrap(); - - // Body is unchanged text — never transcoded to binary. - assert_eq!(op.body().unwrap(), query_body.as_slice()); + fn query_negotiates_response_but_never_encodes_its_request_body() { + use crate::models::{OperationType, ResourceType}; + // A query op negotiates a binary *response* but must never have its + // `application/query+json` body transcoded, because the body is a query + // spec, not a document. The two gates encode exactly that: query is in + // the response-negotiation set but excluded from request-body encoding, + // so `apply_request_binary_encoding` is never reached for a query. assert!( - !crate::binary_json::is_binary(op.body().unwrap()), - "query body must remain text on the wire", + !CosmosDriver::binary_encodes_request_body( + ResourceType::Document, + OperationType::Query + ), + "query request body must never be binary-encoded", ); - // Still advertises a binary response. - assert_eq!( - op.request_headers() - .supported_serialization_formats - .as_deref(), - Some("CosmosBinary"), + assert!( + CosmosDriver::binary_negotiates_response( + ResourceType::Document, + OperationType::Query + ), + "query must negotiate a binary response", ); } } diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs index 8e5bf867c0..dbad3c0b34 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs @@ -251,9 +251,13 @@ fn parse_query_body(body: Option<&[u8]>) -> crate::error::Result, @@ -307,13 +311,22 @@ pub(crate) fn parse_envelope_page( } }; // Binary-negotiated ORDER BY pages arrive as Cosmos binary JSON; the - // envelope parse below is text-only, so transcode first (no-op for text). - let bytes = crate::binary_json::transcode_to_text(&bytes).map_err(|e| { - envelope_error(format!( - "failed to transcode binary ORDER BY envelope page to text: {e}" - )) - })?; - let feed: RawFeedBody = serde_json::from_slice(&bytes).map_err(|e| { + // envelope parse below is text-only, so transcode first. Text pages (the + // default, since binary is opt-in) skip the transcode entirely and parse + // the borrowed bytes directly — `transcode_to_text` would otherwise copy + // the whole page into a fresh `Vec`, taxing every existing ORDER BY + // user for a feature they did not enable. + let feed: RawFeedBody = if crate::binary_json::is_binary(&bytes) { + let text = crate::binary_json::transcode_to_text(&bytes).map_err(|e| { + envelope_error(format!( + "failed to transcode binary ORDER BY envelope page to text: {e}" + )) + })?; + serde_json::from_slice(&text) + } else { + serde_json::from_slice(&bytes) + } + .map_err(|e| { body_error( "failed to parse rewritten-query backend page as a feed body", e, diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/models/mod.rs b/sdk/cosmos/azure_data_cosmos_driver/src/models/mod.rs index 82db665d76..9244414510 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/models/mod.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/models/mod.rs @@ -649,7 +649,7 @@ impl OperationType { /// negotiates a binary *response* — see [`supports_binary_response`]. /// /// [`supports_binary_response`]: OperationType::supports_binary_response - pub(crate) fn supports_binary_encoding(self) -> bool { + pub(crate) fn supports_binary_request_body(self) -> bool { matches!( self, OperationType::Create @@ -661,7 +661,7 @@ impl OperationType { /// True for the ops that may negotiate a binary **response** via the /// `x-ms-cosmos-supported-serialization-formats` header. This is a superset - /// of [`supports_binary_encoding`](OperationType::supports_binary_encoding): + /// of [`supports_binary_request_body`](OperationType::supports_binary_request_body): /// the point item ops plus `Query` / `SqlQuery`. /// /// `ReadFeed` / change feed is intentionally excluded: the backend only @@ -669,7 +669,7 @@ impl OperationType { /// for a `ReadFeed`-with-partition-key request as a known bug — so change /// feed must never advertise binary. pub(crate) fn supports_binary_response(self) -> bool { - self.supports_binary_encoding() + self.supports_binary_request_body() || matches!(self, OperationType::Query | OperationType::SqlQuery) } @@ -915,7 +915,7 @@ mod tests { use serde::{Deserialize, Serialize}; #[test] - fn supports_binary_encoding_covers_only_bodied_point_ops() { + fn supports_binary_request_body_covers_only_bodied_point_ops() { // Matches the binary-encoding spec §2 scope table: create/read/replace/ // upsert. `delete` is excluded (no request or response body); query, // feed, batch, and stored-procedure paths are deferred. @@ -925,7 +925,10 @@ mod tests { OperationType::Replace, OperationType::Upsert, ] { - assert!(op.supports_binary_encoding(), "{op:?} should be supported"); + assert!( + op.supports_binary_request_body(), + "{op:?} should be supported" + ); } for op in [ OperationType::Delete, @@ -937,7 +940,7 @@ mod tests { OperationType::Patch, ] { assert!( - !op.supports_binary_encoding(), + !op.supports_binary_request_body(), "{op:?} should not be supported" ); } From 6c8052c3ec2665a09ad1ce7c2ed0eee59f8f494c Mon Sep 17 00:00:00 2001 From: kundadebdatta Date: Wed, 12 Aug 2026 16:11:03 -0700 Subject: [PATCH 06/13] Cosmos: fix rustfmt in query binary-negotiation test --- .../azure_data_cosmos_driver/src/driver/cosmos_driver.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs index 2938ee02f7..d936beb695 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs @@ -6498,10 +6498,7 @@ mod tests { "query request body must never be binary-encoded", ); assert!( - CosmosDriver::binary_negotiates_response( - ResourceType::Document, - OperationType::Query - ), + CosmosDriver::binary_negotiates_response(ResourceType::Document, OperationType::Query), "query must negotiate a binary response", ); } From f23898a1650f6f8ef5336838d4c582b0dbd652cd Mon Sep 17 00:00:00 2001 From: kundadebdatta Date: Wed, 12 Aug 2026 17:50:10 -0700 Subject: [PATCH 07/13] Cosmos: fix binary ORDER BY integer coercion + close query merge test gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../tests/binary_roundtrip_fuzzer.rs | 67 +++++++--- .../binary_round_trip.rs | 118 ++++++++++++++++- .../integration_tests/order_by_resume.rs | 8 +- .../src/driver/dataflow/query_response.rs | 124 +++++++++++++++++- 4 files changed, 289 insertions(+), 28 deletions(-) diff --git a/sdk/cosmos/azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs b/sdk/cosmos/azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs index dd29880fad..1de1bef4f4 100644 --- a/sdk/cosmos/azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs +++ b/sdk/cosmos/azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs @@ -1919,6 +1919,32 @@ async fn assert_typed_integer_probe( got, sent, "{context}: typed integer probe round-trip changed" ); + + // Also decode the same wide-integer probe through a cross-partition binary + // ORDER BY query. This drives the streaming-merge envelope decode + // (`build_page` re-encodes to binary so the SDK's `deserialize_integer` + // coercion runs), which the point-op read above does not exercise. A merge + // that emitted text instead would fail here with `invalid type: floating + // point, expected u64` for the `wide` field — the text/binary divergence. + let order_by = with_transient_retry("int-probe-order-by", context, || async { + let query = Query::from("SELECT * FROM c WHERE c.id = @id ORDER BY c.id") + .with_parameter("@id", id.as_str())?; + let iter = container + .query_items::(query, FeedScope::full_container(), None) + .await?; + iter.try_collect::>().await + }) + .await?; + assert_eq!( + order_by.len(), + 1, + "{context}: typed integer ORDER BY probe expected exactly one item, got {}", + order_by.len(), + ); + assert_eq!( + order_by[0], sent, + "{context}: typed integer probe changed through the binary ORDER BY merge" + ); Ok(()) } @@ -1972,14 +1998,13 @@ where } /// Queries the just-written item back and asserts it round-trips, covering the -/// query binary-response decode path (which point ops do not exercise). Runs -/// two shapes so both query pipelines are hit under binary encoding: -/// -/// * a single-partition query (`WHERE c.id = @id` scoped to the item's -/// partition) — the passthrough decode path, and -/// * a cross-partition streaming `ORDER BY` query (`... ORDER BY c.id` over the -/// full container) — the k-way merge, whose per-page envelope decode is the -/// binary path added for query support. +/// query binary-response decode path (which point ops do not exercise). Always +/// runs a single-partition query (the passthrough decode path). When +/// `include_cross_partition_order_by` is set, also runs a cross-partition +/// streaming `ORDER BY` query — the k-way merge, whose per-page envelope decode +/// is the binary path added for query support. That fan-out is the most +/// expensive query shape and adds no binary coverage on the text-control config, +/// so callers gate it to the binary configs. /// /// Both filter on the unique `id`, so each returns exactly this item. async fn assert_query_roundtrip( @@ -1990,7 +2015,8 @@ async fn assert_query_roundtrip( sent_hash: &[u8; 32], doc: &Map, context: &str, -) -> Result<(), Box> { + include_cross_partition_order_by: bool, +) -> Result> { let single_partition = with_transient_retry("query", context, || async { let query = Query::from("SELECT * FROM c WHERE c.id = @id").with_parameter("@id", id)?; let iter = container @@ -2008,6 +2034,10 @@ async fn assert_query_roundtrip( "query", ); + if !include_cross_partition_order_by { + return Ok(1); + } + let order_by = with_transient_retry("query-order-by", context, || async { let query = Query::from("SELECT * FROM c WHERE c.id = @id ORDER BY c.id") .with_parameter("@id", id)?; @@ -2026,7 +2056,7 @@ async fn assert_query_roundtrip( "query-order-by", ); - Ok(()) + Ok(2) } /// Asserts a query returned exactly the one expected item and that it @@ -2255,11 +2285,13 @@ async fn binary_encoding_roundtrip_fuzz() -> Result<(), Box> { // Four point-op round-trips this config: create, read, replace, upsert. checked += 4; - // QUERY the item back (single-partition + cross-partition ORDER BY). - // Covers the query binary-response decode path — including the - // streaming ORDER BY per-page envelope decode — which the point ops - // above do not exercise. - assert_query_roundtrip( + // QUERY the item back. The single-partition passthrough query runs + // on every config; the expensive cross-partition ORDER BY fan-out — + // whose per-page envelope decode is the binary path added for query + // support — runs only on the binary configs, where it adds coverage + // (on text-control it would just cost time). + let include_order_by = *label != "text-control"; + let queries_checked = assert_query_roundtrip( &container, &pk, &id, @@ -2267,9 +2299,10 @@ async fn binary_encoding_roundtrip_fuzz() -> Result<(), Box> { &sent_hash, &doc, &context, + include_order_by, ) .await?; - checked += 2; + checked += queries_checked as u64; // The four ops above decode into `serde_json::Value` (→ // `deserialize_any`), so they do NOT cover the native typed-integer @@ -2289,7 +2322,7 @@ async fn binary_encoding_roundtrip_fuzz() -> Result<(), Box> { } println!( - "binary_roundtrip_fuzzer: DONE — {} documents × {} configs × (4 point ops + 2 queries) = {checked} round-trips, all canonical-equal (seed={})", + "binary_roundtrip_fuzzer: DONE — {} documents × {} configs (4 point ops each + 1–2 queries; cross-partition ORDER BY on binary configs only) = {checked} round-trips, all canonical-equal (seed={})", cfg.iterations, configs.len(), cfg.seed diff --git a/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs b/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs index d62a7a9377..d7b3c6bbf1 100644 --- a/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs +++ b/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs @@ -31,7 +31,7 @@ use std::sync::{Arc, Mutex}; const EMULATOR_GATEWAY_URL: &str = "https://eastus.emulator.local"; -#[derive(Debug, Serialize, Deserialize, PartialEq)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] struct TestItem { id: String, pk: String, @@ -104,7 +104,15 @@ async fn build_container(db_name: &str, binary: bool) -> ContainerClient { /// Like [`build_container`], but provisions `partition_count` physical /// partitions so a full-container query fans out across ranges (the passthrough /// cross-partition path). Binary encoding is enabled. -async fn build_multi_partition_container(db_name: &str, partition_count: u32) -> ContainerClient { +/// Builds a multi-partition container with binary encoding enabled, attaching a +/// [`QueryRequestRecorder`] and returning it, so a cross-partition query test can +/// assert that every fan-out page actually advertised a binary response (a plain +/// results-match assertion would still pass if negotiation silently broke and the +/// emulator returned text, since text decodes fine). +async fn build_multi_partition_container_with_recorder( + db_name: &str, + partition_count: u32, +) -> (ContainerClient, Arc) { let config = VirtualAccountConfig::new(vec![VirtualRegion::new( "East US", azure_core::http::Url::parse(EMULATOR_GATEWAY_URL).unwrap(), @@ -112,7 +120,11 @@ async fn build_multi_partition_container(db_name: &str, partition_count: u32) -> .unwrap() .with_consistency(ConsistencyLevel::Session); - let emulator = std::sync::Arc::new(InMemoryEmulatorHttpClient::new(config)); + let recorder = Arc::new(QueryRequestRecorder::default()); + let emulator = std::sync::Arc::new( + InMemoryEmulatorHttpClient::new(config) + .with_request_observer(Arc::clone(&recorder) as Arc), + ); let store = emulator.store(); store.create_database(db_name); store.create_container_with_config( @@ -147,11 +159,12 @@ async fn build_multi_partition_container(db_name: &str, partition_count: u32) -> .await .unwrap(); - client + let container = client .database_client(db_name) .container_client("items") .await - .unwrap() + .unwrap(); + (container, recorder) } /// With binary enabled, an item written through the SDK is binary-encoded on the @@ -570,10 +583,13 @@ async fn binary_query_negotiates_response_and_round_trips() { /// fans out across multiple physical partitions. Each partition's page is /// returned as an independent binary `Documents` envelope, decoded per page /// through the shared choke point — so every item round-trips regardless of -/// which partition served it. +/// which partition served it. The attached recorder asserts every fan-out page +/// actually advertised a binary response, so a silently-broken negotiation +/// (which would return text that still decodes) cannot pass this test. #[tokio::test] async fn binary_cross_partition_query_round_trips() { - let container = build_multi_partition_container("bin-xpart-query", 3).await; + let (container, recorder) = + build_multi_partition_container_with_recorder("bin-xpart-query", 3).await; // Spread items across several partition keys so the fan-out spans ranges. let items: Vec = (0..12) @@ -607,4 +623,92 @@ async fn binary_cross_partition_query_round_trips() { results, expected, "cross-partition query results must round-trip through binary", ); + + assert_query_advertised_binary(&recorder); +} + +/// **Cross-partition binary ORDER BY** — the centerpiece merge path. A +/// full-container `SELECT * ... ORDER BY c.value` fans out across partitions and +/// the driver runs the streaming k-way merge, whose per-page envelope decode +/// (`parse_envelope_page`) is exactly the binary path added for query support. +/// Unlike the passthrough test above, this exercises the *rewritten-envelope* +/// binary decode inside the merge, in an always-run test (previously covered +/// only by a mocked driver test and the live-only fuzzer). +#[tokio::test] +async fn binary_cross_partition_order_by_merges_and_round_trips() { + let (container, recorder) = + build_multi_partition_container_with_recorder("bin-xpart-order-by", 3).await; + + // Interleave values across partition keys so the global order differs from + // any single partition's local order — forcing the k-way merge to actually + // reorder across binary pages rather than concatenate. + let items: Vec = (0..12) + .map(|i| TestItem { + id: format!("o-{i}"), + pk: format!("pk{}", i % 4), + value: (i * 7) % 12, + note: format!("café ☃ {i}"), + }) + .collect(); + for item in &items { + container + .create_item(&item.pk, &item.id, item, Some(write_options_with_content())) + .await + .unwrap(); + } + + let iter = Box::pin(container.query_items( + Query::from("SELECT * FROM c ORDER BY c.value"), + FeedScope::full_container(), + None, + )) + .await + .unwrap(); + let results: Vec = Box::pin(iter.try_collect()).await.unwrap(); + + // The merge must emit items in global ascending `value` order — asserting the + // ordering (not just set membership) proves the binary pages were decoded and + // merged correctly, not merely concatenated. + let mut expected = items; + expected.sort_by(|a, b| a.value.cmp(&b.value).then_with(|| a.id.cmp(&b.id))); + let mut got = results.clone(); + // Stable tie-break on id only for comparison; the service orders equal keys + // arbitrarily, so normalize ties before comparing the full sequence. + got.sort_by(|a, b| a.value.cmp(&b.value).then_with(|| a.id.cmp(&b.id))); + assert_eq!( + got, expected, + "binary ORDER BY must round-trip every item through the merge", + ); + // The values themselves must already be globally non-decreasing as returned. + assert!( + results.windows(2).all(|w| w[0].value <= w[1].value), + "binary ORDER BY results must be in ascending value order, got {:?}", + results.iter().map(|r| r.value).collect::>(), + ); + + assert_query_advertised_binary(&recorder); +} + +/// Asserts a query recorder saw at least one query request and that every query +/// request advertised a binary response while keeping its body text. +fn assert_query_advertised_binary(recorder: &QueryRequestRecorder) { + let formats = recorder.negotiation_formats.lock().unwrap(); + assert!( + !formats.is_empty(), + "expected at least one query request to be recorded", + ); + for value in formats.iter() { + assert_eq!( + value.as_deref(), + Some("CosmosBinary"), + "every query fan-out page must advertise a binary response", + ); + } + let body_is_binary = recorder.body_is_binary.lock().unwrap(); + for is_binary in body_is_binary.iter() { + assert!( + !is_binary, + "query request body must stay text (application/query+json is a spec, not a document)", + ); + } } diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/integration_tests/order_by_resume.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/integration_tests/order_by_resume.rs index 1a6553ebfb..deeee579dc 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/integration_tests/order_by_resume.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/integration_tests/order_by_resume.rs @@ -355,8 +355,14 @@ fn array_envelope_page(rows: &[(&str, i64)], continuation: Option<&str>) -> Cosm } /// Extracts every item's `id` across `page`'s `Documents`, in wire order. +/// Auto-detects the page format: a binary-negotiated ORDER BY merge emits a +/// binary envelope (so the SDK's binary deserializer runs), which is transcoded +/// to text here before the text parse — mirroring the SDK's format-agnostic +/// decode choke point. fn ids_in_page(page: &CosmosResponse) -> Vec { - let value: serde_json::Value = serde_json::from_slice(page.body_bytes()).unwrap(); + let bytes = page.body_bytes(); + let text = crate::binary_json::transcode_to_text(bytes).unwrap(); + let value: serde_json::Value = serde_json::from_slice(&text).unwrap(); value["Documents"] .as_array() .unwrap() diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs index dbad3c0b34..315c23597c 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs @@ -376,6 +376,14 @@ pub(crate) struct PageAggregator { index_metrics: Option, query_metrics: Option, status: CosmosStatus, + /// Set once any absorbed backend page arrived as Cosmos binary JSON. When + /// true, [`build_page`](Self::build_page) emits the synthetic envelope as + /// binary so the SDK decodes it through the binary deserializer — which + /// applies the integral-`Double`→integer coercion that a passthrough binary + /// query already gets. Emitting text here would instead route the payloads + /// through the text deserializer, which hard-fails on a service-echoed + /// integral double for an integer field (the text/binary divergence). + emit_binary: bool, } impl Default for PageAggregator { @@ -388,6 +396,7 @@ impl Default for PageAggregator { index_metrics: None, query_metrics: None, status: CosmosStatus::new(azure_core::http::StatusCode::Ok), + emit_binary: false, } } } @@ -415,6 +424,9 @@ impl PageAggregator { let charge = response.headers().request_charge.unwrap_or_default(); self.request_charge = self.request_charge + charge; self.diagnostics_sources.push(response.diagnostics()); + if response_body_is_binary(response.body()) { + self.emit_binary = true; + } if let Some(id) = &response.headers().activity_id { self.activity_id = Some(id.clone()); } @@ -467,9 +479,12 @@ impl PageAggregator { /// final ordered list of raw item payloads. /// /// Body is a synthetic `{"_rid": "", "Documents": [...], "_count": N}` - /// envelope of `payloads`' bytes unmodified — the wire shape every - /// other feed node returns. `_rid` is left empty (per-item `_rid` - /// identifies each item, not the feed-level one). + /// envelope of `payloads`' bytes — the wire shape every other feed node + /// returns. `_rid` is left empty (per-item `_rid` identifies each item, not + /// the feed-level one). When the source pages were binary, the assembled + /// envelope is re-encoded to Cosmos binary JSON so the SDK's binary + /// deserializer (with integral-`Double`→integer coercion) decodes it, exactly + /// as it would a passthrough binary query; text sources are emitted verbatim. /// /// It's valid for no backend page to have been absorbed (page /// assembled entirely from previously-buffered rows); it then reports @@ -491,6 +506,23 @@ impl PageAggregator { body.extend_from_slice(payloads.len().to_string().as_bytes()); body.push(b'}'); + // When the source pages were binary, re-encode the assembled envelope to + // Cosmos binary JSON so the SDK decodes it through the binary + // deserializer (which coerces a service-echoed integral `Double` into an + // integer target). Emitting text here would route the same payloads + // through the text deserializer, which hard-fails on that double — the + // divergence a passthrough binary query does not have. Text sources keep + // the zero-extra-copy text envelope. + let body = if self.emit_binary { + crate::binary_json::transcode_to_binary(&body).map_err(|e| { + envelope_error(format!( + "failed to transcode merged ORDER BY envelope page to binary: {e}" + )) + })? + } else { + body + }; + let diagnostics = DiagnosticsContext::aggregate_sub_operations(&self.diagnostics_sources) .map(Arc::new) .unwrap_or_else(empty_diagnostics); @@ -528,6 +560,15 @@ fn empty_diagnostics() -> Arc { Arc::new(builder.complete()) } +/// Returns whether a backend page body is Cosmos binary JSON, so the merge can +/// mirror the source format when it re-emits the assembled envelope. +fn response_body_is_binary(body: &ResponseBody) -> bool { + match body { + ResponseBody::Bytes(b) => crate::binary_json::is_binary(b), + _ => false, + } +} + fn envelope_error(message: impl Into>) -> crate::error::CosmosError { crate::error::CosmosError::builder() .with_status(CosmosStatus::SERVICE_ORDER_BY_ENVELOPE_INVALID) @@ -796,6 +837,83 @@ mod tests { assert!(rows.is_empty()); } + #[test] + fn build_page_binary_source_emits_binary_and_coerces_integral_double() { + // Reproduces the text/binary ORDER BY divergence (#5028): a service + // echoes a document's integer field as an integral `Double`. A + // passthrough binary query coerces it into the integer target via the + // binary deserializer; the merge must do the same. Emitting the merged + // envelope as text instead would route the payload through the text + // deserializer, which hard-fails on the float for a `u64` field. + #[derive(serde::Deserialize, PartialEq, Debug)] + struct Doc { + id: String, + wide: u64, + } + + // Force a `Double` (not an integer marker) for `wide`, exactly as the + // service stores every number. `from_f64` serializes as `...0`, a float + // literal the text deserializer rejects for a `u64`. + let wide_double = 9_007_199_254_740_992.0_f64; // 2^53, exactly integral + let payload = serde_json::json!({ + "id": "d1", + "wide": serde_json::Number::from_f64(wide_double).unwrap(), + }); + let payload_raw = serde_json::value::to_raw_value(&payload).unwrap(); + + // Sanity: the text form of this payload cannot decode into `Doc` — this + // is exactly the failure the binary path must avoid. + assert!( + serde_json::from_str::(payload_raw.get()).is_err(), + "the text payload must reject a float into a u64 field (the divergence)" + ); + + let mut aggregator = PageAggregator::new(); + aggregator.emit_binary = true; + let response = aggregator.build_page(&[payload_raw]).unwrap(); + + // The merge emits a binary envelope so the SDK's binary deserializer runs. + let bytes = match response.body() { + ResponseBody::Bytes(b) => b.clone(), + other => panic!("expected a bytes body, got {other:?}"), + }; + assert!( + crate::binary_json::is_binary(&bytes), + "a binary source must produce a binary merged envelope" + ); + + // And that deserializer coerces the integral double into the u64 target. + #[derive(serde::Deserialize)] + struct Feed { + #[serde(alias = "Documents")] + documents: Vec, + } + let feed: Feed = crate::binary_json::from_slice(&bytes).unwrap(); + assert_eq!( + feed.documents, + vec![Doc { + id: "d1".to_owned(), + wide: 9_007_199_254_740_992, + }], + ); + } + + #[test] + fn build_page_text_source_emits_text_envelope() { + // A text source keeps the zero-extra-copy text envelope — no binary + // re-encode, so existing text ORDER BY users are unaffected. + let payload = serde_json::value::to_raw_value(&serde_json::json!({"id":"d1"})).unwrap(); + let aggregator = PageAggregator::new(); + let response = aggregator.build_page(&[payload]).unwrap(); + match response.body() { + ResponseBody::Bytes(b) => assert!( + !crate::binary_json::is_binary(b), + "a text source must stay text" + ), + other => panic!("expected a bytes body, got {other:?}"), + } + } + #[test] fn parse_envelope_page_rejects_missing_rid() { let body = ResponseBody::from_bytes( From 7da859aa09d745fa178d6dab83af4bd155d0e7e0 Mon Sep 17 00:00:00 2001 From: kundadebdatta Date: Wed, 12 Aug 2026 18:33:17 -0700 Subject: [PATCH 08/13] Cosmos: fix re-review doc/test findings A, B, C 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. --- .../docs/BINARY_ENCODING_SPEC.md | 8 ++- .../src/driver/cosmos_driver.rs | 71 +++++++++++++------ 2 files changed, 53 insertions(+), 26 deletions(-) diff --git a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_SPEC.md b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_SPEC.md index f1e9b72c00..ca7df97f61 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_SPEC.md +++ b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_SPEC.md @@ -451,8 +451,10 @@ it through. It sets the option via a `with_binary_encoding` helper on (`operation_options_view`) as every other option, so a default set at the runtime/account layer is honored. Two independent gates apply. Request-body transcoding is honored **only for point item operations** -(`OperationType::supports_binary_request_body`: create, read, replace, upsert, -delete). Response negotiation (the `x-ms-cosmos-supported-serialization-formats` +(`OperationType::supports_binary_request_body`: create, read, replace, upsert; +delete is excluded — it carries no body, though .NET's +`IsPointOperationSupportedForBinaryEncoding` does include it). Response +negotiation (the `x-ms-cosmos-supported-serialization-formats` header) covers the same point item ops **plus query** (`OperationType::supports_binary_response`) — a query advertises a binary response while keeping its `application/query+json` request body text. Feed @@ -547,7 +549,7 @@ rare forms is a possible future optimization.) assembled response body. - `azure_data_cosmos_driver/src/models/mod.rs`: `OperationType::supports_binary_request_body` gates binary encoding to point item - operations (create/read/replace/upsert/delete). + operations (create/read/replace/upsert; delete excluded — no body). - `azure_data_cosmos_driver/src/driver/cosmos_driver.rs`: `execute_operation` resolves `binary_encoding` via the layered `operation_options_view`, applies request-side transcoding for supported operation types, and transcodes the diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs index d936beb695..5d74a11c48 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs @@ -74,16 +74,21 @@ const ACCOUNT_PROPERTIES_CONNECTIVITY_MAX_RETRIES: u32 = 2; const ACCOUNT_PROPERTIES_CONNECTIVITY_BASE_DELAY: Duration = Duration::from_millis(100); /// Serialization formats advertised (`x-ms-cosmos-supported-serialization-formats`) -/// on point operations when binary encoding is enabled. Point operations -/// advertise `CosmosBinary` only — matching the .NET SDK's point-op default +/// when binary encoding is enabled, for every operation that negotiates a binary +/// response — point item ops **and** query (see +/// [`binary_negotiates_response`](CosmosDriver::binary_negotiates_response)). +/// Rust advertises `CosmosBinary` only — matching the .NET SDK's point-op default /// (`RequestInvokerHandler` sets `BinarySerializationFormat = /// SupportedSerializationFormats.CosmosBinary`) — so the service is required to /// reply in binary, preserving the read-side RU/COGS benefit the caller opted -/// into. When the caller also asked for a text payload -/// (`request_text_response`), the driver transcodes the guaranteed-binary -/// response back to text after receiving it, keeping the wire binary in both -/// directions. (The broader `JsonText,CosmosBinary` negotiation applies to -/// query/feed, which is not yet wired.) +/// into. When the caller also asked for a text payload (`request_text_response`), +/// the driver transcodes the guaranteed-binary response back to text after +/// receiving it, keeping the wire binary in both directions. +/// +/// Note: .NET's *query* default is the broader `JsonText,CosmosBinary` ("send +/// either, service chooses"); Rust deliberately forces `CosmosBinary` for query +/// too. Decode is format-agnostic (first-byte detection), so binary-only costs +/// nothing on the decode side. const BINARY_NEGOTIATION_FORMATS: &str = "CosmosBinary"; fn should_retry_account_properties_connectivity_error( @@ -6482,24 +6487,44 @@ mod tests { ); } - #[test] - fn query_negotiates_response_but_never_encodes_its_request_body() { - use crate::models::{OperationType, ResourceType}; - // A query op negotiates a binary *response* but must never have its - // `application/query+json` body transcoded, because the body is a query - // spec, not a document. The two gates encode exactly that: query is in - // the response-negotiation set but excluded from request-body encoding, - // so `apply_request_binary_encoding` is never reached for a query. + #[tokio::test] + async fn query_negotiates_binary_response_without_transcoding_its_body() { + use crate::models::FeedRange; + + // Build a driver with binary encoding enabled and run a real query + // operation through the actual negotiation step + // (`apply_response_negotiation`, the sole header owner that every query + // reaches via `plan_operation`). The behavioral invariant: a query + // advertises a binary *response* while its `application/query+json` + // request body stays text — the body is a query spec, not a document. + let cosmos_runtime = CosmosDriverRuntimeBuilder::new().build().await.unwrap(); + let driver_options = DriverOptions::builder(test_account()).build(); + let driver = CosmosDriver::new(cosmos_runtime, driver_options) + .expect("CosmosDriver::new should succeed in tests"); + + let container = epk_test_container(r#"{"paths":["/pk"],"version":2}"#); + let query_body = + serde_json::to_vec(&serde_json::json!({ "query": "SELECT * FROM c" })).unwrap(); + let op = CosmosOperation::query_items(container, Some(FeedRange::full())) + .with_body(query_body.clone()); + + let options = OperationOptionsBuilder::new() + .with_binary_encoding(crate::options::BinaryEncodingOptions::new().with_enabled(true)) + .build(); + let op = driver.apply_response_negotiation(op, &options); + + // Body is unchanged text — never transcoded to binary. + assert_eq!(op.body().unwrap(), query_body.as_slice()); assert!( - !CosmosDriver::binary_encodes_request_body( - ResourceType::Document, - OperationType::Query - ), - "query request body must never be binary-encoded", + !crate::binary_json::is_binary(op.body().unwrap()), + "query body must remain text on the wire", ); - assert!( - CosmosDriver::binary_negotiates_response(ResourceType::Document, OperationType::Query), - "query must negotiate a binary response", + // Still advertises a binary response. + assert_eq!( + op.request_headers() + .supported_serialization_formats + .as_deref(), + Some("CosmosBinary"), ); } } From a535e11a835e0044f6b8f86e1b79db24ed9cc399 Mon Sep 17 00:00:00 2001 From: kundadebdatta Date: Wed, 12 Aug 2026 18:46:03 -0700 Subject: [PATCH 09/13] Cosmos: address review findings #7 and #12 #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). --- .../src/driver/cosmos_driver.rs | 69 +++++++++++++++---- .../src/in_memory_emulator/dispatch.rs | 7 ++ .../src/in_memory_emulator/operations.rs | 24 +++---- .../src/in_memory_emulator/response.rs | 31 +++++++++ 4 files changed, 106 insertions(+), 25 deletions(-) diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs index 5d74a11c48..17862d9c38 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs @@ -2638,7 +2638,16 @@ impl CosmosDriver { let response = Box::pin(async { let container = operation.container().cloned(); let mut plan = self - .plan_operation(operation, &options, None, &PlanOptions::default()) + .plan_operation_resolved( + operation, + &options, + None, + &PlanOptions::default(), + // Reuse the `binary` already resolved above so the shared + // `apply_response_negotiation` choke point does not re-resolve + // the same layered view for this operation. + Some(binary), + ) .await?; self.execute_plan(&mut plan, container, options).await }) @@ -2699,16 +2708,22 @@ impl CosmosDriver { &self, operation: CosmosOperation, options: &OperationOptions, + resolved_binary: Option, ) -> CosmosOperation { if !Self::binary_negotiates_response(operation.resource_type(), operation.operation_type()) { return operation; } - let binary = self - .operation_options_view(options) - .binary_encoding() - .cloned() - .unwrap_or_default(); + // Reuse a caller-resolved value when available (the `execute_operation` + // path already resolved it for the request-body gate); otherwise resolve + // it here — the query path reaches `plan_operation` directly without a + // prior resolution. + let binary = resolved_binary.unwrap_or_else(|| { + self.operation_options_view(options) + .binary_encoding() + .cloned() + .unwrap_or_default() + }); if binary.enabled { operation.with_supported_serialization_formats(BINARY_NEGOTIATION_FORMATS) } else { @@ -3306,7 +3321,33 @@ impl CosmosDriver { // here so every caller awaits a pointer-sized future instead of having // to pin at its own call site and rediscover this each time the state // grows. - Box::pin(self.plan_operation_inner(operation, options, continuation, plan_options)).await + Box::pin(self.plan_operation_inner(operation, options, continuation, plan_options, None)) + .await + } + + /// [`plan_operation`](Self::plan_operation) with a caller-resolved + /// [`BinaryEncodingOptions`](crate::options::BinaryEncodingOptions), letting + /// `execute_operation` avoid re-resolving the layered options view that it + /// already resolved for the request-body gate. The public `plan_operation` + /// entry point passes `None`, so the query path (which reaches the driver + /// here directly) resolves lazily at the negotiation choke point. + async fn plan_operation_resolved( + &self, + operation: CosmosOperation, + options: &OperationOptions, + continuation: Option<&ContinuationToken>, + plan_options: &PlanOptions, + resolved_binary: Option, + ) -> crate::error::Result { + operation.validate_addressing()?; + Box::pin(self.plan_operation_inner( + operation, + options, + continuation, + plan_options, + resolved_binary, + )) + .await } async fn plan_operation_inner( @@ -3315,6 +3356,7 @@ impl CosmosDriver { options: &OperationOptions, continuation: Option<&ContinuationToken>, plan_options: &PlanOptions, + resolved_binary: Option, ) -> crate::error::Result { if !self.initialized.load(Ordering::Acquire) { let endpoint = AccountEndpoint::from(self.options.account()); @@ -3330,11 +3372,12 @@ impl CosmosDriver { tracing::debug!(operation_type = ?operation.operation_type(), resource_type = ?operation.resource_type(), resource_reference = ?operation.resource_reference(), "planning operation"); // Advertise a binary response when negotiation applies (point item ops - // and query). Point ops also set this in `execute_operation`, but query - // reaches the driver through `plan_operation` directly, so this is the - // single choke point that covers every per-page request built from the - // resulting plan. The header set is idempotent. - let operation = self.apply_response_negotiation(operation, options); + // and query). This is the single choke point every operation passes + // through — including every per-page query request — so the header is + // owned here alone. `execute_operation` resolves the binary options once + // and forwards them via `resolved_binary` to avoid a second resolution; + // the query path passes `None` and resolves lazily. + let operation = self.apply_response_negotiation(operation, options, resolved_binary); // Share the operation across every Request node in the resulting plan. // Per-Request differences are layered on at execution time via @@ -6511,7 +6554,7 @@ mod tests { let options = OperationOptionsBuilder::new() .with_binary_encoding(crate::options::BinaryEncodingOptions::new().with_enabled(true)) .build(); - let op = driver.apply_response_negotiation(op, &options); + let op = driver.apply_response_negotiation(op, &options, None); // Body is unchanged text — never transcoded to binary. assert_eq!(op.body().unwrap(), query_body.as_slice()); diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/dispatch.rs b/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/dispatch.rs index 0ed88ae5ce..ae73fd5501 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/dispatch.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/dispatch.rs @@ -209,6 +209,13 @@ pub(crate) fn parse_request(request: &Request) -> ParsedRequest { // Matching the .NET flag enum, the presence of a `CosmosBinary` token (case- // insensitive, comma-separated) means the client can decode a binary // response. + // + // Fidelity note: this derives the flag from the header alone, whereas the + // real gateway honors the negotiation only for `OperationType.Query`. The + // emulator would therefore return binary for any negotiated feed, but the + // Rust driver only advertises binary for point ops and query + // (`binary_negotiates_response`), so no control-plane feed request that + // Rust sends carries the header — the divergence is unreachable in practice. let binary_response = headers .get_optional_str(&SUPPORTED_SERIALIZATION_FORMATS) .map(|v| { diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/operations.rs b/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/operations.rs index 2d62267d69..aa1aabba15 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/operations.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/operations.rs @@ -25,7 +25,7 @@ use super::response::headers::{ #[cfg(feature = "preview_dtx")] use super::response::headers::{ETAG, REQUEST_CHARGE, SESSION_TOKEN, SUBSTATUS}; use super::response::{ - error_response, success_response, success_response_with_format, ResponseBuilder, + error_response, success_response, success_response_with_format, ResponseBuilder, ResponseFormat, }; use super::ru_model::RuChargingModel; use super::session::SessionToken; @@ -2329,7 +2329,7 @@ fn success_feed_response( items: Vec, page_options: FeedPageOptions<'_>, feed_headers: FeedResponseHeaders, - binary: bool, + format: ResponseFormat, start: Instant, ) -> AsyncRawResponse { let (page, next) = match paginate_values( @@ -2346,7 +2346,7 @@ fn success_feed_response( let mut builder = success_response_with_format( StatusCode::Ok, &body, - binary, + format.is_binary(), 1.0, &feed_headers.session_token, start, @@ -2373,7 +2373,7 @@ fn success_document_feed_response( items: Vec, page_options: FeedPageOptions<'_>, feed_headers: FeedResponseHeaders, - binary: bool, + format: ResponseFormat, start: Instant, ) -> AsyncRawResponse { let (page, next) = match paginate_document_feed_items( @@ -2390,7 +2390,7 @@ fn success_document_feed_response( let mut builder = success_response_with_format( StatusCode::Ok, &body, - binary, + format.is_binary(), 1.0, &feed_headers.session_token, start, @@ -2494,7 +2494,7 @@ fn execute_query_feed( results, FeedPageOptions::from_request(parsed), feed_headers, - parsed.binary_response, + ResponseFormat::from(parsed.binary_response), start, ) } @@ -2519,7 +2519,7 @@ fn execute_document_query_feed( results, FeedPageOptions::from_request(parsed), feed_headers, - parsed.binary_response, + ResponseFormat::from(parsed.binary_response), start, ), Ok(None) => { @@ -2545,7 +2545,7 @@ fn execute_document_query_feed( results, FeedPageOptions::from_request(parsed), feed_headers, - parsed.binary_response, + ResponseFormat::from(parsed.binary_response), start, ) } @@ -2724,7 +2724,7 @@ fn handle_read_feed_databases( databases, FeedPageOptions::from_request(parsed), FeedResponseHeaders::none(), - false, + ResponseFormat::Text, start, ) } @@ -2790,7 +2790,7 @@ fn handle_read_feed_containers( containers, FeedPageOptions::from_request(parsed), FeedResponseHeaders::none(), - false, + ResponseFormat::Text, start, ) } @@ -2852,7 +2852,7 @@ fn handle_read_feed_offers( offers, FeedPageOptions::from_request(parsed), FeedResponseHeaders::none(), - false, + ResponseFormat::Text, start, ) } @@ -3194,7 +3194,7 @@ fn handle_read_feed_items( docs, FeedPageOptions::from_request(parsed), headers, - false, + ResponseFormat::Text, start, ) } diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/response.rs b/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/response.rs index b5b91799c6..34cd2f120d 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/response.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/response.rs @@ -263,6 +263,37 @@ pub(crate) fn success_response( /// Like [`success_response`], but encodes the body as Cosmos binary JSON when /// `binary` is set. Used by the item read/write handlers to honor a client that /// negotiated binary responses via `x-ms-cosmos-supported-serialization-formats`. +/// The serialization format the emulator emits for a feed response body. +/// +/// Replaces a bare positional `bool` on the feed builders so each call site +/// reads `ResponseFormat::Text` / `ResponseFormat::Binary` instead of a naked +/// `false` / `true` that a future edit could silently transpose with an +/// adjacent argument. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum ResponseFormat { + /// UTF-8 text JSON — the default for read-feed and control-plane responses. + Text, + /// Cosmos binary JSON — emitted only when the request negotiated it. + Binary, +} + +impl ResponseFormat { + /// Whether this format is Cosmos binary JSON. + pub(crate) fn is_binary(self) -> bool { + matches!(self, ResponseFormat::Binary) + } +} + +impl From for ResponseFormat { + fn from(binary: bool) -> Self { + if binary { + ResponseFormat::Binary + } else { + ResponseFormat::Text + } + } +} + pub(crate) fn success_response_with_format( status: StatusCode, body: &serde_json::Value, From d9a9cf9f7bf8062e35c4bb22eb83161241c0dc58 Mon Sep 17 00:00:00 2001 From: kundadebdatta Date: Thu, 13 Aug 2026 14:27:41 -0700 Subject: [PATCH 10/13] Cosmos: fix binary ORDER BY multi-page regression + review findings - #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. --- sdk/cosmos/azure_data_cosmos/CHANGELOG.md | 1 + .../binary_round_trip.rs | 35 ++++++++---- .../azure_data_cosmos_driver/CHANGELOG.md | 1 + .../src/driver/cosmos_driver.rs | 8 ++- .../integration_tests/order_by_resume.rs | 53 +++++++++++++++++++ .../src/driver/dataflow/query_response.rs | 17 ++++++ .../dataflow/streaming_ordered_merge.rs | 17 ++++++ .../src/in_memory_emulator/response.rs | 28 +++++----- .../src/models/response_body.rs | 17 +++--- 9 files changed, 146 insertions(+), 31 deletions(-) diff --git a/sdk/cosmos/azure_data_cosmos/CHANGELOG.md b/sdk/cosmos/azure_data_cosmos/CHANGELOG.md index 154e208cba..974c92daee 100644 --- a/sdk/cosmos/azure_data_cosmos/CHANGELOG.md +++ b/sdk/cosmos/azure_data_cosmos/CHANGELOG.md @@ -11,6 +11,7 @@ - Reading and querying items by RID now works end-to-end, including a parent-database cross-check that rejects a container RID belonging to a different database. ([#4687](https://github.com/Azure/azure-sdk-for-rust/pull/4687)) - Added an SDK-generated `x-ms-client-id` header that remains stable for each `CosmosClient`. ([#4844](https://github.com/Azure/azure-sdk-for-rust/pull/4844)) - Added opt-in Cosmos binary JSON encoding for item operations (`create`/`read`/`replace`/`upsert`). Enable it via `CosmosClientBuilder::with_binary_encoding_options` (or the `AZURE_COSMOS_BINARY_ENCODING_ENABLED` environment-variable fallback). Off by default; when disabled, requests and responses are byte-for-byte unchanged. ([#4671](https://github.com/Azure/azure-sdk-for-rust/pull/4671)) +- Extended binary JSON encoding to `query_items`: when binary encoding is enabled, queries negotiate a binary response (the request body stays text `application/query+json`) and decode binary feed pages, including the streaming cross-partition `ORDER BY` merge. Off by default and negotiated on the standard-gateway path. ([#5040](https://github.com/Azure/azure-sdk-for-rust/pull/5040)) - Added `FeedOptions::max_fan_out` (and `FeedOptions::with_max_fan_out`) to cap how many physical partitions a cross-partition query or change feed may fan out to. Applies to `ContainerClient::query_items` and `ContainerClient::query_change_feed`. The cap is enforced only at initial query setup; a partition that splits mid-execution and pushes the fan-out higher does not abort the operation. ([#4855](https://github.com/Azure/azure-sdk-for-rust/pull/4855)) - Added resumable cross-partition streaming `ORDER BY` query support. ([#4800](https://github.com/Azure/azure-sdk-for-rust/pull/4800)) - Added a pluggable client-side diagnostics emission layer — the `DiagnosticsHandler` trait and ordered `DiagnosticsHandlerChain` (registered via `CosmosClientBuilder::with_diagnostics_handler`) — invoked once per operation (singleton and paginated, on success and failure) with the completed `DiagnosticsContext` plus an SDK-supplied `CosmosOperationContext`; the empty default chain is a zero-overhead no-op. ([#4789](https://github.com/Azure/azure-sdk-for-rust/pull/4789)) diff --git a/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs b/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs index d7b3c6bbf1..e01716e269 100644 --- a/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs +++ b/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs @@ -418,11 +418,14 @@ async fn request_text_response_keeps_wire_binary_and_returns_data() { /// application/query+json`), the advertised negotiation header and whether the /// request body was Cosmos binary JSON (first byte `0x80`). Lets a test assert /// that a query advertises a binary *response* while keeping its request body -/// text. +/// text. Query-plan requests (metadata, which must **not** negotiate binary) are +/// recorded separately in `query_plan_formats` so a test can assert they carry +/// no binary header rather than silently skipping them. #[derive(Debug, Default)] struct QueryRequestRecorder { negotiation_formats: Mutex>>, body_is_binary: Mutex>, + query_plan_formats: Mutex>>, } impl RequestObserver for QueryRequestRecorder { @@ -436,9 +439,17 @@ impl RequestObserver for QueryRequestRecorder { if content_type.as_deref() != Some("application/query+json") { return; } + let formats = request + .headers() + .get_optional_str(&azure_core::http::headers::HeaderName::from_static( + "x-ms-cosmos-supported-serialization-formats", + )) + .map(|s| s.to_string()); // The query-plan request shares the `application/query+json` content type - // but is metadata (no data negotiation); skip it so only the data query - // is asserted on. + // but is metadata that must never negotiate binary. Record its advertised + // format separately so a test can assert the header is absent, rather than + // skipping it (which would let a regression that started negotiating on + // query plans pass unnoticed). if request .headers() .get_optional_str(&azure_core::http::headers::HeaderName::from_static( @@ -446,14 +457,9 @@ impl RequestObserver for QueryRequestRecorder { )) .is_some() { + self.query_plan_formats.lock().unwrap().push(formats); return; } - let formats = request - .headers() - .get_optional_str(&azure_core::http::headers::HeaderName::from_static( - "x-ms-cosmos-supported-serialization-formats", - )) - .map(|s| s.to_string()); self.negotiation_formats.lock().unwrap().push(formats); let is_binary = match request.body() { @@ -711,4 +717,15 @@ fn assert_query_advertised_binary(recorder: &QueryRequestRecorder) { "query request body must stay text (application/query+json is a spec, not a document)", ); } + // A query-plan request is metadata and must never negotiate binary — assert + // the header is absent rather than skipping these requests, so a regression + // that started negotiating on query plans is caught. + let query_plan_formats = recorder.query_plan_formats.lock().unwrap(); + for value in query_plan_formats.iter() { + assert_eq!( + value.as_deref(), + None, + "query-plan request must not advertise a binary response", + ); + } } diff --git a/sdk/cosmos/azure_data_cosmos_driver/CHANGELOG.md b/sdk/cosmos/azure_data_cosmos_driver/CHANGELOG.md index acf789f3e8..4574ce159e 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/CHANGELOG.md +++ b/sdk/cosmos/azure_data_cosmos_driver/CHANGELOG.md @@ -9,6 +9,7 @@ - Added `models::is_database_rid`, which reports whether a RID string decodes to a database-level RID (4 bytes). Lets callers that reuse a supplied RID as a database identity reject a wrong-hierarchy RID before it addresses the wrong resource. ([#4640](https://github.com/Azure/azure-sdk-for-rust/pull/4640)) - Added an SDK-generated `x-ms-client-id` header that remains stable for each `CosmosDriver`, including metadata, retry, hedge, probe, and Gateway 2.0 outer HTTP requests. ([#4844](https://github.com/Azure/azure-sdk-for-rust/pull/4844)) - Added a schema-agnostic Cosmos binary JSON codec (`binary_json`) and driver-side binary encoding via `OperationOptions.binary_encoding` (`BinaryEncodingOptions`). When enabled, the driver transcodes item request/response bodies between text and Cosmos binary JSON and negotiates the wire format; it is honored only for point `Document` item operations. Off by default and inert on the wire when unset. ([#4671](https://github.com/Azure/azure-sdk-for-rust/pull/4671)) +- Extended binary response negotiation to `Query`/`SqlQuery` on `Document`: a query advertises a binary response while keeping its text request body, and the streaming cross-partition `ORDER BY` merge emits binary pages so the SDK's integer-preserving binary deserializer decodes them (matching a passthrough binary query). Negotiation is honored on the standard-gateway path; the Gateway 2.0 / thin-client path does not yet carry the header. ([#5040](https://github.com/Azure/azure-sdk-for-rust/pull/5040)) - Added `PlanOptions` (with `DEFAULT_MAX_FAN_OUT`) to `CosmosDriver::plan_operation`, enforcing a maximum fan-out on fresh cross-partition plans. A fresh plan spanning more leaf request nodes than `PlanOptions::max_fan_out` (default 100) is rejected with the new `CosmosStatus::CLIENT_CROSS_PARTITION_FAN_OUT_EXCEEDED` (HTTP 400). The limit is enforced only at initial plan time: resuming from a continuation token skips the check, and a partition split that raises the fan-out mid-execution does not abort the operation. ([#4855](https://github.com/Azure/azure-sdk-for-rust/pull/4855)) - Added `CosmosOperation::db_operation_name`, returning the canonical OpenTelemetry `db.operation.name` (e.g. `read_item`, `query_items`, `execute_batch`, and `read_all_items_of_logical_partition` for a read feed scoped to one logical partition) for an operation. ([#4874](https://github.com/Azure/azure-sdk-for-rust/pull/4874)) - Added `RequestDiagnostics::operation_name`, naming the operation that issued an individual attempt. It is set only where one `DiagnosticsContext` aggregates attempts from more than one operation — today a PATCH, whose attempts report `patch_read_item` / `patch_replace_item` while the context reports `patch_item` — and is `None` otherwise, meaning the attempt shares the context's operation name. `CosmosOperation::is_patch_sub_operation` reports the same distinction on the operation itself. ([#4874](https://github.com/Azure/azure-sdk-for-rust/pull/4874)) diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs index 17862d9c38..96ddd1a7f9 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs @@ -83,7 +83,13 @@ const ACCOUNT_PROPERTIES_CONNECTIVITY_BASE_DELAY: Duration = Duration::from_mill /// reply in binary, preserving the read-side RU/COGS benefit the caller opted /// into. When the caller also asked for a text payload (`request_text_response`), /// the driver transcodes the guaranteed-binary response back to text after -/// receiving it, keeping the wire binary in both directions. +/// receiving it, keeping the wire binary in both directions. **This transcode is +/// honored only for point operations** (the `execute_operation` path); a query +/// drains through `plan_operation` → `execute_plan`, which does not transcode, so +/// `request_text_response` has no effect on a query. The SDK's typed query path +/// is unaffected (it decodes either format transparently via first-byte +/// detection); a raw/FFI query consumer that needs text must transcode the pages +/// itself. /// /// Note: .NET's *query* default is the broader `JsonText,CosmosBinary` ("send /// either, service chooses"); Rust deliberately forces `CosmosBinary` for query diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/integration_tests/order_by_resume.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/integration_tests/order_by_resume.rs index deeee579dc..f13e5428ba 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/integration_tests/order_by_resume.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/integration_tests/order_by_resume.rs @@ -559,6 +559,59 @@ async fn merges_two_binary_partitions_into_global_order() { ); } +/// Regression: a binary-sourced merge must emit **binary** on every output +/// page, including pages served entirely from buffered rows (no backend fetch). +/// With page size 1 and a single 3-row binary backend page, output pages 2 and +/// 3 consume no backend response, so a per-page `emit_binary` flag would leave +/// them text — carrying binary-canonicalized payloads that then fail typed +/// decode. The flag is sticky on the merge, so every page stays binary. +#[tokio::test] +async fn binary_merge_keeps_every_page_binary_including_buffer_only_pages() { + let op = order_by_operation_with_page_size(1); + let plan = order_by_plan(); + + let mut topology = MockTopologyProvider::new(vec![Ok(vec![resolved("", "FF", "pk-0")])]); + let mut executor = MockRequestExecutor::new(vec![Ok(binary_envelope_page( + &[("a", 1), ("b", 2), ("c", 3)], + None, + ))]); + + let mut pipeline = build_streaming_ordered_merge(&plan, &mut topology, &op, None) + .await + .unwrap(); + + let mut formats = Vec::new(); + let mut ids = Vec::new(); + let mut noop_topology = super::super::mocks::NoopTopologyProvider; + loop { + let mut context = PipelineContext::new(&mut executor, Some(&mut noop_topology)); + match pipeline.next_page(&mut context).await.unwrap() { + Some(response) => { + formats.push(crate::binary_json::is_binary(response.body_bytes())); + ids.extend(ids_in_page(&response)); + } + None => break, + } + } + + assert_eq!( + ids, + vec!["a".to_owned(), "b".to_owned(), "c".to_owned()], + "all rows must still be emitted in order", + ); + // The single backend page fills 3 rows; page size 1 emits 3 pages, only the + // first of which touches the backend. Every page must still be binary. + assert!( + formats.len() >= 3, + "expected at least 3 output pages, got {}", + formats.len() + ); + assert!( + formats.iter().all(|&is_binary| is_binary), + "every page of a binary-sourced merge must stay binary, got {formats:?}", + ); +} + /// A single partition, single page: the trivial case must still flow /// through the merge machinery correctly (no fan-out needed). #[tokio::test] diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs index 315c23597c..f3ea533722 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs @@ -410,6 +410,23 @@ impl PageAggregator { self.session_token = session_token; } + /// Seeds the sticky binary-emit flag from the owning merge, so a page served + /// entirely from buffered rows (no backend fetch, so no [`absorb`]) still + /// emits binary when the query's earlier pages were binary. See the + /// `emit_binary` field on the merge for why this must persist across pages. + /// + /// [`absorb`]: Self::absorb + pub(crate) fn seed_emit_binary(&mut self, emit_binary: bool) { + self.emit_binary = emit_binary; + } + + /// Whether this page will emit a Cosmos binary JSON envelope — `true` once + /// any absorbed backend page was binary or the flag was seeded from the + /// merge's sticky state. + pub(crate) fn emits_binary(&self) -> bool { + self.emit_binary + } + pub(crate) fn session_token(&self) -> Option<&SessionToken> { self.session_token.as_ref() } diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/streaming_ordered_merge.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/streaming_ordered_merge.rs index bc37d9774a..340445a097 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/streaming_ordered_merge.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/streaming_ordered_merge.rs @@ -379,6 +379,16 @@ pub(crate) struct StreamingOrderedMerge { /// past rows that were never emitted. Sticky: once set, snapshots resume /// from value boundaries instead of server continuations. continuation_unsafe: bool, + /// Set once any backend page for this query arrived as Cosmos binary JSON. + /// Sticky across pages: a single backend page routinely fills more rows than + /// one output page holds, so later pages are served entirely from buffered + /// rows and consume no backend response. Those rows were still decoded from + /// a binary page, so the emitted envelope must stay binary — otherwise the + /// SDK would text-decode a binary-canonicalized payload (e.g. a wide integer + /// widened to a float) and fail. Tracking this on the merge (not the + /// per-page aggregator) keeps every page's encoding tied to the data, not to + /// whether that page happened to touch the network. + emit_binary: bool, } impl StreamingOrderedMerge { @@ -395,6 +405,7 @@ impl StreamingOrderedMerge { session_token: None, deferred_error: None, continuation_unsafe: false, + emit_binary: false, query_fingerprint, } } @@ -602,6 +613,9 @@ impl PipelineNode for StreamingOrderedMerge { let mut aggregator = PageAggregator::new(); aggregator.seed_session_token(self.session_token.clone()); + // Inherit the sticky binary-emit flag so a page served entirely from + // buffered rows still emits binary when earlier pages were binary. + aggregator.seed_emit_binary(self.emit_binary); // Prime every child up front so the heap sees a head row for each // non-drained child. This page has emitted nothing, but a fill commits @@ -685,6 +699,9 @@ impl PipelineNode for StreamingOrderedMerge { let is_terminal = self.children.is_empty() && self.deferred_error.is_none(); self.session_token = aggregator.session_token().cloned(); + // Persist the binary-emit flag so subsequent buffer-only pages stay + // binary even though they consume no backend response. + self.emit_binary = aggregator.emits_binary(); let response = aggregator.build_page(&payloads)?; Ok(PageResult::Page { response, diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/response.rs b/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/response.rs index 34cd2f120d..391d63b45e 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/response.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/in_memory_emulator/response.rs @@ -263,6 +263,20 @@ pub(crate) fn success_response( /// Like [`success_response`], but encodes the body as Cosmos binary JSON when /// `binary` is set. Used by the item read/write handlers to honor a client that /// negotiated binary responses via `x-ms-cosmos-supported-serialization-formats`. +pub(crate) fn success_response_with_format( + status: StatusCode, + body: &serde_json::Value, + binary: bool, + charge: f64, + session_token: &str, + start: Instant, +) -> ResponseBuilder { + ResponseBuilder::new(status, start) + .with_request_charge(charge) + .with_session_token(session_token) + .with_value_body(body, binary) +} + /// The serialization format the emulator emits for a feed response body. /// /// Replaces a bare positional `bool` on the feed builders so each call site @@ -294,20 +308,6 @@ impl From for ResponseFormat { } } -pub(crate) fn success_response_with_format( - status: StatusCode, - body: &serde_json::Value, - binary: bool, - charge: f64, - session_token: &str, - start: Instant, -) -> ResponseBuilder { - ResponseBuilder::new(status, start) - .with_request_charge(charge) - .with_session_token(session_token) - .with_value_body(body, binary) -} - /// Creates an error response. pub(crate) fn error_response( status: StatusCode, diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/models/response_body.rs b/sdk/cosmos/azure_data_cosmos_driver/src/models/response_body.rs index b75648d9a9..fc18247cc5 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/models/response_body.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/models/response_body.rs @@ -151,13 +151,16 @@ impl ResponseBody { // JSON — it is not binary-aware. A single-preamble binary feed // envelope would therefore be sliced into sub-documents *without* // preambles, which `is_binary` would then route to the text path. - // This is inert today because query/feed binary negotiation is - // deferred (the service does not emit binary feeds without the - // negotiation header), so the binary `Items` branch is only - // exercised by hand-prefixed synthetic tests. When feed/query - // binary negotiation is added, the feed splitter must be made - // binary-aware (or each slice re-prefixed) before this path can - // decode real binary feeds. + // This branch is inert for the query path even though query binary + // negotiation is now enabled: a passthrough query decodes whole + // pages via `into_single`, and the ORDER BY merge parses pages with + // `parse_envelope_page`, which rejects a `Self::Items` body + // outright — so no query flow reaches this splitter with a binary + // body. The `Self::Items` branch has no production caller today and + // is exercised only by hand-prefixed synthetic tests. Should a + // future driver-side feed path route real binary feeds through this + // splitter, it must be made binary-aware (or each slice + // re-prefixed) first. .map(|b| deserialize_response(&b, "failed to deserialize feed item")) .collect(), } From d499a8ad9b0930adfa536ad70df6356034111394 Mon Sep 17 00:00:00 2001 From: kundadebdatta Date: Fri, 14 Aug 2026 11:38:23 -0700 Subject: [PATCH 11/13] Cosmos: silence clippy::too_many_arguments on fuzzer query helper 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. --- .../azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/sdk/cosmos/azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs b/sdk/cosmos/azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs index 1de1bef4f4..b5560c387b 100644 --- a/sdk/cosmos/azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs +++ b/sdk/cosmos/azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs @@ -2007,6 +2007,11 @@ where /// so callers gate it to the binary configs. /// /// Both filter on the unique `id`, so each returns exactly this item. +// The sent-value trio (`sent_canon`, `sent_hash`, `doc`) plus routing context is +// threaded verbatim into `assert_query_hit`/`assert_roundtrip`; bundling it into +// a struct here would only move the argument count to those helpers, so the +// eighth parameter is allowed on this test-only function. +#[allow(clippy::too_many_arguments)] async fn assert_query_roundtrip( container: &ContainerClient, pk: &str, From d85e89713a5d612274e3588a95660048979a931e Mon Sep 17 00:00:00 2001 From: kundadebdatta Date: Fri, 14 Aug 2026 12:57:33 -0700 Subject: [PATCH 12/13] Cosmos: address round-2 review, add skip/take + corpus query coverage 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. --- sdk/cosmos/AGENTS.md | 26 +++ .../tests/binary_roundtrip_fuzzer.rs | 44 +++--- .../binary_round_trip.rs | 148 +++++++++++++++--- .../docs/BINARY_ENCODING_HLD.md | 4 +- .../docs/BINARY_ENCODING_ROUNDTRIP_FUZZER.md | 16 +- .../docs/BINARY_ENCODING_SPEC.md | 6 +- .../src/driver/cosmos_driver.rs | 79 ++++++---- .../integration_tests/order_by_resume.rs | 8 +- .../src/driver/dataflow/query_response.rs | 22 ++- .../src/driver/dataflow/skip_take_page.rs | 85 ++++++++++ .../dataflow/streaming_ordered_merge.rs | 9 +- .../src/models/response_body.rs | 23 +-- .../binary_response_format.rs | 115 ++++++++++++++ .../tests/binary_sampled_testdata.rs | 59 ++++++- sdk/cosmos/ci.yml | 9 +- 15 files changed, 545 insertions(+), 108 deletions(-) diff --git a/sdk/cosmos/AGENTS.md b/sdk/cosmos/AGENTS.md index bb1f345578..68cda574cf 100644 --- a/sdk/cosmos/AGENTS.md +++ b/sdk/cosmos/AGENTS.md @@ -534,6 +534,32 @@ Every public API should document: - **Performance**: RU/s implications, if relevant - **Partition Key**: Whether the operation is partition-scoped +### Comment Brevity (IMPORTANT) + +Keep comments **short and dense**. Long, chatty comment blocks are noise: they age badly, bury the +signal, and read as filler. + +- **Inline comments**: 1–2 lines. 3 is the hard ceiling. Explain *why*, never *what* the code already says. +- **Doc comments**: lead with a one-sentence summary. Add detail only when a caller genuinely needs it + to use the API correctly. +- **Test comments**: usually unnecessary — a good test name plus a clear assertion message says it. + Comment only a non-obvious setup or a subtle invariant. +- **Never** restate an argument three ways, narrate the diff ("this used to be X, now it's Y"), + re-explain something already covered by a nearby doc comment, or write a mini design doc inline. + Link to the spec/HLD instead. +- When a rationale really needs paragraphs, it belongs in `docs/`, not in the source. + +```rust +// ❌ BAD: five lines to say one thing +// This is a client-side re-encode of a payload the driver itself produced, +// not a malformed service envelope, so it is a serialization fault (not the +// 500 `SERVICE_ORDER_BY_ENVELOPE_INVALID`). Include the item ordinal to +// correlate on a large page. + +// ✅ GOOD +// Client-side re-encode failure, not a malformed service envelope. +``` + ## Additional Resources - [Azure Cosmos DB REST API Reference](https://learn.microsoft.com/rest/api/cosmos-db/) diff --git a/sdk/cosmos/azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs b/sdk/cosmos/azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs index b5560c387b..94d958939b 100644 --- a/sdk/cosmos/azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs +++ b/sdk/cosmos/azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs @@ -1920,12 +1920,14 @@ async fn assert_typed_integer_probe( "{context}: typed integer probe round-trip changed" ); - // Also decode the same wide-integer probe through a cross-partition binary - // ORDER BY query. This drives the streaming-merge envelope decode - // (`build_page` re-encodes to binary so the SDK's `deserialize_integer` - // coercion runs), which the point-op read above does not exercise. A merge - // that emitted text instead would fail here with `invalid type: floating - // point, expected u64` for the `wide` field — the text/binary divergence. + // Also decode the same wide-integer probe through a full-container binary + // ORDER BY query, driving the streaming-merge envelope decode (`build_page` + // re-encodes to binary so `deserialize_integer` runs). A merge that emitted + // text would fail here with `invalid type: floating point, expected u64`. + // + // This container has default throughput (one physical partition), so the + // merge runs with a single child — multi-child interleave is covered by the + // 3-partition emulator tests. let order_by = with_transient_retry("int-probe-order-by", context, || async { let query = Query::from("SELECT * FROM c WHERE c.id = @id ORDER BY c.id") .with_parameter("@id", id.as_str())?; @@ -2000,11 +2002,13 @@ where /// Queries the just-written item back and asserts it round-trips, covering the /// query binary-response decode path (which point ops do not exercise). Always /// runs a single-partition query (the passthrough decode path). When -/// `include_cross_partition_order_by` is set, also runs a cross-partition -/// streaming `ORDER BY` query — the k-way merge, whose per-page envelope decode -/// is the binary path added for query support. That fan-out is the most -/// expensive query shape and adds no binary coverage on the text-control config, -/// so callers gate it to the binary configs. +/// `include_order_by` is set, also runs a full-container streaming `ORDER BY` +/// query, whose per-page envelope decode is the binary path added for query +/// support; it adds no binary coverage on text configs, so callers gate it. +/// +/// The live container has default throughput (one physical partition), so the +/// merge runs with a single child — multi-child interleave is covered by the +/// 3-partition emulator tests. /// /// Both filter on the unique `id`, so each returns exactly this item. // The sent-value trio (`sent_canon`, `sent_hash`, `doc`) plus routing context is @@ -2020,7 +2024,7 @@ async fn assert_query_roundtrip( sent_hash: &[u8; 32], doc: &Map, context: &str, - include_cross_partition_order_by: bool, + include_order_by: bool, ) -> Result> { let single_partition = with_transient_retry("query", context, || async { let query = Query::from("SELECT * FROM c WHERE c.id = @id").with_parameter("@id", id)?; @@ -2039,7 +2043,7 @@ async fn assert_query_roundtrip( "query", ); - if !include_cross_partition_order_by { + if !include_order_by { return Ok(1); } @@ -2290,12 +2294,12 @@ async fn binary_encoding_roundtrip_fuzz() -> Result<(), Box> { // Four point-op round-trips this config: create, read, replace, upsert. checked += 4; - // QUERY the item back. The single-partition passthrough query runs - // on every config; the expensive cross-partition ORDER BY fan-out — - // whose per-page envelope decode is the binary path added for query - // support — runs only on the binary configs, where it adds coverage - // (on text-control it would just cost time). - let include_order_by = *label != "text-control"; + // QUERY the item back. The single-partition passthrough query runs on + // every config; the expensive ORDER BY merge runs only on pure + // `binary` — `text-control` adds no binary coverage, and + // `binary+text-response` suppresses query negotiation entirely, so + // both would merge over text. + let include_order_by = *label == "binary"; let queries_checked = assert_query_roundtrip( &container, &pk, @@ -2327,7 +2331,7 @@ async fn binary_encoding_roundtrip_fuzz() -> Result<(), Box> { } println!( - "binary_roundtrip_fuzzer: DONE — {} documents × {} configs (4 point ops each + 1–2 queries; cross-partition ORDER BY on binary configs only) = {checked} round-trips, all canonical-equal (seed={})", + "binary_roundtrip_fuzzer: DONE — {} documents × {} configs (4 point ops each + 1–2 queries; full-container ORDER BY on binary configs only) = {checked} round-trips, all canonical-equal (seed={})", cfg.iterations, configs.len(), cfg.seed diff --git a/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs b/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs index e01716e269..b83a44ed9d 100644 --- a/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs +++ b/sdk/cosmos/azure_data_cosmos/tests/in_memory_emulator_tests/binary_round_trip.rs @@ -101,17 +101,15 @@ async fn build_container(db_name: &str, binary: bool) -> ContainerClient { .unwrap() } -/// Like [`build_container`], but provisions `partition_count` physical -/// partitions so a full-container query fans out across ranges (the passthrough -/// cross-partition path). Binary encoding is enabled. -/// Builds a multi-partition container with binary encoding enabled, attaching a -/// [`QueryRequestRecorder`] and returning it, so a cross-partition query test can -/// assert that every fan-out page actually advertised a binary response (a plain -/// results-match assertion would still pass if negotiation silently broke and the -/// emulator returned text, since text decodes fine). +/// Builds a container with `partition_count` physical partitions (so a +/// full-container query fans out across ranges); `binary` toggles Cosmos binary +/// JSON encoding. The returned [`QueryRequestRecorder`] lets a test assert what +/// each query actually advertised — a results-match assertion alone would still +/// pass if negotiation silently broke, since text decodes fine. async fn build_multi_partition_container_with_recorder( db_name: &str, partition_count: u32, + binary: bool, ) -> (ContainerClient, Arc) { let config = VirtualAccountConfig::new(vec![VirtualRegion::new( "East US", @@ -147,14 +145,17 @@ async fn build_multi_partition_container_with_recorder( EMULATOR_GATEWAY_URL.parse::().unwrap(), azure_core::credentials::Secret::new("dGVzdGtleQ=="), ); - let client = CosmosClientBuilder::new() - .with_binary_encoding_options(BinaryEncodingOptions::new().with_enabled(true)) - .with_runtime( - CosmosRuntimeBuilder::from(emulator.runtime_builder()) - .build() - .await - .unwrap(), - ) + let mut builder = CosmosClientBuilder::new().with_runtime( + CosmosRuntimeBuilder::from(emulator.runtime_builder()) + .build() + .await + .unwrap(), + ); + if binary { + builder = + builder.with_binary_encoding_options(BinaryEncodingOptions::new().with_enabled(true)); + } + let client = builder .build(account, RoutingStrategy::ProximityTo(Region::EAST_US)) .await .unwrap(); @@ -595,7 +596,7 @@ async fn binary_query_negotiates_response_and_round_trips() { #[tokio::test] async fn binary_cross_partition_query_round_trips() { let (container, recorder) = - build_multi_partition_container_with_recorder("bin-xpart-query", 3).await; + build_multi_partition_container_with_recorder("bin-xpart-query", 3, true).await; // Spread items across several partition keys so the fan-out spans ranges. let items: Vec = (0..12) @@ -643,7 +644,7 @@ async fn binary_cross_partition_query_round_trips() { #[tokio::test] async fn binary_cross_partition_order_by_merges_and_round_trips() { let (container, recorder) = - build_multi_partition_container_with_recorder("bin-xpart-order-by", 3).await; + build_multi_partition_container_with_recorder("bin-xpart-order-by", 3, true).await; // Interleave values across partition keys so the global order differs from // any single partition's local order — forcing the k-way merge to actually @@ -695,6 +696,64 @@ async fn binary_cross_partition_order_by_merges_and_round_trips() { assert_query_advertised_binary(&recorder); } +/// `OFFSET`/`LIMIT` and `TOP` route the fan-out through the `SkipTake` node, +/// which splits raw backend page envelopes itself. That splitter was text-only, +/// so binary-enabled skip/take queries hard-failed; this is the end-to-end guard. +#[tokio::test] +async fn binary_cross_partition_skip_take_round_trips() { + let (container, recorder) = + build_multi_partition_container_with_recorder("bin-xpart-skip-take", 3, true).await; + + let items: Vec = (0..10) + .map(|i| TestItem { + id: format!("s-{i:02}"), + pk: format!("pk{}", i % 3), + value: i, + note: format!("skip ☃ {i}"), + }) + .collect(); + for item in &items { + container + .create_item(&item.pk, &item.id, item, Some(write_options_with_content())) + .await + .unwrap(); + } + + let offset_limit = Box::pin(container.query_items::( + Query::from("SELECT * FROM c OFFSET 2 LIMIT 3"), + FeedScope::full_container(), + None, + )) + .await + .unwrap(); + let paged: Vec = Box::pin(offset_limit.try_collect()).await.unwrap(); + assert_eq!( + paged.len(), + 3, + "OFFSET 2 LIMIT 3 must yield exactly 3 items" + ); + + let topped = Box::pin(container.query_items::( + Query::from("SELECT TOP 4 * FROM c"), + FeedScope::full_container(), + None, + )) + .await + .unwrap(); + let top: Vec = Box::pin(topped.try_collect()).await.unwrap(); + assert_eq!(top.len(), 4, "TOP 4 must yield exactly 4 items"); + + // Every returned item must be one we wrote, decoded intact from binary. + for item in paged.iter().chain(top.iter()) { + assert!( + items.contains(item), + "skip/take returned an item that did not round-trip: {item:?}", + ); + } + + assert_query_advertised_binary(&recorder); +} + /// Asserts a query recorder saw at least one query request and that every query /// request advertised a binary response while keeping its body text. fn assert_query_advertised_binary(recorder: &QueryRequestRecorder) { @@ -711,6 +770,12 @@ fn assert_query_advertised_binary(recorder: &QueryRequestRecorder) { ); } let body_is_binary = recorder.body_is_binary.lock().unwrap(); + // Guards against `!is_binary` passing for a body the recorder never saw. + assert_eq!( + body_is_binary.len(), + formats.len(), + "every recorded query must have had its request body inspected", + ); for is_binary in body_is_binary.iter() { assert!( !is_binary, @@ -729,3 +794,50 @@ fn assert_query_advertised_binary(recorder: &QueryRequestRecorder) { ); } } + +/// With binary encoding **disabled**, a query must carry no +/// `x-ms-cosmos-supported-serialization-formats` header. The header is set at the +/// global `plan_operation` choke point, so a regression there would opt every +/// customer into binary. +#[tokio::test] +async fn disabled_binary_query_advertises_no_format() { + let (container, recorder) = + build_multi_partition_container_with_recorder("no-binary-xpart-query", 3, false).await; + + let items: Vec = (0..6) + .map(|i| TestItem { + id: format!("n-{i}"), + pk: format!("pk{}", i % 3), + value: i, + note: format!("plain {i}"), + }) + .collect(); + for item in &items { + container + .create_item(&item.pk, &item.id, item, Some(write_options_with_content())) + .await + .unwrap(); + } + + let iter = Box::pin(container.query_items::( + Query::from("SELECT * FROM c"), + FeedScope::full_container(), + None, + )) + .await + .unwrap(); + let _results: Vec = Box::pin(iter.try_collect()).await.unwrap(); + + let formats = recorder.negotiation_formats.lock().unwrap(); + assert!( + !formats.is_empty(), + "expected at least one query request to be recorded", + ); + for value in formats.iter() { + assert_eq!( + value.as_deref(), + None, + "a query on a binary-disabled client must advertise no serialization format", + ); + } +} diff --git a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_HLD.md b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_HLD.md index e08d37a6b4..8075d3c57f 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_HLD.md +++ b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_HLD.md @@ -397,8 +397,8 @@ opts.binary_encoding_request_text_response = 2; /* 2 = true */ ## Deferred work -* **Query response negotiation** — **done.** A `query_items` call now advertises a binary response via `x-ms-cosmos-supported-serialization-formats` (set once at the `plan_operation` choke point that every per-page request flows through). The query request body is a `application/query+json` spec, not a document, so it intentionally stays text — there is no query request-body encoding. The query *response* decodes via the shared choke point. **Standard-gateway path only:** the Gateway 2.0 / thin-client path re-encodes the request as an RNTBD metadata token list that has no `SupportedSerializationFormats` token, so the header is dropped there and the service returns text (which still decodes). Adding the RNTBD token is a **follow-up**. -* **Cross-partition / ORDER BY merge on binary bytes** — **scalar-key streaming ORDER BY now works on binary.** `parse_envelope_page` transcodes a binary-negotiated page to text before its text-only `serde_json` + `RawValue` envelope parse, so cross-partition ORDER BY over binary item bytes round-trips end to end. Single-partition and `into_single` query drains already round-trip binary directly. Remaining gap: **aggregate / GROUP BY / DISTINCT** merges over binary (and a binary-aware envelope parse that avoids the transcode copy — see the architecture note). +* **Query response negotiation** — **done.** A `query_items` call now advertises a binary response via `x-ms-cosmos-supported-serialization-formats` (set once at the `plan_operation` choke point that every per-page request flows through). The query request body is a `application/query+json` spec, not a document, so it intentionally stays text — there is no query request-body encoding. The query *response* decodes via the shared choke point. **Standard-gateway path only:** the Gateway 2.0 / thin-client path re-encodes the request as an RNTBD metadata token list that has no `SupportedSerializationFormats` token, so the header is dropped there and the service returns **text**. This is a customer-visible limitation, not benign: text pages reintroduce the integral-`Double`→integer divergence (#5028) this feature exists to fix, so a wide integer that round-trips over binary on the standard gateway can still fail typed deserialization on a thin-client account. Adding the RNTBD `SupportedSerializationFormats` token is a **follow-up**. +* **Cross-partition / ORDER BY merge on binary bytes** — **scalar-key streaming ORDER BY now works on binary.** `parse_envelope_page` transcodes a binary-negotiated page to text before its text-only `serde_json` + `RawValue` envelope parse, so cross-partition ORDER BY over binary item bytes round-trips end to end. Single-partition and `into_single` query drains already round-trip binary directly. Remaining gap: **aggregate / GROUP BY / DISTINCT** merges over binary, plus a perf follow-up: a binary page is currently transcoded roughly three times per document (whole-page binary→text, `serde_json` envelope parse, then a per-item text→binary re-encode in `build_page`). A binary-aware envelope reader that yields each payload's binary sub-slice — re-prefixed with the `0x80` preamble — would reduce this to a slice + prefix and drop both the parse copy and the per-item re-encode. Note also that because `emit_binary` is sticky on the merge, once any page is binary the items sourced from **text** pages are re-encoded too, which normalizes key order and collapses duplicate keys. * **Binary feed responses** — the `into_items` feed splitter scans **text** JSON, so binary `Documents` envelopes cannot be sliced by that splitter yet. (The SDK query path uses `into_single`, which is unaffected.) `ReadFeed` / change feed is excluded from binary negotiation: the backend does not honor the header for it. * **`patch`** — excluded from binary encoding for now (the driver's request-side encode intentionally skips patch); transactional `batch` / `bulk` are deferred by spec. * **Cross-implementation vectors** — validate against captured real .NET / Java binary output. diff --git a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_ROUNDTRIP_FUZZER.md b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_ROUNDTRIP_FUZZER.md index 8f9cdfc698..81e3f8e146 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_ROUNDTRIP_FUZZER.md +++ b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_ROUNDTRIP_FUZZER.md @@ -32,10 +32,24 @@ For each generated document `D`: - drive every **body-carrying point op** — `create` → `read` → `replace` → `upsert` — each returning a document `R` (writes use content-response so the response decode path is exercised too); + - **query the item back** — a single-partition `SELECT * FROM c WHERE c.id` + on every config, plus (on the pure-`binary` config only) a full-container + `ORDER BY` query that exercises the streaming merge's binary decode path; - `Hc = hash(canonicalize(strip_system(R)))` for each op's `R`; - **assert `Hc == H0`** for every op — otherwise dump the seed + both canonical forms. +The pure-`binary` config additionally runs a typed wide-integer probe (a +`u64 = u64::MAX` document read back both as a point op and through a binary +`ORDER BY` query) to exercise the native `deserialize_integer` coercion that the +`Value`-based comparisons do not reach. + +> **Merge fan-out caveat.** The fuzzer's container is created with default +> throughput, i.e. a **single physical partition**, so the `ORDER BY` query drives +> the streaming merge with one child. It covers the per-page envelope decode and +> the per-item binary re-encode, but not a true multi-child interleave — that is +> covered by the 3-partition in-memory emulator tests. + These are exactly the four point operations for which binary encoding is honored (`create` / `read` / `replace` / `upsert`); `delete` carries no body, and `patch` / transactional batch / bulk are deferred (see the SPEC/HLD), so they are @@ -123,7 +137,7 @@ flowchart TD NORM0 --> CANON0["json-canon (RFC 8785)\ncanonical string"] CANON0 --> HASH0["SHA-256 -> H0\n(expected)"] - BOUND --> STORE["for each config:\ncreate -> read -> replace -> upsert\n(each returns R)"] + BOUND --> STORE["for each config:\ncreate -> read -> replace -> upsert\n+ query back (ORDER BY on binary)\n(each returns R)"] STORE --> PROJ["project(R, keys(D))\nstrip _rid/_etag/_ts/..."] PROJ --> NORM1["normalize_numbers"] NORM1 --> CANON1["json-canon"] diff --git a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_SPEC.md b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_SPEC.md index ca7df97f61..bfce1f21bc 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_SPEC.md +++ b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_SPEC.md @@ -84,7 +84,11 @@ text — there is no query request-body encoding to do. > request is re-encoded as an RNTBD metadata token list, and there is no > `SupportedSerializationFormats` token, so the header cannot survive the > thin-client wrapping — a query against a Gateway 2.0 account silently returns -> text (which still decodes correctly). Adding the RNTBD token is tracked as a +> **text**. This is a **customer-visible limitation, not benign**: the whole +> point of query binary negotiation is to fix the integral-`Double`→integer +> divergence (#5028), so a wide integer that round-trips over binary on the +> standard gateway can still fail typed deserialization over a thin-client +> account. Adding the RNTBD `SupportedSerializationFormats` token is tracked as a > follow-up. ## 3. Background: the .NET reference diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs index a74d9249b1..54db3df0d4 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/cosmos_driver.rs @@ -83,13 +83,10 @@ const ACCOUNT_PROPERTIES_CONNECTIVITY_BASE_DELAY: Duration = Duration::from_mill /// reply in binary, preserving the read-side RU/COGS benefit the caller opted /// into. When the caller also asked for a text payload (`request_text_response`), /// the driver transcodes the guaranteed-binary response back to text after -/// receiving it, keeping the wire binary in both directions. **This transcode is -/// honored only for point operations** (the `execute_operation` path); a query -/// drains through `plan_operation` → `execute_plan`, which does not transcode, so -/// `request_text_response` has no effect on a query. The SDK's typed query path -/// is unaffected (it decodes either format transparently via first-byte -/// detection); a raw/FFI query consumer that needs text must transcode the pages -/// itself. +/// receiving it, keeping the wire binary in both directions. **Point ops only** — +/// queries bypass that transcode, so a query with `request_text_response` set does +/// not negotiate binary at all (see +/// [`apply_response_negotiation`](CosmosDriver::apply_response_negotiation)). /// /// Note: .NET's *query* default is the broader `JsonText,CosmosBinary` ("send /// either, service chooses"); Rust deliberately forces `CosmosBinary` for query @@ -2709,6 +2706,11 @@ impl CosmosDriver { /// only, so it is safe for query (whose text `application/query+json` body /// must never be transcoded). /// + /// Two guards: a caller-set header is never overwritten, and a **query** with + /// `request_text_response` does not negotiate binary (queries bypass the + /// transcode-back-to-text in `execute_operation`, so binary pages would defeat + /// an explicit text request). Point ops are unaffected. + /// /// [`binary_negotiates_response`]: CosmosDriver::binary_negotiates_response fn apply_response_negotiation( &self, @@ -2720,6 +2722,14 @@ impl CosmosDriver { { return operation; } + // Never clobber a header a caller already set. + if operation + .request_headers() + .supported_serialization_formats + .is_some() + { + return operation; + } // Reuse a caller-resolved value when available (the `execute_operation` // path already resolved it for the request-body gate); otherwise resolve // it here — the query path reaches `plan_operation` directly without a @@ -2730,11 +2740,18 @@ impl CosmosDriver { .cloned() .unwrap_or_default() }); - if binary.enabled { - operation.with_supported_serialization_formats(BINARY_NEGOTIATION_FORMATS) - } else { - operation + if !binary.enabled { + return operation; } + // Queries bypass the response transcode, so honor an explicit text request. + let is_query = matches!( + operation.operation_type(), + crate::models::OperationType::Query | crate::models::OperationType::SqlQuery + ); + if is_query && binary.request_text_response { + return operation; + } + operation.with_supported_serialization_formats(BINARY_NEGOTIATION_FORMATS) } /// Transcodes an operation's **text** request body to Cosmos binary JSON. @@ -3307,6 +3324,24 @@ impl CosmosDriver { options: &OperationOptions, continuation: Option<&ContinuationToken>, plan_options: &PlanOptions, + ) -> crate::error::Result { + // `None` resolves the binary options lazily at the negotiation choke + // point. Boxed to keep this wrapper's future pointer-sized. + Box::pin(self.plan_operation_resolved(operation, options, continuation, plan_options, None)) + .await + } + + /// [`plan_operation`](Self::plan_operation) with an optional caller-resolved + /// [`BinaryEncodingOptions`](crate::options::BinaryEncodingOptions), so + /// `execute_operation` need not re-resolve what it already resolved for the + /// request-body gate. `None` resolves lazily at the negotiation choke point. + async fn plan_operation_resolved( + &self, + operation: CosmosOperation, + options: &OperationOptions, + continuation: Option<&ContinuationToken>, + plan_options: &PlanOptions, + resolved_binary: Option, ) -> crate::error::Result { // Reject mixed name/RID addressing before any IO work is done. The // service classifies a request as name-based or RID-based from its `dbs` @@ -3330,28 +3365,6 @@ impl CosmosDriver { // here so every caller awaits a pointer-sized future instead of having // to pin at its own call site and rediscover this each time the state // grows. - Box::pin(async move { - self.plan_operation_inner(operation, options, continuation, plan_options, None) - .await - }) - .await - } - - /// [`plan_operation`](Self::plan_operation) with a caller-resolved - /// [`BinaryEncodingOptions`](crate::options::BinaryEncodingOptions), letting - /// `execute_operation` avoid re-resolving the layered options view that it - /// already resolved for the request-body gate. The public `plan_operation` - /// entry point passes `None`, so the query path (which reaches the driver - /// here directly) resolves lazily at the negotiation choke point. - async fn plan_operation_resolved( - &self, - operation: CosmosOperation, - options: &OperationOptions, - continuation: Option<&ContinuationToken>, - plan_options: &PlanOptions, - resolved_binary: Option, - ) -> crate::error::Result { - operation.validate_addressing()?; Box::pin(async move { self.plan_operation_inner( operation, diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/integration_tests/order_by_resume.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/integration_tests/order_by_resume.rs index d86b2c7e52..c9055c1f23 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/integration_tests/order_by_resume.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/integration_tests/order_by_resume.rs @@ -603,8 +603,12 @@ async fn binary_merge_keeps_every_page_binary_including_buffer_only_pages() { let mut context = PipelineContext::new(&mut executor, Some(&mut noop_topology)); match pipeline.next_page(&mut context).await.unwrap() { Some(response) => { - formats.push(page_is_binary(&response)); - ids.extend(ids_in_page(&response)); + let page_ids = ids_in_page(&response); + // `page_is_binary` is vacuously true for an empty page. + if !page_ids.is_empty() { + formats.push(page_is_binary(&response)); + } + ids.extend(page_ids); } None => break, } diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs index 1a21ce8374..43b5757890 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/query_response.rs @@ -504,6 +504,11 @@ impl PageAggregator { /// integral-`Double`→integer coercion), exactly as a passthrough binary /// query; text sources are emitted verbatim. /// + /// Because `emit_binary` is sticky, once any page is binary a *text*-sourced + /// payload is re-encoded too, normalizing key order and collapsing duplicate + /// keys. A mixed merge shouldn't occur (a query negotiates one format for its + /// whole lifetime), so this is defensive. + /// /// It's valid for no backend page to have been absorbed (page /// assembled entirely from previously-buffered rows); it then reports /// zero charge and a fresh, empty [`DiagnosticsContext`]. @@ -520,15 +525,24 @@ impl PageAggregator { // bytes verbatim. let items: Vec = payloads .iter() - .map(|payload| { + .enumerate() + .map(|(index, payload)| { let text = payload.get().as_bytes(); if self.emit_binary { crate::binary_json::transcode_to_binary(text) .map(bytes::Bytes::from) .map_err(|e| { - envelope_error(format!( - "failed to transcode merged ORDER BY item to binary: {e}" - )) + // Client-side re-encode failure, not a malformed + // service envelope — a serialization fault, not 500. + crate::error::CosmosError::builder() + .with_status( + crate::error::CosmosStatus::SERIALIZATION_RESPONSE_BODY_INVALID, + ) + .with_message(format!( + "failed to re-encode merged ORDER BY item {index} to binary: {e}" + )) + .with_source(e) + .build() }) } else { Ok(bytes::Bytes::copy_from_slice(text)) diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/skip_take_page.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/skip_take_page.rs index 84cb41edd4..bb188ba1a2 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/skip_take_page.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/skip_take_page.rs @@ -46,10 +46,53 @@ struct RawQueryPage<'a> { /// each a zero-copy [`slice_ref`](bytes::Bytes::slice_ref) of `body`. /// /// An empty (`NoPayload`) body is treated as a zero-document page. +/// +/// A **Cosmos binary JSON** page is transcoded to text to split the `Documents` +/// array, then each document is re-encoded to standalone binary. Per-document +/// binary (rather than the split text slices) keeps the SDK's per-slice `0x80` +/// auto-detection, so an integral `Double` still coerces into an integer target. pub(crate) fn split_feed_envelope(body: &Bytes) -> crate::error::Result> { if body.is_empty() { return Ok(Vec::new()); } + + // No `slice_ref` here: the parsed bytes live in the transcoded buffer. + if crate::binary_json::is_binary(body) { + let text = crate::binary_json::transcode_to_text(body).map_err(|e| { + crate::error::CosmosError::builder() + .with_status(crate::error::CosmosStatus::SERIALIZATION_RESPONSE_BODY_INVALID) + .with_message("failed to transcode binary cross-partition query page to text") + .with_source(e) + .build() + })?; + let page: RawQueryPage = serde_json::from_slice(&text).map_err(|e| { + crate::error::CosmosError::builder() + .with_status(crate::error::CosmosStatus::SERIALIZATION_RESPONSE_BODY_INVALID) + .with_message("failed to parse cross-partition query page envelope") + .with_source(e) + .build() + })?; + return page + .documents + .iter() + .map(|raw| { + crate::binary_json::transcode_to_binary(raw.get().as_bytes()) + .map(Bytes::from) + .map_err(|e| { + crate::error::CosmosError::builder() + .with_status( + crate::error::CosmosStatus::SERIALIZATION_RESPONSE_BODY_INVALID, + ) + .with_message( + "failed to re-encode cross-partition query document to binary", + ) + .with_source(e) + .build() + }) + }) + .collect(); + } + let page: RawQueryPage = serde_json::from_slice(body).map_err(|e| { crate::error::CosmosError::builder() .with_status(crate::error::CosmosStatus::SERIALIZATION_RESPONSE_BODY_INVALID) @@ -195,4 +238,46 @@ mod tests { let out = split_feed_envelope(&envelope(b"not json")); assert!(out.is_err()); } + + #[test] + fn binary_page_splits_into_binary_items() { + let text = br#"{"Documents":[{"id":1},{"id":2},{"id":3}],"_count":3}"#; + let binary = crate::binary_json::transcode_to_binary(text).unwrap(); + assert!(crate::binary_json::is_binary(&binary)); + + let items = split_feed_envelope(&Bytes::from(binary)).unwrap(); + let out = skip_take_items(items, 1, Some(1)); + assert_eq!(out.dropped, 1); + assert_eq!(out.emitted, 1); + assert!(crate::binary_json::is_binary(&out.items[0])); + let doc: serde_json::Value = crate::binary_json::from_slice(&out.items[0]).unwrap(); + assert_eq!(doc, serde_json::json!({ "id": 2 })); + } + + #[test] + fn binary_page_preserves_wide_integer_for_typed_decode() { + // #5028: a text split surfaces the integral `Double` as a float that a + // `u64` target rejects; the binary path re-encodes so it coerces back. + #[derive(serde::Deserialize, PartialEq, Debug)] + struct Doc { + wide: u64, + } + let wide = serde_json::Number::from_f64(9_007_199_254_740_992.0).unwrap(); + let text = serde_json::to_vec(&serde_json::json!({ + "Documents": [{ "wide": wide }], + "_count": 1, + })) + .unwrap(); + let binary = crate::binary_json::transcode_to_binary(&text).unwrap(); + + let items = split_feed_envelope(&Bytes::from(binary)).unwrap(); + assert_eq!(items.len(), 1); + let doc: Doc = crate::binary_json::from_slice(&items[0]).unwrap(); + assert_eq!( + doc, + Doc { + wide: 9_007_199_254_740_992 + } + ); + } } diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/streaming_ordered_merge.rs b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/streaming_ordered_merge.rs index 17ce96bdc3..08b0fe4721 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/streaming_ordered_merge.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/driver/dataflow/streaming_ordered_merge.rs @@ -626,6 +626,9 @@ impl PipelineNode for StreamingOrderedMerge { .ensure_all_streams_filled(context, &mut aggregator) .await { + // A child may have buffered binary-sourced rows before another + // errored; those rows outlive this aggregator, so promote now. + self.emit_binary |= aggregator.emits_binary(); self.continuation_unsafe = true; return Err(err); } @@ -699,9 +702,9 @@ impl PipelineNode for StreamingOrderedMerge { let is_terminal = self.children.is_empty() && self.deferred_error.is_none(); self.session_token = aggregator.session_token().cloned(); - // Persist the binary-emit flag so subsequent buffer-only pages stay - // binary even though they consume no backend response. - self.emit_binary = aggregator.emits_binary(); + // Sticky so buffer-only pages stay binary; `|=` so a page with no fresh + // binary input cannot clear the flag. + self.emit_binary |= aggregator.emits_binary(); let response = aggregator.build_page(&payloads)?; Ok(PageResult::Page { response, diff --git a/sdk/cosmos/azure_data_cosmos_driver/src/models/response_body.rs b/sdk/cosmos/azure_data_cosmos_driver/src/models/response_body.rs index fc18247cc5..e73400f76d 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/src/models/response_body.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/src/models/response_body.rs @@ -145,22 +145,13 @@ impl ResponseBody { } Self::Items(items) => items .into_iter() - // NOTE: `deserialize_response` auto-detects binary per slice via - // the `0x80` preamble, but the feed pipeline that produces - // `Self::Items` splits the `Documents` array by scanning **text** - // JSON — it is not binary-aware. A single-preamble binary feed - // envelope would therefore be sliced into sub-documents *without* - // preambles, which `is_binary` would then route to the text path. - // This branch is inert for the query path even though query binary - // negotiation is now enabled: a passthrough query decodes whole - // pages via `into_single`, and the ORDER BY merge parses pages with - // `parse_envelope_page`, which rejects a `Self::Items` body - // outright — so no query flow reaches this splitter with a binary - // body. The `Self::Items` branch has no production caller today and - // is exercised only by hand-prefixed synthetic tests. Should a - // future driver-side feed path route real binary feeds through this - // splitter, it must be made binary-aware (or each slice - // re-prefixed) first. + // Items are decoded independently, auto-detecting binary per + // slice via the `0x80` preamble. Safe because every producer + // (`skip_take_page::split_feed_envelope`, `build_page`) emits + // each document already standalone-encoded. A future splitter + // that slices a single-preamble envelope by scanning text would + // yield preamble-less sub-documents misrouted to the text path, + // so it must re-prefix per document first. .map(|b| deserialize_response(&b, "failed to deserialize feed item")) .collect(), } diff --git a/sdk/cosmos/azure_data_cosmos_driver/tests/in_memory_emulator_tests/binary_response_format.rs b/sdk/cosmos/azure_data_cosmos_driver/tests/in_memory_emulator_tests/binary_response_format.rs index 2fb78ad8d5..6f8d0ddd13 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/tests/in_memory_emulator_tests/binary_response_format.rs +++ b/sdk/cosmos/azure_data_cosmos_driver/tests/in_memory_emulator_tests/binary_response_format.rs @@ -218,3 +218,118 @@ async fn text_read_of_binary_written_item_yields_text_response() { assert_eq!(decoded["id"], "mixed-1"); assert_eq!(decoded["value"], 314); } + +/// Builds a `POST .../docs` query request (`application/query+json`), optionally +/// advertising a response-format via `x-ms-cosmos-supported-serialization-formats`. +fn query_items_request( + gateway_url: &str, + db: &str, + coll: &str, + query: &str, + serialization_formats: Option<&str>, +) -> Request { + let url = format!("{}/dbs/{}/colls/{}/docs", gateway_url, db, coll); + let mut req = Request::new(Url::parse(&url).unwrap(), Method::Post); + let body = serde_json::json!({ "query": query, "parameters": [] }); + req.set_body(serde_json::to_vec(&body).unwrap()); + req.headers_mut().insert( + HeaderName::from_static("x-ms-documentdb-isquery"), + HeaderValue::from_static("True"), + ); + req.headers_mut().insert( + HeaderName::from_static("content-type"), + HeaderValue::from_static("application/query+json"), + ); + req.headers_mut().insert( + HeaderName::from_static("x-ms-documentdb-query-enablecrosspartition"), + HeaderValue::from_static("True"), + ); + if let Some(formats) = serialization_formats { + req.headers_mut().insert( + SUPPORTED_SERIALIZATION_FORMATS.clone(), + HeaderValue::from(formats.to_string()), + ); + } + req +} + +/// A query advertising `CosmosBinary` gets a binary feed body, asserted on the +/// raw wire bytes — the response half of query negotiation. +#[tokio::test] +async fn binary_query_yields_binary_response() { + let ctx = setup_single_region().await; + + // Seed one item so the query returns a non-empty feed. + let seed = create_item_request( + &ctx.gateway_url, + "testdb", + "testcoll", + &serde_json::json!({ "id": "q-1", "pk": "pk1", "value": 11 }), + r#"["pk1"]"#, + false, + ); + let seed_resp = ctx.emulator.execute_request(&seed).await.unwrap(); + let (seed_status, _h, _b) = collect_raw_response(seed_resp).await; + assert_eq!(seed_status, StatusCode::Created); + + let req = query_items_request( + &ctx.gateway_url, + "testdb", + "testcoll", + "SELECT * FROM c", + Some("CosmosBinary"), + ); + let response = ctx.emulator.execute_request(&req).await.unwrap(); + let (status, _headers, raw) = collect_raw_response(response).await; + + assert_eq!(status, StatusCode::Ok); + assert_eq!( + raw.first(), + Some(&PREAMBLE), + "a query advertising CosmosBinary must return a binary (0x80) feed body", + ); + assert!( + binary_json::is_binary(&raw), + "query response body must be detected as binary", + ); + // The binary feed envelope decodes back to the seeded document. + let decoded: serde_json::Value = binary_json::decode(&raw).unwrap(); + assert_eq!(decoded["Documents"][0]["id"], "q-1"); +} + +/// Counter-case: a `JsonText`-only query gets a text feed body. +#[tokio::test] +async fn jsontext_query_yields_text_response() { + let ctx = setup_single_region().await; + + let seed = create_item_request( + &ctx.gateway_url, + "testdb", + "testcoll", + &serde_json::json!({ "id": "q-2", "pk": "pk1", "value": 22 }), + r#"["pk1"]"#, + false, + ); + let seed_resp = ctx.emulator.execute_request(&seed).await.unwrap(); + let (seed_status, _h, _b) = collect_raw_response(seed_resp).await; + assert_eq!(seed_status, StatusCode::Created); + + let req = query_items_request( + &ctx.gateway_url, + "testdb", + "testcoll", + "SELECT * FROM c", + Some("JsonText"), + ); + let response = ctx.emulator.execute_request(&req).await.unwrap(); + let (status, _headers, raw) = collect_raw_response(response).await; + + assert_eq!(status, StatusCode::Ok); + assert!( + !binary_json::is_binary(&raw), + "a JsonText query must return a text feed body", + ); + let text = std::str::from_utf8(&raw).expect("text response must be valid UTF-8"); + let decoded: serde_json::Value = serde_json::from_str(text).unwrap(); + assert_eq!(decoded["Documents"][0]["id"], "q-2"); +} diff --git a/sdk/cosmos/azure_data_cosmos_perf/tests/binary_sampled_testdata.rs b/sdk/cosmos/azure_data_cosmos_perf/tests/binary_sampled_testdata.rs index 2ce032819f..8349daacd6 100644 --- a/sdk/cosmos/azure_data_cosmos_perf/tests/binary_sampled_testdata.rs +++ b/sdk/cosmos/azure_data_cosmos_perf/tests/binary_sampled_testdata.rs @@ -7,9 +7,11 @@ //! //! The perf crate ships a large collection of representative JSON payloads in //! `testdata/`. This test picks random documents out of that corpus, injects an -//! `id` and a `pk` (the container is partitioned on `/pk`), then creates each -//! document and reads it back — with the SDK's binary-encoding preview enabled — -//! asserting the fields we wrote survive the binary request/response round-trip. +//! `id` and a `pk` (the container is partitioned on `/pk`), then round-trips each +//! document — with the SDK's binary-encoding preview enabled — through create, +//! read, a passthrough query, and an `ORDER BY` query (which routes the page +//! through the streaming k-way merge), asserting the fields we wrote survive +//! every path. //! //! # Test data dependency //! @@ -54,15 +56,18 @@ use std::error::Error; use std::path::{Path, PathBuf}; use azure_core::http::StatusCode; +use azure_data_cosmos::clients::ContainerClient; use azure_data_cosmos::models::ContainerProperties; use azure_data_cosmos::options::{ BinaryEncodingOptions, ConnectionPoolOptions, ContentResponseOnWrite, ItemWriteOptions, OperationOptions, Region, ServerCertificateValidation, }; use azure_data_cosmos::{ - AccountEndpoint, AccountReference, CosmosClient, CosmosRuntime, RoutingStrategy, + AccountEndpoint, AccountReference, CosmosClient, CosmosRuntime, FeedScope, Query, + RoutingStrategy, }; use azure_data_cosmos_driver::models::ConnectionString; +use futures::TryStreamExt; use rand::RngExt; use serde_json::{Map, Value}; use uuid::Uuid; @@ -244,8 +249,28 @@ fn assert_sent_fields_round_tripped(sent: &Map, got: &Value, cont } } +/// Runs `query` across the whole container and returns its single expected hit. +async fn query_one( + container: &ContainerClient, + query: Query, + context: &str, +) -> Result> { + let iter = container + .query_items::(query, FeedScope::full_container(), None) + .await?; + let hits: Vec = iter.try_collect().await?; + assert_eq!( + hits.len(), + 1, + "{context}: expected exactly one hit, got {}", + hits.len(), + ); + Ok(hits.into_iter().next().expect("asserted non-empty")) +} + /// Samples documents from the bundled corpus and round-trips each one through -/// create + read using binary encoding, asserting the sent fields survive. +/// create, read, and both query shapes using binary encoding, asserting the sent +/// fields survive. #[tokio::test] #[cfg_attr( not(test_category = "binary_encoding"), @@ -311,6 +336,30 @@ async fn binary_round_trips_sampled_testdata() -> Result<(), Box> { assert_eq!(read.status(), StatusCode::Ok, "{context}: read"); let read_doc: Value = read.into_model()?; assert_sent_fields_round_tripped(&doc, &read_doc, &format!("{context}: read")); + + // QUERY the document back. Queries negotiate a binary *response* but + // send a text `application/query+json` body, so this covers a decode + // path the point ops above do not reach. + let queried = query_one( + &container, + Query::from("SELECT * FROM c WHERE c.id = @id").with_parameter("@id", id.as_str())?, + &format!("{context}: query"), + ) + .await?; + assert_sent_fields_round_tripped(&doc, &queried, &format!("{context}: query")); + + // Same document through an ORDER BY query, which routes the page through + // the streaming k-way merge instead of the passthrough drain. The merge + // re-encodes each item, so a corpus document that survives here has + // survived a full binary decode/re-encode cycle. + let ordered = query_one( + &container, + Query::from("SELECT * FROM c WHERE c.id = @id ORDER BY c.id") + .with_parameter("@id", id.as_str())?, + &format!("{context}: order-by query"), + ) + .await?; + assert_sent_fields_round_tripped(&doc, &ordered, &format!("{context}: order-by query")); } println!("Round-tripped {SAMPLE_COUNT} sampled documents through binary encoding."); diff --git a/sdk/cosmos/ci.yml b/sdk/cosmos/ci.yml index 81beac5ccb..97bb8b3a69 100644 --- a/sdk/cosmos/ci.yml +++ b/sdk/cosmos/ci.yml @@ -52,9 +52,12 @@ extends: # Live budget for the binary-encoding round-trip fuzzer # (azure_data_cosmos/tests/binary_roundtrip_fuzzer.rs). Only consumed on # the `binary_encoding` live leg (live-platform-matrix.json); a no-op - # elsewhere. Each iteration exercises 3 encoding configs x 4 point ops, so - # 200 => ~2400 round-trips — bounded to fit the live-test time cap. Bump it - # for a deeper soak. + # elsewhere. Each iteration exercises 3 encoding configs; every config runs + # 4 point ops + 1 single-partition query, and the pure-`binary` config adds + # a cross-partition ORDER BY query plus a typed wide-integer probe (itself + # an extra ORDER BY). That is 3*(4+1) + 2 = ~17 round-trips per iteration, + # so 200 => ~3400 — bounded to fit the live-test time cap. Bump it for a + # deeper soak. AZURE_COSMOS_FUZZ_ITERATIONS: '200' # Enable the wide-number generator (integers at or beyond 2^53, where the # service's number model stores the value as an IEEE-754 double). Without From bf2c628921ed3d3ea412c1f0d8597db345e314f3 Mon Sep 17 00:00:00 2001 From: kundadebdatta Date: Fri, 14 Aug 2026 13:51:12 -0700 Subject: [PATCH 13/13] Cosmos: restructure binary-encoding HLD status and add .NET parity 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". --- .../docs/BINARY_ENCODING_HLD.md | 128 ++++++++++++++++-- 1 file changed, 120 insertions(+), 8 deletions(-) diff --git a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_HLD.md b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_HLD.md index 8075d3c57f..0291629cc7 100644 --- a/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_HLD.md +++ b/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_HLD.md @@ -2,7 +2,7 @@ This document is the high-level design for Cosmos **binary JSON** encoding in the Rust SDK and driver. It captures the goals, the wire/transcoding model, the -component layout, testing, and the intentionally deferred work. +component layout, testing, and the current support status. For the phased implementation plan and low-level wire details, see [`BINARY_ENCODING_SPEC.md`](https://github.com/Azure/azure-sdk-for-rust/blob/main/sdk/cosmos/azure_data_cosmos_driver/docs/BINARY_ENCODING_SPEC.md). @@ -22,7 +22,7 @@ Because the option lives on the driver and is schema-agnostic, the driver perfor A self-contained, in-tree **end-to-end validation loop** is included via the in-memory emulator (no Docker, no live account, no external test vectors). -> **Scope:** item operations (`create` / `replace` / `upsert` / `read`) encode request bodies and decode responses; `query` negotiates a binary response (its `application/query+json` request body stays text). Patch, transactional batch, and bulk are intentionally deferred (see [Deferred work](#deferred-work)). +> **Scope:** item operations (`create` / `replace` / `upsert` / `read`) encode request bodies and decode responses; `query` negotiates a binary response (its `application/query+json` request body stays text). Patch, transactional batch, and bulk are intentionally excluded (see [Binary encoding support status](#binary-encoding-support-status)). --- @@ -395,13 +395,125 @@ opts.binary_encoding_request_text_response = 2; /* 2 = true */ --- -## Deferred work +## Binary encoding support status + +Legend: **Done** shipped · **Pending** actionable follow-up · **Blocked** waits on +non-binary work · **N/A** out of scope by design. + +### By operation + +| Area | Request encode | Response negotiate | Response decode | Status | +|---|:--:|:--:|:--:|---| +| Point item ops (`create` / `read` / `replace` / `upsert`) | Yes | Yes | Yes | **Done** | +| `delete` | — (no body) | No | — | **Pending** — .NET negotiates it; see parity #2 | +| Query — single-partition | text by design | Yes | Yes | **Done** | +| Query — passthrough cross-partition | text by design | Yes | Yes | **Done** | +| Query — streaming ORDER BY (scalar keys) | text by design | Yes | Yes | **Done** | +| Query — `OFFSET` / `LIMIT` / `TOP` (SkipTake) | text by design | Yes | Yes | **Done** | +| Query — aggregate / GROUP BY / DISTINCT | — | — | — | **Blocked** — engine absent | +| `patch` | No | No | Yes | **N/A** — client-side RMW; its inner read/replace are encoded | +| Change feed / `ReadFeed` | No | No | capable, unused | **N/A** — backend does not honor the header | +| Transactional `batch` / `bulk` / stored procedures / control-plane | No | No | — | **N/A** — deferred by spec | + +### Pending work + +| # | Item | Why it matters | Size | +|---|---|---|---| +| 1 | **Gateway 2.0 / thin client** — carry `SupportedSerializationFormats` as an RNTBD metadata token | Customer-visible. The thin-client path re-encodes requests as an RNTBD token list with no such token, so the header is dropped and the service returns **text** — reintroducing the integral-`Double`→integer divergence (#5028) this feature exists to fix. A wide integer that round-trips on the standard gateway can still fail typed deserialization on a thin-client account. | Medium | +| 2 | **`parse_envelope_page` on binary (perf + fidelity)** — see below | Efficiency and byte fidelity only; no correctness gap | Medium–Large | +| 3 | **`delete` negotiation** — add to `supports_binary_response` to match .NET | Wire-scope parity; low impact (no request body, usually no response body) | Small | +| 4 | **Cross-implementation vectors** — validate against captured real .NET / Java binary output | Our encoder emits none of the compact forms (reference dedup, system strings), so emulator-based tests never exercise them. A slice-based reader would pass every test we have and still corrupt real service data. | Medium | +| 5 | **Aggregate / GROUP BY / DISTINCT** | **Blocked, not pending.** `validate_query_info` rejects all three cross-partition in *any* encoding, so there is no merge to make binary-aware. Whoever builds the engine owns the binary path with it — ideally on a format-agnostic value model (like .NET's `CosmosElement`) so binary is inherent, not retrofitted. Single-partition DISTINCT is a passthrough drain and already round-trips binary. | — | + +#### Detail: item 2, binary-aware `parse_envelope_page` + +A binary page is currently transcoded roughly three times per document: whole-page +binary→text, `serde_json` envelope parse, then a per-item text→binary +re-encode in `build_page`. A binary-aware reader would decode only `orderByItems` / +`_rid` and keep each payload as a **view** — the refcounted page `Bytes` plus an +offset — so emitted items are the service's original bytes. + +> **Do not slice a document out and re-prefix it with `0x80`.** Reference strings +> (`STR_R1`-`STR_R4`) resolve against *absolute page offsets* (see +> `Reader::resolve_reference`) and the interning scope is the whole page, so a +> detached sub-slice mis-resolves any reference pointing outside it — silently +> returning wrong text rather than erroring, whenever the target bytes happen to +> start with a string marker. + +A view keeps the page alive, and `Reader::new(buf, offset)` already reads from an +arbitrary start. Trade-off: a buffered row pins its whole source page (peak +retention ~ `fan_out x page_size`). ORDER BY gains most, because the merge fetches +from every partition but emits a subset — today every fetched page is transcoded in +full even when a `TOP` discards it. Blast radius is `ResponseBody::Items` (driver +public API, also consumed by the native FFI crate) plus a `build_page` restructure, +since merged rows span multiple source pages. + +Note also that because `emit_binary` is sticky on the merge, once any page is binary +the items sourced from **text** pages are re-encoded too, which normalizes key order +and collapses duplicate keys. + +#### Invariant for future feed splitters + +`skip_take_page::split_feed_envelope` detects a binary envelope, transcodes it, and +re-encodes each document **standalone**, so every `ResponseBody::Items` producer +emits per-document binary that `into_items` auto-detects by preamble. Any future +splitter must keep that invariant: slicing a single-preamble envelope without +re-encoding per document yields preamble-less sub-documents misrouted to the text +path. -* **Query response negotiation** — **done.** A `query_items` call now advertises a binary response via `x-ms-cosmos-supported-serialization-formats` (set once at the `plan_operation` choke point that every per-page request flows through). The query request body is a `application/query+json` spec, not a document, so it intentionally stays text — there is no query request-body encoding. The query *response* decodes via the shared choke point. **Standard-gateway path only:** the Gateway 2.0 / thin-client path re-encodes the request as an RNTBD metadata token list that has no `SupportedSerializationFormats` token, so the header is dropped there and the service returns **text**. This is a customer-visible limitation, not benign: text pages reintroduce the integral-`Double`→integer divergence (#5028) this feature exists to fix, so a wide integer that round-trips over binary on the standard gateway can still fail typed deserialization on a thin-client account. Adding the RNTBD `SupportedSerializationFormats` token is a **follow-up**. -* **Cross-partition / ORDER BY merge on binary bytes** — **scalar-key streaming ORDER BY now works on binary.** `parse_envelope_page` transcodes a binary-negotiated page to text before its text-only `serde_json` + `RawValue` envelope parse, so cross-partition ORDER BY over binary item bytes round-trips end to end. Single-partition and `into_single` query drains already round-trip binary directly. Remaining gap: **aggregate / GROUP BY / DISTINCT** merges over binary, plus a perf follow-up: a binary page is currently transcoded roughly three times per document (whole-page binary→text, `serde_json` envelope parse, then a per-item text→binary re-encode in `build_page`). A binary-aware envelope reader that yields each payload's binary sub-slice — re-prefixed with the `0x80` preamble — would reduce this to a slice + prefix and drop both the parse copy and the per-item re-encode. Note also that because `emit_binary` is sticky on the merge, once any page is binary the items sourced from **text** pages are re-encoded too, which normalizes key order and collapses duplicate keys. -* **Binary feed responses** — the `into_items` feed splitter scans **text** JSON, so binary `Documents` envelopes cannot be sliced by that splitter yet. (The SDK query path uses `into_single`, which is unaffected.) `ReadFeed` / change feed is excluded from binary negotiation: the backend does not honor the header for it. -* **`patch`** — excluded from binary encoding for now (the driver's request-side encode intentionally skips patch); transactional `batch` / `bulk` are deferred by spec. -* **Cross-implementation vectors** — validate against captured real .NET / Java binary output. +--- + +## Rust vs .NET parity + +How binary encoding compares to the .NET SDK (`Azure/azure-cosmos-dotnet-v3`). +Binary encoding spans three independent concerns: **request encode** (body +serialized as binary), **response negotiate** (advertise +`x-ms-cosmos-supported-serialization-formats: CosmosBinary`), and **response +decode** (auto-detected by the `0x80` first byte). + +> Last verified: 2026-08-10, against `azure-cosmos-dotnet-v3` `main`. + +### Enablement model + +| | .NET | Rust | +|---|---|---| +| Opt-in gate | `ConfigurationManager.IsBinaryEncodingEnabled()` (env var) + `ItemRequestOptions.EnableBinaryResponseOnPointOperations` | `BinaryEncodingOptions` (client default + per-op override) | +| Suppressed with custom serializer | Yes — `GetTargetResponseSerializationFormat` returns `Text` | N/A (SDK owns serde) | +| Response decode | Format-agnostic `JsonNavigator` (first-byte detect) | Shared `deserialize_response` / `is_binary` choke point | +| Status | Preview / opt-in | Preview / opt-in | + +### Divergences + +| # | Difference | Detail | Severity | +|---|---|---|---| +| 1 | Aggregate / GROUP BY / DISTINCT cross-partition | .NET runs them; its merge is on the format-agnostic `CosmosElement` model, so binary works for free. Rust's `validate_query_info` **rejects them in any encoding** — the engine does not exist yet. | Real capability gap (not binary-specific) | +| 2 | `delete` negotiation | .NET's `IsPointOperationSupportedForBinaryEncoding` includes `Delete`; Rust's `supports_binary_request_body` / `supports_binary_response` exclude it. | Minor — pending item 3 | +| 3 | Gateway 2.0 negotiation | Honored on the standard gateway only; the thin-client path drops the header and the service returns text. | Real gap — pending item 1 | +| 4 | Patch mechanism | .NET Patch is a real server op, not binary-negotiated. Rust Patch is a client-side read-modify-write, so its internal read/replace **are** encoded when enabled. Both functionally correct. | Cosmetic / architectural | +| 5 | Negotiation header value | .NET query default = `"JsonText,CosmosBinary"`; .NET point ops = `"CosmosBinary"`. Rust = `"CosmosBinary"` everywhere, so it forces binary rather than advertising "either". | Minor wire diff | + +### Matched by design + +* Point item ops encode requests and decode responses identically. +* Single-partition and passthrough cross-partition queries: text request body, negotiated binary response, per-page binary decode. +* Query request body always stays text (`application/query+json` is a query spec, not a document). +* Change feed / `ReadFeed` excluded from negotiation (the backend returns binary for ReadFeed-with-partition-key as a known bug). +* Batch / bulk / stored procedures / control-plane resources never use binary JSON. +* Response decode is a single format-agnostic choke point on both sides. + +### Bottom line + +For point operations, single-partition queries, and every cross-partition query +shape Rust currently supports, the two SDKs are **functionally equivalent** on +binary encoding. Divergence #1 is a missing query engine rather than a binary +issue; #2 and #3 are the actionable binary items. + +### .NET source references + +* `src/Handler/RequestInvokerHandler.cs` — `IsPointOperationSupportedForBinaryEncoding` (create/replace/delete/read/upsert). +* `src/RequestOptions/QueryRequestOptions.cs` — `PopulateRequestOptions` sets the header for `OperationType.Query` only (with the ReadFeed backend-bug comment). +* `src/Query/v2Query/DocumentQueryExecutionContextBase.cs` — `DefaultSupportedSerializationFormats = "JsonText,CosmosBinary"`. +* `src/Resource/Container/ContainerCore.Items.cs` — `GetTargetRequestSerializationFormat` / `GetTargetResponseSerializationFormat`. ---