-
Notifications
You must be signed in to change notification settings - Fork 1
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
FetchGraph
andFetchStepData
structs have been refactored to use a type-state pattern, generic overSingleTypeFetchStep
orMultiTypeFetchStep
. 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
FetchStepSelections
struct has been introduced to centralize the management of output selection sets for fetch steps. This replaces the previousTypeAwareSelection
for 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
SingleTypeFetchStep
for simpler, single-type selections. After this initial phase, it is explicitly converted toMultiTypeFetchStep
before optimization, enabling more complex multi-type selection handling and optimizations. - Improved Error Reporting: The
UnsupportedFieldInEntities
error 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.lock
dependencies, 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.rs
and is usingSingleTypeFetchStep
as state.This ensures that the step is built with a single type in mind.
fetch/optimize/
files, and usingMultiTypeFetchStep
as 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
struct
for dealing with fetch-step selectionsoutput
input
bench
operation worksexpect
andtry_as_single