Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix @tag on @fold-related field inside nonexistent @optional. #258

Merged
merged 3 commits into from
Apr 13, 2023
Merged

Fix @tag on @fold-related field inside nonexistent @optional. #258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fbd1da2
Fix `@tag` on `@fold`-related field inside nonexistent `@optional`.
obi1kenobi Apr 13, 2023
82c08ef
Update format in traces.
obi1kenobi Apr 13, 2023
a25aef1
Remove outdated TODO.
obi1kenobi Apr 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 80 additions & 76 deletions trustfall_core/src/interpreter/execution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,27 +392,17 @@ fn compute_fold<'query, AdapterT: Adapter<'query> + 'query>(
FieldRef::FoldSpecificField(fold_specific_field) => {
let cloned_field = imported_field.clone();
let fold_eid = fold_specific_field.fold_eid;
iterator =
Box::new(
compute_fold_specific_field(
fold_specific_field.fold_eid,
&fold_specific_field.kind,
iterator,
)
.map(move |mut ctx| {
// TODO: Check whether the tagged value is coming from an `@optional` scope
// that did not exist, in order to satisfy its filtering semantics.
// Right now this will produce wrong results on tagged
// specific fields inside an `@optional` scope that doesn't exist.
ctx.imported_tags.insert(
cloned_field.clone(),
TaggedValue::Some(ctx.values.pop().expect(
"fold-specific field computed and pushed onto the stack",
)),
);
ctx
}),
);
iterator = Box::new(
compute_fold_specific_field_with_separate_value(
fold_specific_field.fold_eid,
&fold_specific_field.kind,
iterator,
)
.map(move |(mut ctx, tagged_value)| {
ctx.imported_tags.insert(cloned_field.clone(), tagged_value);
ctx
}),
);
}
}
}
Expand Down Expand Up @@ -459,13 +449,17 @@ fn compute_fold<'query, AdapterT: Adapter<'query> + 'query>(
neighbor_contexts,
);

let fold_elements = match collect_fold_elements(computed_iterator, &max_fold_size) {
None => {
// We were able to discard this fold early.
return None;
}
Some(f) => f,
// Check whether this @fold is inside an @optional that doesn't exist.
// This is not the same as having *zero* elements: nonexistent != empty.
let fold_exists = context.vertices[&expanding_from_vid].is_some();
let fold_elements = if fold_exists {
// N.B.: Note the `?` at the end here!
// This lets us early-discard folds that failed a post-processing filter.
Some(collect_fold_elements(computed_iterator, &max_fold_size)?)
} else {
None
};

context
.folded_contexts
.insert_or_error(fold_eid, fold_elements)
Expand Down Expand Up @@ -502,45 +496,54 @@ fn compute_fold<'query, AdapterT: Adapter<'query> + 'query>(
let mut cloned_carrier = carrier.clone();
let fold_component = fold.component.clone();
let final_iterator = post_filtered_iterator.map(move |mut ctx| {
// If the @fold is inside an @optional that doesn't exist,
// its outputs should be `null` rather than empty lists (the usual for empty folds).
// Transformed outputs should also be `null` rather than their usual transformed defaults.
let did_fold_root_exist = ctx.vertices[&expanding_from_vid].is_some();
let default_value = if did_fold_root_exist {
Some(ValueOrVec::Vec(vec![]))
} else {
None
};

let fold_elements = ctx.folded_contexts.get(&fold_eid).unwrap();
let fold_elements = &ctx.folded_contexts[&fold_eid];
debug_assert_eq!(
// Two ways to check if the `@fold` is inside an `@optional` that didn't exist:
ctx.vertices[&expanding_from_vid].is_some(),
fold_elements.is_some(),
"\
mismatch on whether the fold below {expanding_from_vid:?} was inside an `@optional`: {ctx:?}",
);

// Add any fold-specific field outputs to the context's folded values.
for (output_name, fold_specific_field) in &fold.fold_specific_outputs {
let value = match fold_specific_field {
FoldSpecificFieldKind::Count => {
ValueOrVec::Value(FieldValue::Uint64(fold_elements.len() as u64))
}
};
// If the @fold is inside an @optional that doesn't exist,
// its outputs should be `null` rather than empty lists (the usual for empty folds).
// Transformed outputs should also be `null` rather than their usual transformed defaults.
let value = fold_elements
.as_ref()
.map(|elements| match fold_specific_field {
FoldSpecificFieldKind::Count => {
ValueOrVec::Value(FieldValue::Uint64(elements.len() as u64))
}
});
ctx.folded_values
.insert_or_error(
(fold_eid, output_name.clone()),
did_fold_root_exist.then_some(value),
)
.unwrap();
.insert_or_error((fold_eid, output_name.clone()), value)
.expect("this fold output was already computed");
}

// Prepare empty vectors for all the outputs from this @fold component.
// If the fold-root vertex didn't exist, the default is `null` instead.
let default_value = if fold_elements.is_some() {
Some(ValueOrVec::Vec(vec![]))
} else {
None
};
let mut folded_values: BTreeMap<(Eid, Arc<str>), Option<ValueOrVec>> = output_names
.iter()
.map(|output| ((fold_eid, output.clone()), default_value.clone()))
.collect();

// Don't bother trying to resolve property values on this @fold when it's empty.
// Skip the adapter resolve_property() calls and add the empty output values directly.
if fold_elements.is_empty() {
let fold_contains_elements = fold_elements
.as_ref()
.map(|e| !e.is_empty())
.unwrap_or(false);
if !fold_contains_elements {
// We need to make sure any outputs from any nested @fold components (recursively)
// are set to empty lists.
// are set to the default value (empty list if the @fold existed and was empty,
// null if it didn't exist because it was inside an @optional).
let mut queue: Vec<_> = fold_component.folds.values().collect();
while let Some(inner_fold) = queue.pop() {
for output in inner_fold.fold_specific_outputs.keys() {
Expand All @@ -553,9 +556,17 @@ fn compute_fold<'query, AdapterT: Adapter<'query> + 'query>(
}
} else {
// Iterate through the elements of the fold and get the values we need.
let mut output_iterator: ContextIterator<'query, AdapterT::Vertex> =
Box::new(fold_elements.clone().into_iter());
let mut output_iterator: ContextIterator<'query, AdapterT::Vertex> = Box::new(
fold_elements
.as_ref()
.expect("fold did not contain elements")
.clone()
.into_iter(),
);
for output_name in output_names.iter() {
// This is a slimmed-down version of computing a context field:
// - it does not restore the prior active vertex after getting each value
// - it already knows that the context field is guaranteed to exist
let context_field = &fold.component.outputs[output_name.as_ref()];
let vertex_id = context_field.vertex_id;
let moved_iterator = Box::new(output_iterator.map(move |context| {
Expand Down Expand Up @@ -662,7 +673,16 @@ fn apply_fold_specific_filter<'query, AdapterT: Adapter<'query>>(
iterator: ContextIterator<'query, AdapterT::Vertex>,
) -> ContextIterator<'query, AdapterT::Vertex> {
let fold_specific_field = filter.left();
let field_iterator = compute_fold_specific_field(fold.eid, fold_specific_field, iterator);
let field_iterator = Box::new(compute_fold_specific_field_with_separate_value(fold.eid, fold_specific_field, iterator).map(|(mut ctx, tagged_value)| {
let value = match tagged_value {
TaggedValue::Some(value) => value,
TaggedValue::NonexistentOptional => {
unreachable!("while applying fold-specific filter, the @fold turned out to not exist: {ctx:?}")
}
};
ctx.values.push(value);
ctx
}));

apply_filter(
adapter,
Expand Down Expand Up @@ -729,24 +749,6 @@ pub(super) fn compute_context_field_with_separate_value<'query, AdapterT: Adapte
}
}

pub(super) fn compute_fold_specific_field<'query, Vertex: Clone + Debug + 'query>(
fold_eid: Eid,
fold_specific_field: &FoldSpecificFieldKind,
iterator: ContextIterator<'query, Vertex>,
) -> ContextIterator<'query, Vertex> {
// TODO: this is broken, an `@output` on a fold-specific field (like Count)
// that is inside a non-existent @optional produces `null` not 0.
// Ensure output type inference handles this correctly too...
// TODO: use the compute_fold_specific_field_with_separate_value() fn here instead
match fold_specific_field {
FoldSpecificFieldKind::Count => Box::new(iterator.map(move |mut ctx| {
let value = ctx.folded_contexts[&fold_eid].len();
ctx.values.push(FieldValue::Uint64(value as u64));
ctx
})),
}
}

pub(super) fn compute_fold_specific_field_with_separate_value<
'query,
Vertex: Clone + Debug + 'query,
Expand All @@ -757,11 +759,13 @@ pub(super) fn compute_fold_specific_field_with_separate_value<
) -> ContextOutcomeIterator<'query, Vertex, TaggedValue> {
match fold_specific_field {
FoldSpecificFieldKind::Count => Box::new(iterator.map(move |ctx| {
// TODO: this is broken, an `@output` on a fold-specific field (like Count)
// that is inside a non-existent @optional produces `null` not 0.
// Ensure output type inference handles this correctly too...
let value = ctx.folded_contexts[&fold_eid].len();
(ctx, TaggedValue::Some(FieldValue::Uint64(value as u64)))
// TODO: Ensure output type inference handles this correctly too...
let folded_contexts = ctx.folded_contexts[&fold_eid].as_ref();
let value = match folded_contexts {
None => TaggedValue::NonexistentOptional,
Some(v) => TaggedValue::Some(FieldValue::Uint64(v.len() as u64)),
};
(ctx, value)
})),
}
}
Expand Down
5 changes: 0 additions & 5 deletions trustfall_core/src/interpreter/filtering.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,11 +323,6 @@ pub(super) fn apply_filter<'query, AdapterT: Adapter<'query>>(
}
Some(Argument::Tag(field_ref @ FieldRef::FoldSpecificField(fold_field))) => {
let argument_value_iterator = if component.folds.contains_key(&fold_field.fold_eid) {
// Check if the fold is guaranteed to exist or not -- it might be inside an @optional!
// TODO: ^^^^^

// This value comes from one of this component's folds:
// the @tag is a sibling to the current computation and needs to be materialized.
compute_fold_specific_field_with_separate_value(
fold_field.fold_eid,
&fold_field.kind,
Expand Down
33 changes: 20 additions & 13 deletions trustfall_core/src/interpreter/hints/dynamic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ use async_graphql_parser::types::Type;
use crate::{
interpreter::{
execution::{
compute_context_field_with_separate_value, compute_fold_specific_field, QueryCarrier,
compute_context_field_with_separate_value,
compute_fold_specific_field_with_separate_value, QueryCarrier,
},
hints::Range,
Adapter, ContextIterator, ContextOutcomeIterator, InterpretedQuery, TaggedValue,
Expand Down Expand Up @@ -42,12 +43,14 @@ macro_rules! compute_candidate_from_tagged_value {
};
}

macro_rules! resolve_fold_field {
macro_rules! resolve_fold_specific_field {
($iterator:ident, $initial_candidate:ident, $candidate:ident, $value:ident, $blk:block) => {
Box::new($iterator.map(move |mut ctx| {
let $value = ctx.values.pop().expect("no value in values() stack");
Box::new($iterator.map(move |(ctx, tagged_value)| {
let mut $candidate = $initial_candidate.clone();
$blk(ctx, $candidate)
if let TaggedValue::Some($value) = tagged_value {
$blk
}
(ctx, $candidate)
}))
};
}
Expand Down Expand Up @@ -165,46 +168,50 @@ impl<'a> DynamicallyResolvedValue<'a> {
fold_field: &'a FoldSpecificField,
contexts: ContextIterator<'vertex, VertexT>,
) -> ContextOutcomeIterator<'vertex, VertexT, CandidateValue<FieldValue>> {
let iterator = compute_fold_specific_field(fold_field.fold_eid, &fold_field.kind, contexts);
let iterator = compute_fold_specific_field_with_separate_value(
fold_field.fold_eid,
&fold_field.kind,
contexts,
);
let initial_candidate = self.initial_candidate;

match &self.operation {
Operation::Equals(_, _) => {
resolve_fold_field!(iterator, initial_candidate, candidate, value, {
resolve_fold_specific_field!(iterator, initial_candidate, candidate, value, {
candidate.intersect(CandidateValue::Single(value));
})
}
Operation::NotEquals(_, _) => {
resolve_fold_field!(iterator, initial_candidate, candidate, value, {
resolve_fold_specific_field!(iterator, initial_candidate, candidate, value, {
candidate.exclude_single_value(&value);
})
}
Operation::LessThan(_, _) => {
resolve_fold_field!(iterator, initial_candidate, candidate, value, {
resolve_fold_specific_field!(iterator, initial_candidate, candidate, value, {
candidate.intersect(CandidateValue::Range(Range::with_end(
Bound::Excluded(value),
false,
)));
})
}
Operation::LessThanOrEqual(_, _) => {
resolve_fold_field!(iterator, initial_candidate, candidate, value, {
resolve_fold_specific_field!(iterator, initial_candidate, candidate, value, {
candidate.intersect(CandidateValue::Range(Range::with_end(
Bound::Included(value),
false,
)));
})
}
Operation::GreaterThan(_, _) => {
resolve_fold_field!(iterator, initial_candidate, candidate, value, {
resolve_fold_specific_field!(iterator, initial_candidate, candidate, value, {
candidate.intersect(CandidateValue::Range(Range::with_start(
Bound::Excluded(value),
false,
)));
})
}
Operation::GreaterThanOrEqual(_, _) => {
resolve_fold_field!(iterator, initial_candidate, candidate, value, {
resolve_fold_specific_field!(iterator, initial_candidate, candidate, value, {
candidate.intersect(CandidateValue::Range(Range::with_end(
Bound::Included(value),
false,
Expand All @@ -213,7 +220,7 @@ impl<'a> DynamicallyResolvedValue<'a> {
}
Operation::OneOf(_, _) => {
let fold_field = fold_field.clone();
resolve_fold_field!(iterator, initial_candidate, candidate, value, {
resolve_fold_specific_field!(iterator, initial_candidate, candidate, value, {
let values = value
.as_slice()
.unwrap_or_else(|| {
Expand Down
4 changes: 2 additions & 2 deletions trustfall_core/src/interpreter/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ pub struct DataContext<Vertex: Clone + Debug> {
vertices: BTreeMap<Vid, Option<Vertex>>,
values: Vec<FieldValue>,
suspended_vertices: Vec<Option<Vertex>>,
folded_contexts: BTreeMap<Eid, Vec<DataContext<Vertex>>>,
folded_contexts: BTreeMap<Eid, Option<Vec<DataContext<Vertex>>>>,
folded_values: BTreeMap<(Eid, Arc<str>), Option<ValueOrVec>>,
piggyback: Option<Vec<DataContext<Vertex>>>,
imported_tags: BTreeMap<FieldRef, TaggedValue>,
Expand Down Expand Up @@ -141,7 +141,7 @@ where
suspended_vertices: Vec<Option<Vertex>>,

#[serde(default, skip_serializing_if = "BTreeMap::is_empty")]
folded_contexts: BTreeMap<Eid, Vec<DataContext<Vertex>>>,
folded_contexts: BTreeMap<Eid, Option<Vec<DataContext<Vertex>>>>,

#[serde(default, skip_serializing_if = "BTreeMap::is_empty")]
folded_values: BTreeMap<(Eid, Arc<str>), Option<ValueOrVec>>,
Expand Down
Loading