Skip to content

Commit

Permalink
Add negative filter tests (#425)
Browse files Browse the repository at this point in the history
* add not_one_of test

* add greater than test

* add greater than or equal to test

* Ensure we properly read negative numbers from the filter argument

* add equal test

* add not equal test

* add less than test

* add less than or equal to test

* add one_of test

* Add comment to each test that we added about why we use negative filter

* ensure we safely pull number from fieldvalue in get_max_fold_count_limit

* add test that filters for a count less than usize::max

* make the test do what it said it was going to do

* improve comment across all new added tests

* fix docstring

Co-authored-by: Predrag Gruevski <[email protected]>

* fix the rest of the test files for not_eq test after previous commit

* make test query names more clear

* add comment about why we .min on Vec::with_capacity

* fix output of fold_count_filter_one_of_with_negative_arg

* add better comments to usize_from_fv and panic if we dont get a finite number

* improve usize_from_field_value doc comment again

* Update trustfall_core/src/interpreter/execution.rs

Co-authored-by: Predrag Gruevski <[email protected]>

---------

Co-authored-by: Predrag Gruevski <[email protected]>
  • Loading branch information
u9g and obi1kenobi authored Aug 4, 2023
1 parent ab840bb commit 7c84689
Show file tree
Hide file tree
Showing 46 changed files with 6,212 additions and 5 deletions.
42 changes: 37 additions & 5 deletions trustfall_core/src/interpreter/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,27 @@ fn construct_outputs<'query, AdapterT: Adapter<'query>>(
}))
}

/// Extracts numeric [`FieldValue`] into a `usize`, clamping negative numbers to 0.
/// Returns `None` on `FieldValue::Null`, and panics otherwise.
fn usize_from_field_value(field_value: &FieldValue) -> Option<usize> {
match field_value {
FieldValue::Int64(num) => {
Some(usize::try_from(*num.max(&0)).expect("i64 can be converted to usize"))
}
FieldValue::Uint64(num) => {
Some(usize::try_from(*num).expect("i64 can be converted to usize"))
}
FieldValue::Null => None,
FieldValue::Float64(_)
| FieldValue::List(_)
| FieldValue::Enum(_)
| FieldValue::Boolean(_)
| FieldValue::String(_) => {
panic!("got field value {field_value:#?} in usize_from_field_value which should only ever get Int64, Uint64, or Null")
}
}
}

/// If this IRFold has a filter on the folded element count, and that filter imposes
/// a max size that can be statically determined, return that max size so it can
/// be used for further optimizations. Otherwise, return None.
Expand All @@ -255,11 +276,15 @@ fn get_max_fold_count_limit(carrier: &mut QueryCarrier, fold: &IRFold) -> Option
FoldSpecificFieldKind::Count,
Argument::Variable(var_ref),
) => {
let variable_value = query_arguments[&var_ref.variable_name].as_usize().unwrap();
let variable_value =
usize_from_field_value(&query_arguments[&var_ref.variable_name])
.expect("for field value to be coercible to usize");
Some(variable_value)
}
Operation::LessThan(FoldSpecificFieldKind::Count, Argument::Variable(var_ref)) => {
let variable_value = query_arguments[&var_ref.variable_name].as_usize().unwrap();
let variable_value =
usize_from_field_value(&query_arguments[&var_ref.variable_name])
.expect("for field value to be coercible to usize");
// saturating_sub() here is a safeguard against underflow: in principle,
// we shouldn't see a comparison for "< 0", but if we do regardless, we'd prefer to
// saturate to 0 rather than wrapping around. This check is an optimization and
Expand All @@ -269,7 +294,13 @@ fn get_max_fold_count_limit(carrier: &mut QueryCarrier, fold: &IRFold) -> Option
}
Operation::OneOf(FoldSpecificFieldKind::Count, Argument::Variable(var_ref)) => {
match &query_arguments[&var_ref.variable_name] {
FieldValue::List(v) => v.iter().map(|x| x.as_usize().unwrap()).max(),
FieldValue::List(v) => v
.iter()
.map(|x| {
usize_from_field_value(x)
.expect("for field value to be coercible to usize")
})
.max(),
_ => unreachable!(),
}
}
Expand Down Expand Up @@ -297,8 +328,9 @@ fn collect_fold_elements<'query, Vertex: Clone + Debug + 'query>(
// and as an optimization we'd like to stop pulling elements as soon as possible.
// If we are able to pull more than `max_fold_count_limit + 1` elements,
// we know that this fold is going to get filtered out, so we might as well
// stop materializing its elements early.
let mut fold_elements = Vec::with_capacity(*max_fold_count_limit);
// stop materializing its elements early. Limit the max allocation size since
// it might not always be fully used.
let mut fold_elements = Vec::with_capacity((*max_fold_count_limit).min(16));

let mut stopped_early = false;
for _ in 0..*max_fold_count_limit {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
Ok(TestParsedGraphQLQuery(
schema_name: "numbers",
query: Query(
root_connection: FieldConnection(
position: Pos(
line: 3,
column: 5,
),
name: "Number",
arguments: {
"max": Int64(6),
"min": Int64(4),
},
),
root_field: FieldNode(
position: Pos(
line: 3,
column: 5,
),
name: "Number",
coerced_to: Some("Composite"),
connections: [
(FieldConnection(
position: Pos(
line: 5,
column: 13,
),
name: "value",
), FieldNode(
position: Pos(
line: 5,
column: 13,
),
name: "value",
output: [
OutputDirective(),
],
)),
(FieldConnection(
position: Pos(
line: 7,
column: 13,
),
name: "primeFactor",
fold: Some(FoldGroup(
fold: FoldDirective(),
transform: Some(TransformGroup(
transform: TransformDirective(
kind: Count,
),
filter: [
FilterDirective(
operation: Equals((), VariableRef("neg_two")),
),
],
)),
)),
), FieldNode(
position: Pos(
line: 7,
column: 13,
),
name: "primeFactor",
connections: [
(FieldConnection(
position: Pos(
line: 8,
column: 17,
),
name: "value",
alias: Some("factors"),
), FieldNode(
position: Pos(
line: 8,
column: 17,
),
name: "value",
alias: Some("factors"),
output: [
OutputDirective(),
],
)),
],
transform_group: Some(TransformGroup(
transform: TransformDirective(
kind: Count,
),
filter: [
FilterDirective(
operation: Equals((), VariableRef("neg_two")),
),
],
)),
)),
],
),
),
arguments: {
"neg_two": Int64(-2),
},
))
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
TestGraphQLQuery (
// Ensure that we properly handle negative numbers in a `=` filter on a folded edge's count.
schema_name: "numbers",
query: r#"
{
Number(min: 4, max: 6) {
... on Composite {
value @output
primeFactor @fold @transform(op: "count") @filter(op: "=", value: ["$neg_two"]) {
factors: value @output
}
}
}
}"#,
arguments: {
"neg_two": Int64(-2),
},
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
Ok(TestIRQuery(
schema_name: "numbers",
ir_query: IRQuery(
root_name: "Number",
root_parameters: EdgeParameters(
contents: {
"max": Int64(6),
"min": Int64(4),
},
),
root_component: IRQueryComponent(
root: Vid(1),
vertices: {
Vid(1): IRVertex(
vid: Vid(1),
type_name: "Composite",
coerced_from_type: Some("Number"),
),
},
folds: {
Eid(1): IRFold(
eid: Eid(1),
from_vid: Vid(1),
to_vid: Vid(2),
edge_name: "primeFactor",
component: IRQueryComponent(
root: Vid(2),
vertices: {
Vid(2): IRVertex(
vid: Vid(2),
type_name: "Prime",
),
},
outputs: {
"factors": ContextField(
vertex_id: Vid(2),
field_name: "value",
field_type: "Int",
),
},
),
post_filters: [
Equals(Count, Variable(VariableRef(
variable_name: "neg_two",
variable_type: "Int!",
))),
],
),
},
outputs: {
"value": ContextField(
vertex_id: Vid(1),
field_name: "value",
field_type: "Int",
),
},
),
variables: {
"neg_two": "Int!",
},
),
arguments: {
"neg_two": Int64(-2),
},
))
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
TestInterpreterOutputData(
schema_name: "numbers",
outputs: {
"factors": Output(
name: "factors",
value_type: "[Int]!",
vid: Vid(2),
),
"value": Output(
name: "value",
value_type: "Int",
vid: Vid(1),
),
},
results: [],
)
Loading

0 comments on commit 7c84689

Please sign in to comment.