-
Notifications
You must be signed in to change notification settings - Fork 283
Cosmos: fix #2616 by using RawValue, and fix query rewriting #2617
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
base: main
Are you sure you want to change the base?
Cosmos: fix #2616 by using RawValue, and fix query rewriting #2617
Conversation
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
e237068
to
dfe2868
Compare
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.
Pull Request Overview
This PR fixes issue #2616 by addressing the query engine’s contract with RawValue and ensuring that the rewritten query from the pipeline is used by the SDK executor. Key changes include updating the return type to Box for query items, modifying the executor to properly set and use the base query request, and adjusting the Cargo.toml feature flag for the preview query engine.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
sdk/cosmos/azure_data_cosmos/tests/framework/query_engine.rs | Updated pop_item return type to use RawValue. |
sdk/cosmos/azure_data_cosmos/src/query/mod.rs | Exposed the query text field to accommodate rewriting. |
sdk/cosmos/azure_data_cosmos/src/query/executor.rs | Revised executor initialization and query rewriting to use the pipeline’s authoritative query. |
sdk/cosmos/azure_data_cosmos/src/query/engine.rs | Modified PipelineResult to use Box for items. |
sdk/cosmos/azure_data_cosmos/Cargo.toml | Adjusted the preview_query_engine feature to enable serde_json/raw_value support. |
Comments suppressed due to low confidence (2)
sdk/cosmos/azure_data_cosmos/src/query/executor.rs:74
- [nitpick] Consider using a more descriptive expect message that provides additional context about the base_request's usage to aid debugging.
self.base_request.as_ref().expect("base_request should be set when pipeline is set")
sdk/cosmos/azure_data_cosmos/src/query/executor.rs:110
- [nitpick] Consider replacing the generic expect message "we just set it" with a more informative message to better explain the assumption behind pipeline initialization.
self.pipeline.as_mut().expect("we just set it")
self.pipeline = Some(pipeline); | ||
self.pipeline.as_mut().expect("we just set it") | ||
( | ||
self.pipeline.as_mut().expect("we just set it"), |
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.
Nit: since this is impossible, maybe just "unwrap" and avoid the pooled string? Just a comment instead?
Closes #2616
It's a tale as old as time. Abstraction is written, then implemented, and found lacking somewhere ;). This PR has a few small fix-ups for the query engine abstraction that I found while integrating the query engine. Once these were fixed, it worked perfectly though! 🎉
Here are the changes:
The query engine works with
serde_json::value::RawValue
, which represents "unparsed" JSON. It's just a wrapper aroundstr
, and I had hoped it would be easy to consume it and convert it in to aVec<u8>
, so that's what I had the QueryPipeline abstraction expect as a result from the pipeline. Alas, it's not really possible to do that. You can access the underlying bytes viaRawValue::get()
but that returns&str
and there's no way to "move" the bytes out of theRawValue
. But that's OK, we can just have the pipeline send backBox<RawValue>
values directly, since the pipeline is always working with JSON anyway. So, that's what I did. (This is #2616, which I filed when I thought I could work around it and fix the SDK later, but I couldn't do that with the below issue so I decided to just fix them both here first).There was also a bug in how the query "executor" worked. I built the base query request as soon as the pipeline was created, using the query the user passed in. But the gateway may rewrite the query. The query engine handles this and exposes the "authoritative" query out of
QueryPipeline::query()
, but the SDK's executor ignored that and used the original query. A pretty simple fix.