-
Notifications
You must be signed in to change notification settings - Fork 3.3k
fix - full partition key routing bug #48237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
dibahlfi
wants to merge
3
commits into
main
Choose a base branch
from
users/dibahl/full-partitionkey-regression
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |||||
| #### Breaking Changes | ||||||
|
|
||||||
| #### Bugs Fixed | ||||||
| * Fixed complete-partition-key queries scanning documents instead of using partition-key routing, which caused excessive RU consumption and latency for aggregates such as `COUNT`. See [PR 48237](https://github.com/Azure/azure-sdk-for-python/pull/48237) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| #### Other Changes | ||||||
|
|
||||||
|
|
||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| apiMdSha256: 1538a79b2c2da38fb83fd37ccea945314bf2f34218acda3d8f15e2f0268f3040 | ||
| parserVersion: 0.3.28 | ||
| parserVersion: 0.3.30 | ||
| pythonVersion: 3.13.14 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -93,7 +93,6 @@ | |
| from ._inference_service import _InferenceService | ||
| from .documents import ConnectionPolicy, DatabaseAccount | ||
| from .partition_key import ( | ||
| _build_partition_key_from_properties, | ||
| _Undefined, | ||
| _Empty, | ||
| _PartitionKeyKind, | ||
|
|
@@ -3368,9 +3367,21 @@ def __GetBodiesFromQueryResult(result: dict[str, Any]) -> list[dict[str, Any]]: | |
| base.set_session_token_header(self, req_headers, path, request_params, options, partition_key_range_id) | ||
|
|
||
| # Check if the overlapping ranges can be populated | ||
| # | ||
| # Complete partition keys are classified upstream in | ||
| # container.py::query_items and routed via the PartitionKey request | ||
| # header on __Post. Do NOT re-add an EPK conversion for complete keys | ||
| # here: it drops the PartitionKey header and defeats the server-side | ||
| # index-only aggregate (COUNT/etc.), causing a full partition scan. | ||
| # Only feed ranges and hierarchical-prefix keys belong on the EPK path | ||
| # below. Note the sync classifier lives in container.py while the async | ||
| # twin classifies inside __QueryFeed, so do not mirror the async branch | ||
| # into this file. | ||
| # | ||
| # The container_properties pop below is a no-op in production (nothing | ||
| # sets that kwarg anymore); it is retained solely as a test API shim. | ||
|
Comment on lines
+3381
to
+3382
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we no longer do this in production do we still need the code that pops it? what is the value of the test that uses this? |
||
| feed_range_epk = None | ||
| container_properties = kwargs.pop("container_properties", None) | ||
| is_full_pk_scope = False | ||
| kwargs.pop("container_properties", None) | ||
|
dibahlfi marked this conversation as resolved.
|
||
| if "feed_range" in kwargs: | ||
| feed_range = kwargs.pop("feed_range") | ||
| feed_range_epk = FeedRangeInternalEpk.from_json(feed_range).get_normalized_range() | ||
|
|
@@ -3379,27 +3390,6 @@ def __GetBodiesFromQueryResult(result: dict[str, Any]) -> list[dict[str, Any]]: | |
| prefix_partition_key_value: _SequentialPartitionKeyType = kwargs.pop("prefix_partition_key_value") | ||
| feed_range_epk = ( | ||
| prefix_partition_key_obj._get_epk_range_for_prefix_partition_key(prefix_partition_key_value)) | ||
| elif options.get("partitionKey") is not None and container_properties is not None: | ||
| partition_key_value = options["partitionKey"] | ||
| partition_key_obj = _build_partition_key_from_properties(container_properties) | ||
| if not partition_key_obj._is_prefix_partition_key(partition_key_value): | ||
| # Full-PK returns a single-value inclusive range; normalize to | ||
| # [min, max) before routing-map overlap resolution. | ||
| # | ||
| # NOTE: do NOT pop the PartitionKey header here. The pop is | ||
| # deferred to the `if pagination_state is not None:` block | ||
| # below, i.e. until we've confirmed the new feed-range | ||
| # routing path is actually taking over. If routing comes back | ||
| # with zero overlaps (stale cache, mid-split, etc.) we fall | ||
| # through to the regular __Post path, and that fallthrough | ||
| # must still carry the legacy PK header — otherwise the | ||
| # backend gets a request with no partition scoping and either | ||
| # raises BAD_REQUEST (cross-partition disabled) or silently | ||
| # runs an unscoped cross-partition query (wrong results). | ||
| feed_range_epk = partition_key_obj._get_epk_range_for_partition_key( | ||
| partition_key_value | ||
| ).to_normalized_range() | ||
| is_full_pk_scope = True | ||
|
|
||
| # If feed_range_epk exist, query with the range | ||
| if feed_range_epk is not None: | ||
|
|
@@ -3446,21 +3436,17 @@ def _is_input_scope_single_partition() -> bool: | |
| return cached_is_single_partition | ||
|
|
||
| if inbound_serialized_continuation and inbound_token_payload is None: | ||
| scope_is_single_partition = False | ||
| if not is_full_pk_scope: | ||
| scope_is_single_partition = _is_input_scope_single_partition() | ||
| scope_is_single_partition = _is_input_scope_single_partition() | ||
| if _should_bridge_legacy_continuation( | ||
| inbound_serialized_continuation, | ||
| inbound_token_payload, | ||
| is_full_pk_scope, | ||
| scope_is_single_partition, | ||
| ): | ||
| legacy_bridge_in_use = True | ||
| # Hot path: legacy is the normal inbound shape for full-PK | ||
| # and currently-single-partition feed-range queries (we | ||
| # just emitted one). The bridge wires the legacy string | ||
| # into the internal pagination queue; the outbound token | ||
| # format on the next page is unchanged. | ||
| # Hot path: legacy is the normal inbound shape for currently | ||
| # single-partition feed-range queries. The bridge wires the | ||
| # string into the internal pagination queue; the outbound | ||
| # token format on the next page is unchanged. | ||
| _LOGGER.debug( | ||
| "Bridging inbound legacy continuation into internal pagination state; " | ||
| "outbound token format will remain unchanged (legacy single-string)." | ||
|
|
@@ -3510,13 +3496,6 @@ def _is_input_scope_single_partition() -> bool: | |
| ) | ||
|
|
||
| if pagination_state is not None: | ||
| if is_full_pk_scope: | ||
| # Drop the legacy partition-key header now that the | ||
| # feed-range routing path is taking over. The inner POSTs | ||
| # in the loop set PartitionKeyRangeID / StartEpkString / | ||
| # EndEpkString explicitly; sending both routing styles on | ||
| # one request is undefined on the service side. | ||
| req_headers.pop(http_constants.HttpHeaders.PartitionKey, None) | ||
| results: dict[str, Any] = {} | ||
| feedrange_response_headers: CaseInsensitiveDict = CaseInsensitiveDict() | ||
| consecutive_no_progress_pages = 0 | ||
|
|
@@ -3532,8 +3511,7 @@ def _checkpoint_and_reraise(error: Exception) -> NoReturn: | |
| resource_id_str, | ||
| query, | ||
| feed_range_epk, | ||
| is_full_pk_scope, | ||
| (not is_full_pk_scope) and _is_input_scope_single_partition(), | ||
| _is_input_scope_single_partition(), | ||
| ) | ||
| except Exception as continuation_write_error: # pylint: disable=broad-exception-caught | ||
| _LOGGER.warning( | ||
|
|
@@ -3701,8 +3679,7 @@ def _checkpoint_and_reraise(error: Exception) -> NoReturn: | |
| resource_id_str, | ||
| query, | ||
| feed_range_epk, | ||
| is_full_pk_scope, | ||
| (not is_full_pk_scope) and _is_input_scope_single_partition(), | ||
| _is_input_scope_single_partition(), | ||
| ) | ||
| # End feed_range pagination block. | ||
| self.last_response_headers = feedrange_response_headers | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.