-
Notifications
You must be signed in to change notification settings - Fork 272
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
Type conditioned fetching #4748
Conversation
This comment has been minimized.
This comment has been minimized.
CI performance tests
|
…c is not up to date, but the number of fetches is accurate.
…e been filtered out
apollo-router/src/json_ext.rs
Outdated
@@ -21,6 +24,11 @@ pub(crate) type Object = Map<ByteString, Value>; | |||
|
|||
const FRAGMENT_PREFIX: &str = "... on "; | |||
|
|||
static TYPE_CONDITIONS_REGEX: Lazy<Regex> = Lazy::new(|| { | |||
Regex::new(r"(?:\|\[)(.+?)(?:,\s*|)(?:\])") |
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: Maybe easier to understand when captures are used if these are named.
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.
fixed in d14d077
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.
I'm not sure this is really fixed. I can see that you've named one of the groups "", but I can't see where it's being used. It looks like you are still using the indices of the various captures.
e.g.:
let path = TYPE_CONDITIONS_REGEX.replace(s, |caps: &Captures| {
type_conditions.extend(
caps.extract::<1>()
.1
.map(|s| s.split(',').map(|s| s.to_string()))
.into_iter()
.flatten(),
);
""
});
You are always using extract, group 1
, which is a bit opaque. If you want to use the name then it would be something like: caps.name("condition")
and you'd need to do some handling for not being present.
Anyway, it was a NIT, so I'm fine with the extract
approach that you are using, in which case no need to name the capture group.
If you do think it's worth naming the various capture groups, then you should use them to make the code a bit less opaque when you are pulling out the conditions as I outlined above.
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.
actually fixed in 6c67cac 😁
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.
Now you are making me feel really picky, but there multiple used of extract()
. May as well change all of them.
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.
Let's followup on that!
This is the rust counterpart of #4748
This is the rust counterpart of #4748
Type conditioned fetching (PR #4748)
Type conditioned fetching
When querying a field that is in a path of 2 or more unions, the query planner was not able to handle different selections and would aggressively collapse selections in fetches yielding an incorrect plan.
This change introduces an experimental configuration option to enable type conditioned fetching:
By @o0Ignition0o in #4748