-
Couldn't load subscription status.
- Fork 3
refactor(qp): introduce Single/Multi state for FetchGraph and steps #280
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?
Conversation
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.
Summary of Changes
Hello @dotansimha, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly refactors the query planner's internal representation of fetch graphs and steps. By introducing a type-state pattern and a dedicated structure for managing selection sets, the changes aim to enhance the clarity, correctness, and extensibility of the query planning logic, especially for advanced scenarios involving federated queries and type-specific selections.
Highlights
- Type-State Refactoring: The
FetchGraphandFetchStepDatastructs have been refactored to use a type-state pattern, generic overSingleTypeFetchSteporMultiTypeFetchStep. This allows for more precise modeling of fetch steps during different phases of query planning, distinguishing between single-type and multi-type selection handling. - Centralized Selection Management: A new
FetchStepSelectionsstruct has been introduced to centralize the management of output selection sets for fetch steps. This replaces the previousTypeAwareSelectionfor output and provides robust mechanisms for handling selections across multiple types, merging, and applying aliases. - Two-Phase Graph Construction: The fetch graph is now initially constructed with
SingleTypeFetchStepfor simpler, single-type selections. After this initial phase, it is explicitly converted toMultiTypeFetchStepbefore optimization, enabling more complex multi-type selection handling and optimizations. - Improved Error Reporting: The
UnsupportedFieldInEntitieserror variant has been enhanced to include both the type name and the field name, providing more detailed context for debugging minification errors. - Dependency Downgrades: Several
Cargo.lockdependencies, includingclap,hyper-util,rustls, andwinnow, have been downgraded to earlier versions. - New Benchmark Test: A new test,
test_bench_operation, has been added to benchmark the query planner's performance and correctness with a larger, more complex GraphQL operation and supergraph schema.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request introduces a significant and well-structured refactoring by introducing SingleTypeFetchStep and MultiTypeFetchStep states for FetchGraph and FetchStepData. This greatly improves type safety and clarity throughout the query planning process. The introduction of FetchStepSelections is also a great step towards better encapsulation of selection logic.
I've identified a few areas for improvement, mainly concerning potential panics in the new selections.rs module which should be addressed to ensure robustness. I've also noted some dependency downgrades in Cargo.lock that would be good to clarify.
Overall, this is a high-quality refactoring.
✅
|
35eab7d to
caca758
Compare
| pub fn to_multi_type(self) -> FetchGraph<MultiTypeFetchStep> { | ||
| let new_graph = self | ||
| .graph | ||
| .map(|_, w| w.clone().into_multi_type(), |_, _| ()); |
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.
We need to clone here?
lib/query-planner/src/planner/fetch/optimize/merge_passthrough_child.rs
Outdated
Show resolved
Hide resolved
6b87255 to
5169f7a
Compare
de0928e to
d5d5b7c
Compare
… replace `output`
e9864f4 to
a80cb5b
Compare
The "state" representation of a fetch step and fetch graph.
We split the work we do on the FetchGraph to to distinct parts:
fetch_step.rsand is usingSingleTypeFetchStepas state.This ensures that the step is built with a single type in mind.
fetch/optimize/files, and usingMultiTypeFetchStepas state.This ensures that the step is optimized with multiple types in mind.
With this setup, we can ensure that some parts of the logic/capabilities that can be performed on a selection set or a step are limited,
and either scoped to a single type or a multi-type context.
TODO
structfor dealing with fetch-step selectionsoutputinputbenchoperation worksexpectandtry_as_single