Azure / azure-sdk-for-go

This repository is for active development of the Azure SDK for Go. For consumers of the SDK we recommend visiting our public developer docs at:
https://docs.microsoft.com/azure/developer/go/
MIT License
1.65k stars 844 forks source link

azcosmos: it seems that NewQueryItemsPager retrieves documents from multiple partitions #20543

Closed ItalyPaleAle closed 1 year ago

ItalyPaleAle commented 1 year ago

Bug Report

The documentation for NewQueryItemsPager states that it returns results limited to a single partition, and that is the behavior I desire.

However, I am getting results from multiple partitions.

These are (2 of) the test documents within the collection:

{
    "id": "cosmosdb-sidecar-7a0d0502-6acd-4e9a-b80a-23bc5a30a068||bulk-nopk||1",
    "value": "cosmosdb-sidecar-7a0d0502-6acd-4e9a-b80a-23bc5a30a068||bulk-nopk||1",
    "partitionKey": "cosmosdb-sidecar-7a0d0502-6acd-4e9a-b80a-23bc5a30a068||bulk-nopk||1",
}
{
    "id": "cosmosdb-sidecar-7a0d0502-6acd-4e9a-b80a-23bc5a30a068||bulk-pk||1",
    "value": "cosmosdb-sidecar-7a0d0502-6acd-4e9a-b80a-23bc5a30a068||bulk-pk||1",
    "partitionKey": "cosmosdb-sidecar-7a0d0502-6acd-4e9a-b80a-23bc5a30a068",
}

(I've omitted the internal properties)

This is the query I run:

pager := c.client.NewQueryItemsPager("SELECT * FROM r WHERE ARRAY_CONTAINS(@keys, r.id)", pk, queryOpts)

Where:

I would expect to retrieve only the key containing bulk-pk, since it's the only one with that partition key. Instead, surprisingly I'm getting back the other key too.

It seems that this is performing cross-partition queries.

ealsur commented 1 year ago

The REST API request passes the Partition Key header and our tests cover exactly that behavior: https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/data/azcosmos/emulator_cosmos_query_test.go#L86 (the test creates 2 sets of documents and queries on a single pk).

The code is not passing the Cross Partition Queries header (https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/data/azcosmos/cosmos_query_request_options.go#L40-L72)

Query resolution then it's up to the service.

What is the Partition Key Definition for this container?

ItalyPaleAle commented 1 year ago

This is very odd then. Is it because my query uses ARRAY_CONTAINS maybe?

What is the Partition Key Definition for this container?

/partitionKey

ealsur commented 1 year ago

@ItalyPaleAle If you remove the ARRAY_CONTAINS and just filter with an OR clause, does it work differently?

ealsur commented 1 year ago

I created a test suite that mirrors what is described in the issue and the test only gets 1 document:

https://github.com/ealsur/azure-sdk-for-go/commit/6051edfdfe047c840ea79fbb0dfec71fe96bc8ea

ItalyPaleAle commented 1 year ago

Uhmmm I'd be damned, I can't repro the issue anymore now, even though it happened consistently last night.

Maybe it was a service issue? Something transient?

Anyways, thanks for checking as well. I'll close this issue.

ealsur commented 1 year ago

@ItalyPaleAle I don't know if there were any new service-side deployments that happened last night, but please re-activate if needed.

ItalyPaleAle commented 1 year ago

Will do.

In the meanwhile, I've used this as an opportunity to implement some additional checks. So if for whatever reason we were to get back records from a different partition, they'd be ignored and wouldn't cause Dapr to crash like before.

I like to call these fixes "787-style", where we don't know what causes them or how to prevent them, but at least we mitigate them if they were to happen 🤣

ealsur commented 1 year ago

@ItalyPaleAle could it be that Dapr is setting the cross-partition header in the request and that might cause this behavior? https://github.com/dapr/components-contrib/blob/8488a78dd7598992ce597e8f7a16ee2824bac97e/state/azure/cosmosdb/cosmosdb.go#L92

ItalyPaleAle commented 1 year ago

@ealsur I think you're right. That was meant to only apply to some scenario, but it looks like it was implemented "globally" on the client.

Good catch, that would explain what's going on. Thanks!