Skip to content

Commit

Permalink
Refactor IR to allow transformed values on the LHS of filter operations.
Browse files Browse the repository at this point in the history
  • Loading branch information
@obi1kenobi
obi1kenobi committed Jun 13, 2024
1 parent ead46c5 commit be76adc
Show file tree
Hide file tree
Showing 114 changed files with 483 additions and 307 deletions.
8 changes: 4 additions & 4 deletions trustfall_core/src/frontend/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ use crate::{
ir::{
get_typename_meta_field, Argument, ContextField, EdgeParameters, Eid, FieldRef, FieldValue,
FoldSpecificField, FoldSpecificFieldKind, IREdge, IRFold, IRQuery, IRQueryComponent,
IRVertex, IndexedQuery, LocalField, Operation, Recursive, TransformationKind, Type, Vid,
TYPENAME_META_FIELD,
IRVertex, IndexedQuery, LocalField, Operation, OperationSubject, Recursive,
TransformationKind, Type, Vid, TYPENAME_META_FIELD,
},
schema::{get_builtin_scalars, FieldOrigin, Schema},
util::{BTreeMapTryInsertExt, TryCollectUniqueKey},
Expand Down Expand Up @@ -232,15 +232,15 @@ fn make_local_field_filter_expr(
property_name: &Arc<str>,
property_type: &Type,
filter_directive: &FilterDirective,
) -> Result<Operation<LocalField, Argument>, Vec<FrontendError>> {
) -> Result<Operation<OperationSubject, Argument>, Vec<FrontendError>> {
let left = LocalField { field_name: property_name.clone(), field_type: property_type.clone() };

filters::make_filter_expr(
schema,
component_path,
tags,
current_vertex_vid,
left,
OperationSubject::LocalField(left),
filter_directive,
)
}
Expand Down
43 changes: 36 additions & 7 deletions trustfall_core/src/interpreter/execution.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ use std::{
use crate::{
ir::{
Argument, ContextField, EdgeParameters, Eid, FieldRef, FieldValue, FoldSpecificFieldKind,
IREdge, IRFold, IRQueryComponent, IRVertex, IndexedQuery, LocalField, Operation, Recursive,
Vid,
IREdge, IRFold, IRQueryComponent, IRVertex, IndexedQuery, LocalField, Operation,
OperationSubject, Recursive, Vid,
},
util::BTreeMapTryInsertExt,
};
Expand Down Expand Up @@ -105,7 +105,7 @@ fn compute_component<'query, AdapterT: Adapter<'query> + 'query>(
iterator = coerce_if_needed(adapter.as_ref(), carrier, root_vertex, iterator);

for filter_expr in &root_vertex.filters {
iterator = apply_local_field_filter(
iterator = apply_filter_with_arbitrary_subject(
adapter.as_ref(),
carrier,
component,
Expand Down Expand Up @@ -713,12 +713,35 @@ mismatch on whether the fold below {expanding_from_vid:?} was inside an `@option
Box::new(final_iterator)
}

fn apply_filter_with_arbitrary_subject<'query, AdapterT: Adapter<'query>>(
adapter: &AdapterT,
carrier: &mut QueryCarrier,
component: &IRQueryComponent,
current_vid: Vid,
filter: &Operation<OperationSubject, Argument>,
iterator: ContextIterator<'query, AdapterT::Vertex>,
) -> ContextIterator<'query, AdapterT::Vertex> {
let subject = filter.left();

match subject {
OperationSubject::LocalField(field) => apply_local_field_filter(
adapter,
carrier,
component,
current_vid,
filter.map_left(|_| field),
iterator,
),
OperationSubject::TransformedField(_) => todo!(),
}
}

fn apply_local_field_filter<'query, AdapterT: Adapter<'query>>(
adapter: &AdapterT,
carrier: &mut QueryCarrier,
component: &IRQueryComponent,
current_vid: Vid,
filter: &Operation<LocalField, Argument>,
filter: Operation<&LocalField, &Argument>,
iterator: ContextIterator<'query, AdapterT::Vertex>,
) -> ContextIterator<'query, AdapterT::Vertex> {
let local_field = filter.left();
Expand All @@ -730,7 +753,7 @@ fn apply_local_field_filter<'query, AdapterT: Adapter<'query>>(
carrier,
component,
current_vid,
&filter.map(|_| (), |r| r),
&filter.map(|_| (), |r| *r),
field_iterator,
)
}
Expand Down Expand Up @@ -1051,8 +1074,14 @@ fn perform_entry_into_new_vertex<'query, AdapterT: Adapter<'query>>(
let vertex_id = vertex.vid;
let mut iterator = coerce_if_needed(adapter, carrier, vertex, iterator);
for filter_expr in vertex.filters.iter() {
iterator =
apply_local_field_filter(adapter, carrier, component, vertex_id, filter_expr, iterator);
iterator = apply_filter_with_arbitrary_subject(
adapter,
carrier,
component,
vertex_id,
filter_expr,
iterator,
);
}
Box::new(iterator.map(move |mut x| {
x.record_vertex(vertex_id);
Expand Down
127 changes: 85 additions & 42 deletions trustfall_core/src/interpreter/hints/vertex_info.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use crate::{
interpreter::InterpretedQuery,
ir::{
Argument, FieldRef, FieldValue, IREdge, IRFold, IRQueryComponent, IRVertex, LocalField,
Operation, Vid,
Operation, OperationSubject, Vid,
},
};

Expand Down Expand Up @@ -169,12 +169,12 @@ impl<T: InternalVertexInfo + super::sealed::__Sealed> VertexInfo for T {
.filter(|c| c.vertex_id == current_vertex.vid)
.map(|c| RequiredProperty::new(c.field_name.clone()));

let properties = properties.chain(
current_vertex
.filters
.iter()
.map(|f| RequiredProperty::new(f.left().field_name.clone())),
);
let properties = properties.chain(current_vertex.filters.iter().map(|f| {
RequiredProperty::new(match f.left() {
OperationSubject::LocalField(field) => field.field_name.clone(),
OperationSubject::TransformedField(_) => todo!(),
})
}));

let properties = properties.chain(current_component.vertices.values().flat_map(|v| {
v.filters
Expand Down Expand Up @@ -210,11 +210,12 @@ impl<T: InternalVertexInfo + super::sealed::__Sealed> VertexInfo for T {
}

let query_variables = self.query_variables();
let vertex = self.current_vertex();

// We only care about filtering operations that are both:
// - on the requested property of this vertex, and
// - statically-resolvable, i.e. do not depend on tagged arguments
let mut relevant_filters = filters_on_local_property(self.current_vertex(), property)
let mut relevant_filters = filters_on_local_property(vertex, property)
.filter(|op| {
// Either there's no "right-hand side" in the operator (as in "is_not_null"),
// or the right-hand side is a variable.
Expand All @@ -223,10 +224,24 @@ impl<T: InternalVertexInfo + super::sealed::__Sealed> VertexInfo for T {
.peekable();

// Early-return in case there are no filters that apply here.
let field = relevant_filters.peek()?.left();
let first_filter = relevant_filters.peek()?;

// We currently ignore filters applied to transformed values of local properties.
// This is sound because each filter we consider only *narrows* the set of possible values,
// meaning that non-evaluated filters are a lost optimization opportunity,
// not a correctness problem.
//
// If tweaking this logic, make sure to tweak the logic in `filters_on_local_property`
// as well -- their behaviors have to match!
//
// TODO: This is an optimization opportunity for the future.
let local_field = match first_filter.left() {
OperationSubject::LocalField(field) => field,
OperationSubject::TransformedField(_) => return None,
};

let candidate =
compute_statically_known_candidate(field, relevant_filters, query_variables)
compute_statically_known_candidate(local_field, relevant_filters, query_variables)
.map(|x| x.into_owned());
debug_assert!(
// Ensure we never return a range variant with a completely unrestricted range.
Expand Down Expand Up @@ -288,8 +303,22 @@ impl<T: InternalVertexInfo + super::sealed::__Sealed> VertexInfo for T {
// Early-return in case there are no filters that apply here.
let first_filter = relevant_filters.first()?;

// We currently ignore filters applied to transformed values of local properties.
// This is sound because each filter we consider only *narrows* the set of possible values,
// meaning that non-evaluated filters are a lost optimization opportunity,
// not a correctness problem.
//
// If tweaking this logic, make sure to tweak the logic in `filters_on_local_property`
// as well -- their behaviors have to match!
//
// TODO: This is an optimization opportunity for the future.
let local_field = match first_filter.left() {
OperationSubject::LocalField(field) => field,
OperationSubject::TransformedField(_) => return None,
};

let initial_candidate = self.statically_required_property(property).unwrap_or_else(|| {
if first_filter.left().field_type.nullable() {
if local_field.field_type.nullable() {
CandidateValue::All
} else {
CandidateValue::Range(Range::full_non_null())
Expand Down Expand Up @@ -390,13 +419,27 @@ impl<T: InternalVertexInfo + super::sealed::__Sealed> VertexInfo for T {
fn filters_on_local_property<'a: 'b, 'b>(
vertex: &'a IRVertex,
property_name: &'b str,
) -> impl Iterator<Item = &'a Operation<LocalField, Argument>> + 'b {
vertex.filters.iter().filter(move |op| op.left().field_name.as_ref() == property_name)
) -> impl Iterator<Item = &'a Operation<OperationSubject, Argument>> + 'b {
vertex.filters.iter().filter(move |op| {
let left = op.left();
match left {
OperationSubject::LocalField(field) => field.field_name.as_ref() == property_name,
OperationSubject::TransformedField(..) => {
// We are currently ignoring field transformations for purposes of determining
// required property values. This is sound because every new filter we account for
// can only *reduce* the set of values that are allowed, so skipping filters
// means we lose out on optimization opportunities without harming correctness.
//
// TODO: This is an opportunity for further optimization.
false
}
}
})
}

fn compute_statically_known_candidate<'a, 'b>(
field: &'a LocalField,
relevant_filters: impl Iterator<Item = &'a Operation<LocalField, Argument>>,
relevant_filters: impl Iterator<Item = &'a Operation<OperationSubject, Argument>>,
query_variables: &'b BTreeMap<Arc<str>, FieldValue>,
) -> Option<CandidateValue<Cow<'b, FieldValue>>> {
let is_subject_field_nullable = field.field_type.nullable();
Expand Down Expand Up @@ -467,43 +510,43 @@ mod tests {
// Both `= 1` and `!= 1` are impossible to satisfy simultaneously.
(
vec![
Operation::NotEquals(local_field.clone(), first_var.clone()),
Operation::Equals(local_field.clone(), first_var.clone()),
Operation::NotEquals(local_field.clone().into(), first_var.clone()),
Operation::Equals(local_field.clone().into(), first_var.clone()),
],
Some(CandidateValue::Impossible),
),
// `= 2` and `!= 1` means the value must be 2.
(
vec![
Operation::NotEquals(local_field.clone(), first_var.clone()),
Operation::Equals(local_field.clone(), second_var.clone()),
Operation::NotEquals(local_field.clone().into(), first_var.clone()),
Operation::Equals(local_field.clone().into(), second_var.clone()),
],
Some(CandidateValue::Single(&variables["second"])),
),
//
// `one_of [1, 2]` and `!= 1` allows only `2`.
(
vec![
Operation::OneOf(local_field.clone(), list_var.clone()),
Operation::NotEquals(local_field.clone(), first_var.clone()),
Operation::OneOf(local_field.clone().into(), list_var.clone()),
Operation::NotEquals(local_field.clone().into(), first_var.clone()),
],
Some(CandidateValue::Single(&variables["second"])),
),
//
// `one_of [1, 2, 3]` and `not_one_of [1, 2]` allows only `3`.
(
vec![
Operation::OneOf(local_field.clone(), longer_list_var.clone()),
Operation::NotOneOf(local_field.clone(), list_var.clone()),
Operation::OneOf(local_field.clone().into(), longer_list_var.clone()),
Operation::NotOneOf(local_field.clone().into(), list_var.clone()),
],
Some(CandidateValue::Single(&variables["third"])),
),
//
// `>= 2` and `not_one_of [1, 2]` produces the exclusive > 2 range
(
vec![
Operation::GreaterThanOrEqual(local_field.clone(), second_var.clone()),
Operation::NotOneOf(local_field.clone(), list_var.clone()),
Operation::GreaterThanOrEqual(local_field.clone().into(), second_var.clone()),
Operation::NotOneOf(local_field.clone().into(), list_var.clone()),
],
Some(CandidateValue::Range(Range::with_start(
Bound::Excluded(&variables["second"]),
Expand All @@ -514,9 +557,9 @@ mod tests {
// `>= 2` and `is_not_null` and `not_one_of [1, 2]` produces the exclusive non-null > 2 range
(
vec![
Operation::GreaterThanOrEqual(local_field.clone(), second_var.clone()),
Operation::NotOneOf(local_field.clone(), list_var.clone()),
Operation::IsNotNull(local_field.clone()),
Operation::GreaterThanOrEqual(local_field.clone().into(), second_var.clone()),
Operation::NotOneOf(local_field.clone().into(), list_var.clone()),
Operation::IsNotNull(local_field.clone().into()),
],
Some(CandidateValue::Range(Range::with_start(
Bound::Excluded(&variables["second"]),
Expand All @@ -527,8 +570,8 @@ mod tests {
// `> 2` and `is_not_null` produces the exclusive non-null > 2 range
(
vec![
Operation::GreaterThan(local_field.clone(), second_var.clone()),
Operation::IsNotNull(local_field.clone()),
Operation::GreaterThan(local_field.clone().into(), second_var.clone()),
Operation::IsNotNull(local_field.clone().into()),
],
Some(CandidateValue::Range(Range::with_start(
Bound::Excluded(&variables["second"]),
Expand All @@ -539,9 +582,9 @@ mod tests {
// `<= 2` and `!= 2` and `is_not_null` produces the exclusive non-null < 2 range
(
vec![
Operation::LessThanOrEqual(local_field.clone(), second_var.clone()),
Operation::NotEquals(local_field.clone(), second_var.clone()),
Operation::IsNotNull(local_field.clone()),
Operation::LessThanOrEqual(local_field.clone().into(), second_var.clone()),
Operation::NotEquals(local_field.clone().into(), second_var.clone()),
Operation::IsNotNull(local_field.clone().into()),
],
Some(CandidateValue::Range(Range::with_end(
Bound::Excluded(&variables["second"]),
Expand All @@ -552,8 +595,8 @@ mod tests {
// `< 2` and `is_not_null` produces the exclusive non-null < 2 range
(
vec![
Operation::LessThan(local_field.clone(), second_var.clone()),
Operation::IsNotNull(local_field.clone()),
Operation::LessThan(local_field.clone().into(), second_var.clone()),
Operation::IsNotNull(local_field.clone().into()),
],
Some(CandidateValue::Range(Range::with_end(
Bound::Excluded(&variables["second"]),
Expand All @@ -563,21 +606,21 @@ mod tests {
//
// `is_not_null` by itself only eliminates null
(
vec![Operation::IsNotNull(local_field.clone())],
vec![Operation::IsNotNull(local_field.clone().into())],
Some(CandidateValue::Range(Range::full_non_null())),
),
//
// `!= null` also elminates null
(
vec![Operation::NotEquals(local_field.clone(), null_var.clone())],
vec![Operation::NotEquals(local_field.clone().into(), null_var.clone())],
Some(CandidateValue::Range(Range::full_non_null())),
),
//
// `!= 1` by itself doesn't produce any candidates
(vec![Operation::NotEquals(local_field.clone(), first_var.clone())], None),
(vec![Operation::NotEquals(local_field.clone().into(), first_var.clone())], None),
//
// `not_one_of [1, 2]` by itself doesn't produce any candidates
(vec![Operation::NotEquals(local_field.clone(), list_var.clone())], None),
(vec![Operation::NotEquals(local_field.clone().into(), list_var.clone())], None),
];

for (filters, expected_output) in test_data {
Expand Down Expand Up @@ -625,28 +668,28 @@ mod tests {
// The local field is non-nullable.
// When we apply a range bound on the field, the range must be non-nullable too.
(
vec![Operation::GreaterThanOrEqual(local_field.clone(), first_var.clone())],
vec![Operation::GreaterThanOrEqual(local_field.clone().into(), first_var.clone())],
Some(CandidateValue::Range(Range::with_start(
Bound::Included(&variables["first"]),
false,
))),
),
(
vec![Operation::GreaterThan(local_field.clone(), first_var.clone())],
vec![Operation::GreaterThan(local_field.clone().into(), first_var.clone())],
Some(CandidateValue::Range(Range::with_start(
Bound::Excluded(&variables["first"]),
false,
))),
),
(
vec![Operation::LessThan(local_field.clone(), first_var.clone())],
vec![Operation::LessThan(local_field.clone().into(), first_var.clone())],
Some(CandidateValue::Range(Range::with_end(
Bound::Excluded(&variables["first"]),
false,
))),
),
(
vec![Operation::LessThanOrEqual(local_field.clone(), first_var.clone())],
vec![Operation::LessThanOrEqual(local_field.clone().into(), first_var.clone())],
Some(CandidateValue::Range(Range::with_end(
Bound::Included(&variables["first"]),
false,
Expand Down
Loading

0 comments on commit be76adc

Please sign in to comment.