[SQL-2712] Update IN translation to use $in by jpowell-mongo · Pull Request #144 · mongodb/mongosql

jpowell-mongo · 2026-04-28T19:59:42Z

Summary

Note: Tests contribute to most of the line count, implementation code is much smaller.

This change updates mongosql to preserve the IN operator in SQL queries rather than translate them into a disjunction of ORs. Index Utilization is preserved.

Before, our rewriters would translate the query to a disjunction of ORs:

-- original query
SELECT a IN (b, c, d)
-- query after rewrites
SELECT a = b OR a = c OR a = d

Now, the code will preserve the IN operator, and later translate the code to the corresponding mql operator:

-- original query
SELECT a IN (b, c, d)
-- query after rewrites
SELECT a IN (b, c, d)

Along with these changes, I've made changes throughout the rest of the code, to allow IN / NOT IN to be translated to $in and { "$not": { "$in": [...] } } expressions in match stage aggregation pipelines, and use $in and $nin in $expr expressions.

./target/debug/mongosql-cli "SELECT * FROM nullable_fields WHERE a IN (100, 250, 322)"

Will produce this aggregation pipeline:

[
    { "$match": { "a": { "$in": [100, 250, 322] } } },
    { "$project": { "nullable_fields": "$$ROOT", "_id": 0 } },
]

Changes

Introduce In and NotIn enums to mir, air, ast, and codegen codepaths
Introduce a Tuple enum type throughut mir, air, and codegen. It operates the same as the Array type but I felt like it made it clearer to understand the cases.
Remove the InTupleRewriteVisitorPass, update the SingleTupleRewriter with a special case to not unwrap single element IN expressions (ex: SELECT * FROM orders where status IN ('pending'))
General threading of the new In, NotIn, and Tuple logic through the codebase

Testing

Unit Tests added at each step of the compiler, mir, air, codegen.
Index Utilization test to validate index usage still works.
Validated that the SQL from related Looker ticket works as expected.
Manual testing to validate $in expressions are used.

…QL-2712-in-operator

mattChiaravalloti

I made it through everything except translator and codegen. I'm getting nervous I'll lose these big comments at this point so I'm submitting this partial review now. I'll follow up with thoughts on translator and codegen. This is a very large change! Nice job getting to all of the areas that need to be addressed. There is more work to do to get this generally correct but we're on a good path here.

mattChiaravalloti · 2026-05-18T14:18:58Z

    current_db: index_usage_filter
    query: "SELECT * FROM nullable_fields WHERE a > 100 AND b IN (SELECT b FROM non_nullable_fields WHERE a < 300)"
    expected_utilization: IX_SCAN
+  - description: in operator uses the index


Suggested change

- description: in operator uses the index

- description: in operator uses the index

[nit] All other tests in this file have a newline between them, so I suggest adding one here as well

mattChiaravalloti · 2026-05-18T14:21:56Z

[Questions/Thoughts] Should we also add any of the following tests?

NOT IN for nullable_fields should use an IX_SCAN

IN for non_nullable_fields should use an IX_SCAN I think... you can check that against mongodb manually and then add the test if you observe index utilization is possible in this case (I suspect it is)

NOT IN for non_nullable_fields should use an IX_SCAN (again, check this and if it does then we should add the test so we don't regress on this support in the future)

mattChiaravalloti · 2026-05-18T14:36:15Z

+            let op = BinaryOp::arbitrary(g);
+            // The parser always produces a Tuple on the RHS of IN/NOT IN, so arbitrary
+            // generation must match that shape to satisfy the round-trip property.
+            let right = if op == BinaryOp::In || op == BinaryOp::NotIn {


[Praise] Smart!

mattChiaravalloti · 2026-05-18T14:37:16Z


 #[cfg(test)]
 mod test;
-


[Question] Is there a reason you got rid of this newline?

mattChiaravalloti · 2026-05-18T15:46:43Z

    }),
 );

+test_algebrize!(


[Question] Is this the same test as the one from binary.rs? Does it need to live in both places? I think the test file names correspond to input types, not output types, so this test is better placed in binary.rs. We can likely remove this here.

Question still stands. My suggestion is that we remove this test; this PR should not need to make changes to this file.

mattChiaravalloti · 2026-05-18T15:59:17Z

+    /// Rewrites `IN` and `NOT IN` scalar functions to native match language when the
+    /// left-hand side is a field access and every value in the list is a literal.
+    ///
+    /// Any other shape (subqueries, field refs in the list, functions) falls through
+    /// to `None` so the expression stays in `ExprLanguage`.


[Praise] Nice! Good thinking here, and the tests for this look great too.

mattChiaravalloti

Rest of my thoughts -- this time focused on translator and codegen (and, implicitly, air::desugarer).

mattChiaravalloti · 2026-05-18T16:47:01Z

+        };
+
+        let expression = match in_op.input {
+            None => return Err(Error::InvalidMatchLanguageInputRef),


[Suggestion] Why would this ever be None? Can we update mir::MatchLanguageIn.input to be FieldPath instead of an Option<FieldPath>? Looking at the optimization that creates mir::MatchLanguageIn, there doesn't seem to be a need to make it an option since it is always set to Some(field_path).

mattChiaravalloti · 2026-05-18T16:50:43Z

+pub struct
+    FieldRef {


Suggested change

pub struct

FieldRef {

pub struct FieldRef {

[nit/suggestion/question] Why the newline?

mattChiaravalloti · 2026-05-18T17:04:10Z

+            // $nin is match-language-only and rejected by the server in $project; NotIn must
+            // emit { "$not": [{ "$in": [...] }] } via SqlSemanticOperator codegen.
+            In => ScalarFunctionType::Sql(SqlOperator::In),
+            NotIn => ScalarFunctionType::Sql(SqlOperator::NotIn),


[Thought] (blocking) I'm thinking through how algrebrization, translation, and codegen will work for these operators. It seems that we may still have additional work to do in the algebrization step which will impact these other steps.

In particular, we need to determine if the algebrized expression is nullable -- right now, we're doing that the way every other binary op does it but that's wrong! Since IN/NOT IN deal with an expr and an array of exprs, we determine nullability by either the LHS being nullable or any element of the RHS being nullable, right? nullable_a IN (1, 2, 3) is possibly null, non_nullable_a IN (1, 2, NULL, 3) is possibly null, non_nullable_a IN (1, 2, 3) is never null, and nullable_a IN (1, 2, NULL, 3) is possibly null. We'll need to update algebrization to account for this. Then, when we're doing translation here, we'll follow the correct path to translate these as SQL semantic or MQL semantic operators (if the scalar function is not nullable, we can translate to the MQL version, if it is nullable we need to translate to the SQL version).

On top of that, we may need to deal with the SQL version in the desugarer. Specifically, a IN (1, 2, NULL, 3) should be true if a is 1, 2, or 3, or it should be null if it itself is null or if it is not 1, 2, or 3 (because the result of the third comparison is null). However, as currently implemented we will just codegen {$in: ["$a", [1, 2, null, 3]]} and not get the expected null result when we should. The desugarer could handle this as part of the sql_null_semantics_operators pass.

Ultimately, the reason I'm commenting this here is because I think we'll need to update this translation util function to return MqlOperator::In/MqlOperator::NotIn in the even the function itself is not nullable.

I updated the algebrizer and parsing logic here:
6d16cde

I'll make the desugarer change in another PR

mattChiaravalloti · 2026-05-18T17:05:42Z

    );
 }
+
+mod in_operator {


[Chore] We'll need to add tests for when the functions are nullable (is_nullable: true) -- these tests only handle when it that value is false.

mattChiaravalloti · 2026-05-18T17:08:16Z

+        translator::{self, test::match_query::mir_field_input},
+    };
+
+    fn make_translator() -> translator::MqlTranslator {


[Question] This helper function, along with the test functions themselves, look identical to the implementation of the test_translate_match_query macro. Is there any difference between the two? Is there a reason you didn't use the macro?

mattChiaravalloti · 2026-05-18T19:07:22Z

One last idea is that we may want to update the semantics.md doc to reflect these changes. It should no longer be considered part of the spec that X IN Y is rewritten to X = ANY(Y), etc. This section in particular covers this. We may also want to add some spec query tests to demonstrate how IN and NOT IN work.

…ype conversion

mattChiaravalloti

Marking as approved even though I am leaving a ton of comments. I just don't want to block this from merging while I'm gone.

There is still work to be done even though I marked as approved:

all outstanding open comment threads should either result in code changes or have answers to them that explain why not (I resolved the ones that are addressed in one way or another)
- A lot of the threads are still just some bookkeeping changes like fixing test names and/or inputs, addressing odd newlines, etc.
still need to add the spec/query tests as discussed offline
still need to update the desugarer before this can merge since it won't behave correctly until we do (we'll see the spec/query tests fail without this work done)

Feel free to tag someone else in in addition to Nick to give this another thorough pass if you want!

mattChiaravalloti · 2026-05-22T20:40:25Z

    }),
 );

+test_algebrize!(


Question still stands. My suggestion is that we remove this test; this PR should not need to make changes to this file.

mattChiaravalloti · 2026-05-22T20:42:05Z

+        env = map! {
+            ("foo", 1u16).into() => Schema::Document( Document {
+                keys: map! {
+                    "a".into() => Schema::Atomic(Atomic::String),


Suggested change

"a".into() => Schema::Atomic(Atomic::String),

"a".into() => Schema::Atomic(Atomic::Integer),

We still need to update this or else it will ultimately fail when schema checking is implemented correctly.

mattChiaravalloti · 2026-05-22T20:42:33Z

+        env = map! {
+            ("foo", 1u16).into() => Schema::Document( Document {
+                keys: map! {
+                    "a".into() => Schema::Atomic(Atomic::String),


Suggested change

"a".into() => Schema::Atomic(Atomic::String),

"a".into() => Schema::Atomic(Atomic::Integer),

Same issue applies here.

mattChiaravalloti · 2026-05-22T20:43:19Z

+        env = map! {
+            ("foo", 1u16).into() => Schema::Document( Document {
+                keys: map! {
+                    "a".into() => Schema::Atomic(Atomic::String),


Suggested change

"a".into() => Schema::Atomic(Atomic::String),

"a".into() => Schema::Atomic(Atomic::Boolean),

Same issue applies here as well.

mattChiaravalloti · 2026-05-22T20:44:09Z

+        env = map! {
+            ("foo", 1u16).into() => Schema::Document( Document {
+                keys: map! {
+                    "a".into() => Schema::Atomic(Atomic::Integer),
+                },
+                required: set!{},
+                additional_properties: false,
+                ..Default::default()
+            }),
+        },


Suggested change

env = map! {

("foo", 1u16).into() => Schema::Document( Document {

keys: map! {

"a".into() => Schema::Atomic(Atomic::Integer),

},

required: set!{},

additional_properties: false,

..Default::default()

}),

},

Since this test uses all Literals, we don't even need to include an env.

mattChiaravalloti · 2026-05-22T21:10:31Z

@@ -1 +1 @@
 /// Optimizes IS and LIKE expressions such that they can be translated and


[Suggestion] We should update this comment a bit to reflect that this now supports IN/NOT IN as well.

mattChiaravalloti · 2026-05-22T21:13:05Z

+    use super::*;
+
+    test_schema!(
+        in_operator_requires_two_args,


Suggested change

in_operator_requires_two_args,

not_in_operator_requires_two_args,

[Question/Suggestion] Should we name these for in and not_in distinctly?

This question applies throughout this file but I'll only comment it once.

mattChiaravalloti · 2026-05-22T21:14:39Z

+    );
+
+    test_schema!(
+        basic_in_operator_schema_is_boolean,


Question still stands, thought there's a typo in my suggestion. Should be not_in_operator_schema_is_boolean.

mattChiaravalloti · 2026-05-22T21:17:43Z

+    /// Contains an `unreachable!()` branch for the non-array RHS match arm. This branch is
+    /// structurally unreachable: the preceding guard already returns
+    /// [`Error::SchemaChecking`] for any RHS that does not satisfy `Schema::Array(Any)`.
+    fn get_in_operator_comparison_schema(


[Praise] The implementation of the method is fantastic!

mattChiaravalloti · 2026-05-22T21:19:07Z

+    use crate::{air, mapping_registry::MqlMappingRegistryValue};
+
+    test_translate_expression!(
+        it_converts_to_mql_when_lhs_is_nullable,


[Question] (blocking) Shouldn't it be that if either side is nullable it translates to SQL? And when both are not nullable it translates to MQL?

It's really just the test name that is misleading, and the input that is invalid. If either operand is marked with is_nullable: true, the entire expression should also be marked with is_nullable: true. This test sets is_nullable: false for the overall In expression, but marks one of its operands, the LHS, with is_nullable: true.

Can you fix this test to be valid to demonstrate that when none of the args is nullable, the translation is MQL. And then a separate test that shows when either arg is nullable (and therefore the expression is nullable) it translates to SQL?

jpowell-mongo added 9 commits April 28, 2026 15:58

In-Progress: Introduce dedicated In and NotIn operators

ce810ea

Proliferate and everywhere

faca587

Fill in missing match cases

8738571

Merge branch 'main' of github.com:jpowell-mongo/mongosql-jp into jp/S…

de7e9f6

…QL-2712-in-operator

In-Progress add Tuple through the mir expression

619440c

Unit tests to validate transformation

d0a59ac

In Operator passed through the representatiosn

dafdb05

In Progress - and should use match langauge instead of expr

9141e8a

Ensure is used for queries instead of

69c8fa4

jpowell-mongo commented May 5, 2026

View reviewed changes

Comment thread mongosql/src/mir/definitions.rs

jpowell-mongo added 6 commits May 5, 2026 10:46

Drive-by test fix, clippy cleanup

03a7e20

Use FieldRef rather than expression for MatchLanguageIn

3dbf6b8

Unit tests modify existing tests

aed0d3a

Add special case to not unwrap single tuple in array

95d350b

Code cleanup and unit tests

825931d

Clippy warning

b821e45

jpowell-mongo commented May 7, 2026

View reviewed changes

Comment thread mongosql-cli/src/main.rs

jpowell-mongo added 2 commits May 7, 2026 13:18

cargo fmt

ec34d38

Fix pretty_print fuzz test failure

ed8da23

jpowell-mongo marked this pull request as ready for review May 7, 2026 21:20

jpowell-mongo requested a review from a team as a code owner May 7, 2026 21:20

jpowell-mongo requested a review from mattChiaravalloti May 7, 2026 21:20

Remove TODO

ffe9dba

jpowell-mongo requested a review from nicholascioli May 7, 2026 21:22

jpowell-mongo changed the title ~~[DRAFT] [WORK IN PROGRESS] Update IN translation to use $in~~ [SQL-2712] Update IN translation to use $in May 7, 2026

mattChiaravalloti requested changes May 18, 2026

View reviewed changes

jpowell-mongo added 2 commits May 19, 2026 11:31

Remove unused InTupleRewritePass

12daafc

Use Array type instead of dedicated Tuple type

d757059

jpowell-mongo added 7 commits May 21, 2026 10:18

Create new in_comparison_get_schema_function and unit tests

5bcaccc

Doc comments

8a8558c

Custom nullability check for In / Not In operator

24dd29d

Update algrebrization for In operators. Inspect tuples for implicit t…

6d16cde

…ype conversion

Clippy and failing unit tests

0d0806a

Clippy

2b39b14

Restore ANY_SCHEMA_ADDENDUM

d33d633

mattChiaravalloti approved these changes May 22, 2026

View reviewed changes

	- description: in operator uses the index

	- description: in operator uses the index

	"a".into() => Schema::Atomic(Atomic::String),
	"a".into() => Schema::Atomic(Atomic::Integer),

	"a".into() => Schema::Atomic(Atomic::String),
	"a".into() => Schema::Atomic(Atomic::Boolean),

		@@ -1 +1 @@
		/// Optimizes IS and LIKE expressions such that they can be translated and

	in_operator_requires_two_args,
	not_in_operator_requires_two_args,

Conversation

jpowell-mongo commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Note: Tests contribute to most of the line count, implementation code is much smaller.

Changes

Testing

Uh oh!

Uh oh!

Uh oh!

mattChiaravalloti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattChiaravalloti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattChiaravalloti commented May 18, 2026

Uh oh!

mattChiaravalloti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

jpowell-mongo commented Apr 28, 2026 •

edited

Loading