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

add isNull condition for payload filtering #1617

Merged
merged 17 commits into from
Apr 1, 2023
Merged

add isNull condition for payload filtering #1617

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
82d9f56
add minimal working is_null filter
ibrahim-akrab Mar 28, 2023
75decc4
add is_null condition to grpc api (backward compatible)
ibrahim-akrab Mar 28, 2023
d988d55
add unit tests is_null and is_empty conditions
ibrahim-akrab Mar 28, 2023
aafb6ef
add is_null to points.proto file
ibrahim-akrab Mar 28, 2023
de40037
add some failing OpenAPI tests
ibrahim-akrab Mar 29, 2023
c09a621
fix a failing test due to change in collection data
ibrahim-akrab Mar 29, 2023
47837c5
refactor MultiValue's check for is_null
ibrahim-akrab Mar 29, 2023
ce3feb8
fix is_empty condition not picking up "key":[]
ibrahim-akrab Mar 29, 2023
7ecbd64
remove duplicate OpenAPI integration test
ibrahim-akrab Mar 29, 2023
6c4baae
reuse same variable in condition checker tests
ibrahim-akrab Mar 29, 2023
ed0f3e9
update grpc docs
ibrahim-akrab Mar 30, 2023
19977b4
fix is_null cardinality estimation to match is_empty
ibrahim-akrab Mar 30, 2023
48b303e
update openapi specs
ibrahim-akrab Mar 30, 2023
2a6a828
remove unused debug statements
ibrahim-akrab Mar 30, 2023
4eef537
add new test points to original test_collection
ibrahim-akrab Mar 30, 2023
974ceb2
fix failing tests according to newly added points
ibrahim-akrab Mar 30, 2023
3bcac3b
add the `"key":[null]` test_case
ibrahim-akrab Mar 30, 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
Prev Previous commit
Next Next commit
add unit tests is_null and is_empty conditions
  • Loading branch information
@ibrahim-akrab
ibrahim-akrab committed Mar 28, 2023
commit d988d555a5802a5c33e89674c1b1401de9a4014c
13 changes: 9 additions & 4 deletions lib/segment/src/index/query_estimator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,12 @@ mod tests {
exp: TOTAL / 2,
max: TOTAL,
},
Condition::IsNull(condition) => CardinalityEstimation {
primary_clauses: vec![PrimaryCondition::IsNull(condition.to_owned())],
min: 0,
exp: TOTAL / 2,
max: TOTAL,
},
}
}

Expand Down Expand Up @@ -259,8 +265,7 @@ mod tests {
assert_eq!(estimation.primary_clauses.len(), 1);
match &estimation.primary_clauses[0] {
PrimaryCondition::Condition(field) => assert_eq!(&field.key, "size"),
PrimaryCondition::Ids(_) => panic!(),
PrimaryCondition::IsEmpty(_) => panic!(),
_ => panic!(),
}
assert!(estimation.max <= TOTAL);
assert!(estimation.exp <= estimation.max);
Expand Down Expand Up @@ -372,13 +377,13 @@ mod tests {
PrimaryCondition::Condition(field) => {
assert!(vec!["price".to_owned(), "size".to_owned(),].contains(&field.key))
}
PrimaryCondition::Ids(_) => panic!("Should not go here"),
PrimaryCondition::IsEmpty(_) => panic!("Should not go here"),
_ => panic!("Should not go here"),
});
assert!(estimation.max <= TOTAL);
assert!(estimation.exp <= estimation.max);
assert!(estimation.min <= estimation.exp);
}

#[test]
fn test_combine_must_estimations() {
let estimations = vec![CardinalityEstimation {
Expand Down
38 changes: 38 additions & 0 deletions lib/segment/src/payload_storage/query_checker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,8 @@ mod tests {
"rating": vec![3, 7, 9, 9],
"color": "red",
"has_delivery": true,
"parts": [],
"packaging": null
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to add a test for a field that is [null]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is the filtering supposed to behave in that case?

Copy link
Member

Choose a reason for hiding this comment

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

I guess [null] is equivalent to an empty array, so not null.
WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the current implementation, it will not be matched with neither is_empty nor is_null.
If you want it to be matched with is_empty that's fine. However, what about [null, [null]], is that even a valid payload? if so, should it be equivalent to empty as well?

Copy link
Contributor Author

@ibrahim-akrab ibrahim-akrab Mar 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Another concern , wouldn't it be confusing for "key":[null] to get matched as empty and not as null with the word "null" in it? I don't see it making sense in the documentation 😕

Copy link
Member

Choose a reason for hiding this comment

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

Forget about my comment, I don't think it is necessary to test this.
Sorry for derailing your research.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's alright, I added it anyway to test for any future changes regarding this case.

})
.into();

Expand Down Expand Up @@ -275,8 +277,44 @@ mod tests {
},
}));

let is_empty_condition_3 = Filter::new_must(Condition::IsEmpty(IsEmptyCondition {
is_empty: PayloadField {
key: "packaging".to_string(),
},
}));

assert!(!payload_checker.check(0, &is_empty_condition_1));
assert!(payload_checker.check(0, &is_empty_condition_2));
assert!(payload_checker.check(0, &is_empty_condition_3));

let is_null_condition_1 = Filter::new_must(Condition::IsNull(IsNullCondition {
ibrahim-akrab marked this conversation as resolved.
Show resolved Hide resolved
is_null: PayloadField {
key: "amount".to_string(),
},
}));

let is_null_condition_2 = Filter::new_must(Condition::IsNull(IsNullCondition {
is_null: PayloadField {
key: "parts".to_string(),
},
}));

let is_null_condition_3 = Filter::new_must(Condition::IsNull(IsNullCondition {
is_null: PayloadField {
key: "something_else".to_string(),
},
}));

let is_null_condition_4 = Filter::new_must(Condition::IsNull(IsNullCondition {
is_null: PayloadField {
key: "packaging".to_string(),
},
}));

assert!(!payload_checker.check(0, &is_null_condition_1));
assert!(!payload_checker.check(0, &is_null_condition_2));
assert!(!payload_checker.check(0, &is_null_condition_3));
assert!(payload_checker.check(0, &is_null_condition_4));

let match_red = Condition::Field(FieldCondition::new_match(
"color".to_string(),
Expand Down
54 changes: 54 additions & 0 deletions lib/segment/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1533,6 +1533,60 @@ mod tests {
);
}

#[test]
fn test_parse_empty_query() {
let query = r#"
{
"should": [
{
"is_empty" : {
"key" : "Jason"
}
}
]
}
"#;

let filter: Filter = serde_json::from_str(query).unwrap();
println!("{filter:?}");
ibrahim-akrab marked this conversation as resolved.
Show resolved Hide resolved
let should = filter.should.unwrap();

assert_eq!(should.len(), 1);
let c = match should.get(0) {
Some(Condition::IsEmpty(c)) => c,
_ => panic!("Condition::IsEmpty expected"),
};

assert_eq!(c.is_empty.key.as_str(), "Jason");
}

#[test]
fn test_parse_null_query() {
let query = r#"
{
"should": [
{
"is_null" : {
"key" : "Jason"
}
}
]
}
"#;

let filter: Filter = serde_json::from_str(query).unwrap();
println!("{filter:?}");
ibrahim-akrab marked this conversation as resolved.
Show resolved Hide resolved
let should = filter.should.unwrap();

assert_eq!(should.len(), 1);
let c = match should.get(0) {
Some(Condition::IsNull(c)) => c,
_ => panic!("Condition::IsNull expected"),
};

assert_eq!(c.is_null.key.as_str(), "Jason");
}

#[test]
fn test_payload_query_parse() {
let query1 = r#"
Expand Down