-
Notifications
You must be signed in to change notification settings - Fork 247
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
[Cosmos] Add APIs to perform single-partition queries against a container #1814
Conversation
UPDATE: I decided to just leave things as-is in
I was just reminded why I went with the lifetime approach when I looked at this earlier in the week. The problem is that if we continue to require that let pipeline = self.pipeline.clone(); // A clone so we can move into the closure.
Pageable::new(move |continuation| {
// Another clone so we can move in to the `async` block.
let pipeline = pipeline.clone();
async move {
pipeline.send(...).await...
}
}); Currently Another option might be to allow a Having said that, it looks like maybe that's what the existing unofficial SDK code does? Maybe I'm fussing about a non-issue? Would be interested to hear any thoughts you have here @heaths. Putting a lifetime parameter on fn query_items<'a>(&'a self, query: &str) -> Pageable<'a, ...>; In practice, the lifetimes are all elided, so you don't see them. The main impacts of putting a lifetime parameter on
|
4b44745
to
f0b03d1
Compare
Cloning the pipeline is designed to be cheap. It's a For now until we get substantial feedback otherwise, I think we should just clone the pipeline. |
IIRC, we have some helper functions already that might be close or even do that. IF they don't, I suggest actually adding some globals. You definitely won't be the only ones writing "params" as headers. That's another reason why I'm pretty sure we already have serde de/serialization helpers for that. Update: Never mind. I'm conflating https://github.com/Azure/azure-sdk-for-rust/blob/feature/track2/sdk/typespec/typespec_client_core/src/date/mod.rs but the idea could be the same, along with implementing All that said, I haven't reviewed the code just yet but going through your ample PR comments hereabove first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I love it! A few nits and questions, but nothing blocking.
I initially started by implementing |
The next iteration is live, I think I've covered all the feedback. I'm going to try and land this soon so that I can move on to item CRUD (#1811) |
441201e
to
78026bd
Compare
5205efb
to
b747266
Compare
Closes #1809
This adds the initial rudimentary APIs to perform a single-partition query against items in a container.
Some example code (see the
cosmos_query
example for a larger example):A few pieces of note:
A query can be parameterized. To do that, instead of passing the raw string in, you'd call
Query::from("...")
and then call.with_parameter("@name", "value")
on the resultingQuery
to build up the query and parametersWe only support single-partition queries. This is aligned with the support we currently provide in other SDKs that only support Gateway mode. Over time we can expand this to cross-partition queries, but there are significant limitations to those.
For serializing Partition Keys we have to do something a little custom and can't just use
serde_json
as-is. The partition key for a query is encoded as an HTTP header, which means we have to escape any non-ASCII characters because HTTP headers may not contain non-ASCII characters. Serde's default JSON serialization does not perform that escaping, it allows non-ASCII UTF-8 characters to pass through unescaped. Even Rust's built-in escaping is unsuitable because it uses the format\u{XXXX}
, when JSON requires\uXXXX
(sans-{}
). So we do some custom stuff. Importantly, we still use the standard library's logic to encode characters as UTF-16, which handles surrogate pairs properly.UPDATE: I reverted this change and decided to leave
Pageable
as-is I changedPageable
to include a lifetime parameter. Previously, it expected the provided closure to return a future with the'static
lifetime, which would require the Future to own all necessary data. However, the implementation I used forquery_items
required that the future borrowedself.pipeline
, so it could use the same HTTP pipeline to make the requests. That isn't totally necessary. I could clone the pipeline and give the clone to the closure, but that seemed heavier to me, so I hesitated to do that. Though, after the conversations we've had about lifetimes, I'm waffling on that and considering rolling that change back and requiring that thePageable
hold its own HTTP pipeline and avoid being coupled to the lifetime of the client that created it. I'll continue to look into that.There are also fairly substantial API docs in place. We don't have a great way to preview those docs (maybe we should? publish updated docs to an Azure Static Web App on PR build?) but you can run
cargo doc --package azure_data_cosmos --open
, if you're on a local machine or RDP, or use #1808, if on a codespace, to view them.