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.

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

Add negative filter tests #425

Merged
merged 23 commits into from
Aug 4, 2023
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
bd3dce1
add not_one_of test
u9g Aug 2, 2023
80f7445
add greater than test
u9g Aug 2, 2023
e0d9cb8
add greater than or equal to test
u9g Aug 2, 2023
db1a1ec
Ensure we properly read negative numbers from the filter argument
u9g Aug 2, 2023
3c596cc
add equal test
u9g Aug 2, 2023
8972b17
add not equal test
u9g Aug 2, 2023
cd3f06a
add less than test
u9g Aug 2, 2023
fd17237
add less than or equal to test
u9g Aug 2, 2023
f06b816
add one_of test
u9g Aug 2, 2023
84790fd
Add comment to each test that we added about why we use negative filter
u9g Aug 2, 2023
7df2358
ensure we safely pull number from fieldvalue in get_max_fold_count_limit
u9g Aug 3, 2023
065f4de
add test that filters for a count less than usize::max
u9g Aug 3, 2023
41e7afa
make the test do what it said it was going to do
u9g Aug 3, 2023
de347e0
improve comment across all new added tests
u9g Aug 3, 2023
cfebd35
fix docstring
u9g Aug 3, 2023
6412f9f
fix the rest of the test files for not_eq test after previous commit
u9g Aug 3, 2023
bf4aeec
make test query names more clear
u9g Aug 3, 2023
cd9bf2b
Merge branch 'add-negative-filter-tests' of https://github.com/u9g/tr…
u9g Aug 3, 2023
714890e
add comment about why we .min on Vec::with_capacity
u9g Aug 3, 2023
b28be22
fix output of fold_count_filter_one_of_with_negative_arg
u9g Aug 3, 2023
1645666
add better comments to usize_from_fv and panic if we dont get a finit…
u9g Aug 4, 2023
af9bbd9
improve usize_from_field_value doc comment again
u9g Aug 4, 2023
895eb70
Update trustfall_core/src/interpreter/execution.rs
u9g Aug 4, 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
36 changes: 32 additions & 4 deletions trustfall_core/src/interpreter/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,24 @@ fn construct_outputs<'query, AdapterT: Adapter<'query>>(
}))
}

/// takes a FieldValue and produces a usize clamped to 0 and usize::MAX
u9g marked this conversation as resolved.
Show resolved Hide resolved
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"))
obi1kenobi marked this conversation as resolved.
Show resolved Hide resolved
}
FieldValue::Float64(_)
| FieldValue::List(_)
| FieldValue::Enum(_)
| FieldValue::Boolean(_)
| FieldValue::String(_)
| FieldValue::Null => None,
u9g marked this conversation as resolved.
Show resolved Hide resolved
}
}

/// 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 +273,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 +291,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 @@ -298,7 +326,7 @@ fn collect_fold_elements<'query, Vertex: Clone + Debug + 'query>(
// 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);
let mut fold_elements = Vec::with_capacity((*max_fold_count_limit).min(16));
u9g marked this conversation as resolved.
Show resolved Hide resolved

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),
},
))
u9g marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
TestGraphQLQuery (
// We use a negative number as a filter to ensure that we properly handle the negative number case in get_max_fold_count_limit.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few issues here:

  • This line is too long. We don't currently have a linter set up, but please try to keep lines below 100 chars in all files to make sure they are readable.
  • The comment references implementation details that may change and that ultimately aren't important. The test ensures that comparisons with negative numbers on count work in general, and would catch any failures even if they aren't in get_max_fold_count_limit(). I wouldn't name that function or any other implementation details here.
  • get_max_fold_count_limit() is code and a function. Please try to always put code in backticks and have function names followed by () to show they are functions.

Copy link
Contributor Author

@u9g u9g Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spruced up the comments in the tests to look like this:

// Ensure that we properly handle negative numbers in a `not_one_of` filter on a folded edge's count.

I believe this is the longest one, and it's 101 characters including // so I think that's fine for now. I removed any mentions to internals in tests, good call.

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