[SQL-3301] Support Binary Comparisons in Match Language - #172
[SQL-3301] Support Binary Comparisons in Match Language#172jpowell-mongo wants to merge 20 commits into
Conversation
| fn visit_stage(&mut self, node: Stage) -> Stage { | ||
| let node = node.walk(self); | ||
| match node { | ||
| Stage::MqlIntrinsic(MqlStage::MatchFilter(f)) => { |
There was a problem hiding this comment.
Note:
Although this looks like net new code, this logic is a near copy of the Stage::Filter(f) => match *f.source match arm, but with small adjustments to work for MqlStage::MatchFilter instead.
| /// `IXSCAN`, `IDHACK`, and the `EXPRESS_*` family all read through an index, so | ||
| /// they map to `IxScan`. `IDHACK` is the server's fast path for a sole `_id` | ||
| /// equality predicate, and `EXPRESS_IXSCAN` (MongoDB 8.0+) is the equivalent | ||
| /// fast path for other single-field index lookups; both use an index. |
There was a problem hiding this comment.
[praise] excellent find, thank you for expanding and documenting
| // When true, a FieldPath whose key is absent from theta is left unchanged (a no-op success) | ||
| // rather than failing. This mirrors how `visit_expression` treats an unbound `Reference`, and | ||
| // lets a MatchFilter bubble past stages with an empty theta (Unwind, Derived) the way an | ||
| // `$expr` Filter already does. It is left false for Sort, whose substitution must keep failing | ||
| // on a missing key: that failure is what prevents a Sort from reordering past a Filter or | ||
| // MatchFilter (which would otherwise loop forever, since `is_filter_filter_only_change` does | ||
| // not exempt Sort swaps). |
There was a problem hiding this comment.
[praise] excellent comment
[question] "... since is_filter_filter_only_change does not exempt Sort swaps)." Should we update how that is handled to prevent Sort stages from being swapped above Filter stages (of any variety) so that we don't need to depend on this?
| // leaf is a `<field> = <literal>` comparison: | ||
| // | ||
| // SELECT ... | ||
| // FROM lineitem, nation n1, nation n2 |
There was a problem hiding this comment.
| // FROM lineitem, nation n1, nation n2 | |
| // FROM nation n1, nation n2 |
It looks like lineitem is not actually used in this test, so having it here in the comment is a bit confusing. Did you mean to include it in the test, or should we remove it from the comment?
There was a problem hiding this comment.
Comment still stands.
| /// When this returns `false`, the expression is left to be translated to | ||
| /// `$expr` language so the existing `$getField`-based translation handles | ||
| /// the field correctly. | ||
| fn is_match_addressable(field_path: &FieldPath) -> bool { |
There was a problem hiding this comment.
Note: Consider moving this to the try_into for field patch to field access. Moving it down will make this cleaner and catch bugs elsewhere.
There was a problem hiding this comment.
After investigation, there wasn't an easy way to merge this into the try_into on its own because the usage of try_into doesn't perfect match the is_match_addressable usage across the codebase. I'm going to leave this here for now.
|
Running a patch to see how TPCH performance is with these changes: |
| } | ||
| } | ||
|
|
||
| // Native match language ($match find syntax) cannot reference correlated `let` |
There was a problem hiding this comment.
Note: After discussion the general case of correlated subqueries suggests that we may not be able to rewrite this to match-language without some performance degradation. This isn't really a question about using the same datasource and moreso about correlated subqueries.
We may want to consider a rewrite_to_expr_lang for quereis with correlated subqueries.
There was a problem hiding this comment.
Follow up:
Based on Evergreen patches, there is no performance impact (based on our tests) of special casing correlated subqueries by attempting to keep them above joins. Before merging, I'll raise an investigation ticket for correlated subqueries as those are the queries (TPCH-19) causing the tpch dataset to fail.
The null guard was removed from rewrite_comparison in 523953b, but the surrounding prose was left describing the guarded behavior. Several comments asserted guards that their own assertions disproved, which made the actual behavior hard to establish from reading the code. - rewrite_comparison: replace the "Null / missing correctness" section with a per-operator account of MQL type bracketing, and note that this pass is the only place these semantics can be set (MatchNullFiltering runs later and only visits Stage::Filter). Correct the Returns section. - Remove the dead branch whose arms both returned the same comparison, and the field_path clone it required. - Fix four test comments claiming guards, and one that misstated the operators under test. - extract_comparison: describe what it actually does with an And rather than a guard shape that is no longer emitted. - filter.yml: correct the OR pipeline shown in a comment. The IX_SCAN expectation was already right. No behavior change; 3137 tests pass.
- stage_movement: give the LateralJoin LHS schema unwrap an expect message. The surrounding code uses bare unwraps, but this one is new, so name the invariant it relies on. - use_def_analysis: unwrap the bare block left behind when visit_field_path was converted to let/else. Pure reindent. No behavior change; 3137 tests pass.
mattChiaravalloti
left a comment
There was a problem hiding this comment.
This looks really great. Very exciting change! I have a few change requests, but they're mostly related to comments and test coverage -- very few, if any, have to do with functionality.
There was a problem hiding this comment.
[praise/chore] The three noop tests are great. Can we also add a test for when a MatchFilter does successfully result in a pre-filter for an Unwind source?
| /// MQL comparisons are *type-bracketed* — they only match values in the same | ||
| /// BSON type class as their argument — which gives that behavior for free | ||
| /// for most operators: |
There was a problem hiding this comment.
[nit] It is implied by this comment but we may want to state explicitly that type-bracketing exclusively applies to match language, not expr language!
Enterprise temp> db.foo.insertMany([{_id: 0, a: null}, {_id: 1}, {_id: 2, a: 1}, {_id: 3, a: 5}, {_id: 4, a: 2}])
{
acknowledged: true,
insertedIds: { '0': 0, '1': 1, '2': 2, '3': 3, '4': 4 }
}
Enterprise temp> db.foo.aggregate([{$match: {$expr: {$lt: ["$a", 5]}}}])
[ { _id: 0, a: null }, { _id: 1 }, { _id: 2, a: 1 }, { _id: 4, a: 2 } ]
Enterprise temp> db.foo.aggregate([{$match: {a: {$lt: 5}}}])
[ { _id: 2, a: 1 }, { _id: 4, a: 2 } ]
| /// - `$lt`, `$lte`, `$gt`, `$gte` never match null or missing, so they are | ||
| /// emitted as bare comparisons. |
There was a problem hiding this comment.
[issue] Is this true? What about this example:
Enterprise temp> db.foo.insertMany([{_id: 0, a: null}, {_id: 1}, {_id: 2, a: 1}, {_id: 3, a: 5}, {_id: 4, a: 2}])
{
acknowledged: true,
insertedIds: { '0': 0, '1': 1, '2': 2, '3': 3, '4': 4 }
}
Enterprise temp> db.foo.aggregate([{$match: {a: {$lte: null}}}])
[ { _id: 0, a: null }, { _id: 1 } ]
null is equal to null, and missing is less than null, so these 2 documents pass this filter. In SQL, though, we should expect an empty result set since comparing to a null value results in null which is equivalent to false. I believe this applies to $lte and $gte since they involve and equality check that can cross type boundaries.
The comment below implies that we only consider literal null values problematic for $eq and $ne.
| /// covered as well as `a <> 10`. | ||
| /// | ||
| /// Note that this pass is the only place these semantics can be adjusted. | ||
| /// `MatchNullFilteringOptimizer`, which inserts `{field: {$gt: null}}` |
There was a problem hiding this comment.
[issue] MatchNullFilteringOptimizer creates an expr-language Filter stage that does {$gt: [field, null]}, which is different than a match-language MatchFilter stage that does {field: {$gt: null}}.
Example:
Enterprise temp> db.foo.insertMany([{_id: 0, a: null}, {_id: 1}, {_id: 2, a: 1}, {_id: 3, a: 5}, {_id: 4, a: 2}])
{
acknowledged: true,
insertedIds: { '0': 0, '1': 1, '2': 2, '3': 3, '4': 4 }
}
Enterprise temp> db.foo.aggregate([{$match: {a: {$gt: null}}}])
Enterprise temp> db.foo.aggregate([{$match: {a: {$ne: null}}}])
[ { _id: 2, a: 1 }, { _id: 3, a: 5 }, { _id: 4, a: 2 } ]
Enterprise temp> db.foo.aggregate([{$match: {$expr: {$gt: ["$a", null]}}}])
[ { _id: 2, a: 1 }, { _id: 3, a: 5 }, { _id: 4, a: 2 } ]
Enterprise temp> db.foo.aggregate([{$match: {$expr: {$ne: ["$a", null]}}}])
[ { _id: 1 }, { _id: 2, a: 1 }, { _id: 3, a: 5 }, { _id: 4, a: 2 } ]
This plays into my previous comment about how we handle null literals in this optimization.
We should update this comment to make the behavior distinction between $expr and match (find) language in $match stages. The implementation is correct, since it rejects any comparison operator that has a null literal operand, but even the comment above that conditional says it is only relevant for $eq and $ne.
| // MQL cannot express a SQL comparison against NULL: `$eq: null` matches | ||
| // null and missing, `$ne: null` matches every present non-null value, | ||
| // while SQL evaluates both to UNKNOWN for every row. Leave it in $expr. |
There was a problem hiding this comment.
[issue/follow-up] This is the in-line comment I was talking about above! This conditional is correct and we should keep it applied to all comparison operators. What we should update is this comment to mention it is relevant for all other non-$eq/$ne because of type-bracketing.
| args: vec![null_guard, comparison], | ||
| cache: SchemaCache::new(), | ||
| })) | ||
| } |
There was a problem hiding this comment.
[praise] the implementation of this function is beautiful!
| ))) | ||
| ); | ||
|
|
||
| // A NULL literal operand has no faithful match language equivalent — `$eq: null` |
There was a problem hiding this comment.
[chore] We'll want to add additional tests for the other comparison operators to show they also do not get rewritten when they have a literal null operand
| // A delimited identifier like `$a.b` is a single field whose name literally | ||
| // contains a `$` and a `.`. The FieldAccess -> FieldPath conversion must | ||
| // preserve it as one path component (not split on `.`, not drop the `$a`). | ||
| #[test] | ||
| fn field_access_with_dollar_and_dot_converts_to_single_component_path() { | ||
| let fa = match *mir_field_access("foo", "$a.b", true) { | ||
| Expression::FieldAccess(fa) => fa, | ||
| _ => unreachable!(), | ||
| }; | ||
| let fp: FieldPath = fa.try_into().unwrap(); | ||
| assert_eq!(fp, mir_field_path("foo", vec!["$a.b"])); | ||
| // Assert on `fields` directly: FieldPath's PartialEq ignores is_nullable | ||
| // and we want the component boundary to be explicit here. | ||
| assert_eq!(fp.fields, vec!["$a.b".to_string()]); | ||
| } |
There was a problem hiding this comment.
[chore] This test may be better placed in mir/definitions.rs since it is testing impl TryFrom<FieldAccess> for FieldPath which is implemented there. This test is not strictly related to this optimization so it is out of place in this test file.
| let left_schema = n | ||
| .source | ||
| .schema(self.schema_state) | ||
| .expect("LateralJoin LHS schema must be inferable"); |
There was a problem hiding this comment.
[chore] As of #180, left_schema is already computed earlier in this match branch, so you'll have access to that variable! When you rebase, you'll see it.
| // leaf is a `<field> = <literal>` comparison: | ||
| // | ||
| // SELECT ... | ||
| // FROM lineitem, nation n1, nation n2 |
There was a problem hiding this comment.
Comment still stands.
Summary
Extends the rewrite_to_match_language MIR optimizer to rewrite SQL comparison operators (<, <=, <>, =, >, >=) into native MQL match language (find syntax) instead of leaving them in $expr. These changes encourage index usage for more statements and will likely improve index utilization for customers.
Key Changes
Rewrite To Match Language Optimizer
Stage Movement
Use Def Analysis
noop_on_missing_keyto allow MatchFilter stages to bubble up above Unwind stages, which has an empty theta (substitution map)Tests