-
Notifications
You must be signed in to change notification settings - Fork 999
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
Allow to filter queries by child entity #3184
Conversation
9afb13b
to
46d2872
Compare
Could you rebase this to latest master when you get a chance? |
46d2872
to
771a995
Compare
@lutter done! :) Note that this PR is only for filtering, for sorting we have this one: #3096 (targeted into this PR, not |
809b6fd
to
f34918a
Compare
f34918a
to
2a77b5d
Compare
1e9f27b
to
5523cf7
Compare
5523cf7
to
6e04d05
Compare
6e04d05
to
6f9312f
Compare
b863adb
to
461a827
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.
Getting closer! I actually hadn't thought about the issue that forced you to use group by
but I think using the exists(select 1 ..)
strategy I outlined in the comment will make this easier.
We'll also need tests for all this; the best place for them is probably graphql/tests/query.rs
- I recently revamped them to (hopefully) make it easier to add tests there. The tests for this should use child filters that traverse Musician.bands
, Musician.written_songs
, Band.members
, Song.written_by
and SongStat.song
. For example, what I have in mind for Musician.bands
is something like musicians(where: { bands_: { name: "something" } }) { id }
461a827
to
2c30a78
Compare
f465e1e
to
56e515c
Compare
56e515c
to
21bbe8b
Compare
e6ab8cd
to
322beea
Compare
2a61917
to
bdc00dc
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.
This is getting really, really close. There's a few smallish things to touch up, but other than that, this is almost ready.
@@ -82,6 +82,19 @@ impl From<&str> for EntityType { | |||
|
|||
impl CheapClone for EntityType {} | |||
|
|||
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord, Hash)] | |||
pub struct EntityFilterDerivative(bool); |
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.
This would be better as an enum AttributeStorage { Direct, Derived }
; even clearer would be to make EntityFilter::Child
a struct, i.e.
Child {
attr: Attribute,
entity_type: EntityType,
filter: Box<EntityFilter>,
derived: bool,
},
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.
graphql/src/schema/api.rs
Outdated
return Some(input_values); | ||
} | ||
|
||
let input_field_type = input_field_type.unwrap(); |
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.
This, from line 401, would be clearer and avoid an unwrap()
like this:
let input_field_type = match input_field_type {
None => {
let mut input_values: Vec<InputValue> = vec![];
if let Some(parent) = parent_type_name {
extend_with_child_filter_input_value(field, &parent, &mut input_values);
}
return Some(input_values);
}
Some(input_field_type) => input_field_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.
Or, even better to do what follows in the match, too and avoid the early return:
let mut input_values: Vec<InputValue> = match input_field_type {
None => {
vec![]
}
Some(input_field_type) => vec![
"",
"not",
"contains",
"contains_nocase",
"not_contains",
"not_contains_nocase",
]
.into_iter()
.map(|filter_type| {
input_value(
&field.name,
filter_type,
Type::ListType(Box::new(Type::NonNullType(Box::new(
input_field_type.clone(),
)))),
)
})
.collect(),
};
if let Some(parent) = parent_type_name {
extend_with_child_filter_input_value(field, &parent, &mut input_values);
}
Some(input_values)
This could also be a let mut input_values = input_field_type.map(|input_field_type| ...).unwrap_or(vec![])
but I am fine with either
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.
graphql/src/store/query.rs
Outdated
Type::NonNullType(inner) => resolve_type_name(inner), | ||
Type::NamedType(name) => name, | ||
} | ||
} |
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.
This is get_base_type
from graph/src/data/graphql
(just do something like use graph::data::graphql::TypeExt as _
to be able to use 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.
Child(attr, entity, child_filter, _) => { | ||
if child_filter_ancestor { | ||
return Err(StoreError::QueryExecutionError( | ||
"Only a single level sub filter is allowed".to_string(), |
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.
How about something like Child filters can not be nested
?
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.
|
||
out.push_sql("exists (select 1"); | ||
|
||
out.push_sql(" from "); |
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.
Very minor but these two push_sql
could be collapsed into one
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 parent_column = self.table.primary_key(); | ||
|
||
if child_column.is_list() { | ||
// Type A: p.id = any(c.{parent_field}) |
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.
Shouldn't that be c.id = any(i.{parent_field})
? And similar in the comments for the other cases
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.
out.push_sql(child_prefix); | ||
out.push_identifier(BLOCK_RANGE_COLUMN)?; | ||
out.push_sql(" @> "); | ||
out.push_bind_param::<Integer, _>(&self.block)?; |
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.
This needs to use BlockRangeColumn::contains
- the way it is written now only works for mutable entities, but would fail for immutable ones since they don't have a block_range
column, only a block$
column
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.
This is good to go - there's one small thing around how the commits are structured/named: see this for how we usually do that. With this PR, it might be best to just squash everything into a mega commit |
@ALL (@lutter, @kamilkisiela) when is the ETA for merge of this awesome addition to the graphprotocol? :) |
474af82
to
b68d755
Compare
b68d755
to
976dc62
Compare
@lutter I squashed all commits and renamed it |
@kamilkisiela @lutter and anyone else involved in this PR. Kinda late to the party on this one, but I'd like to offer a HUGE thank you for getting this into the graph-node. It's just such an amazing boost to querying. Thank you!! |
#2960
Given the schema:
The
sender_
is added toPurpose_filter
input object.input Purpose_filter { ... + sender_: Sender_filter }
The
Sender_filter
input object is untouched.Example
Child filters are limited to be only 1 level deep, so no
Child(Child(...))
and this pull request DOES NOT implement a support for interface types.Continues #3022