support more aggregation operators when generating native query configurations #120

hallettj · 2024-11-07T22:33:38Z

Adds or refines support for these operators:

Arithmetic Expression Operators

$abs
$add
$divide
$multiply
$subtract

Array Expression Operators

$arrayElemAt

Boolean Expression Operators

$and
$not
$or

Comparison Expression Operators

$eq
$gt
$gte
$lt
$lte
$ne

Set Expression Operators

$allElementsTrue
$anyElementTrue

String Expression Operators

$split

Trigonometry Expression Operators

$sin
$cos
$tan
$asin
$acos
$atan
$asinh
$acosh
$atanh
$sinh
$cosh
$tanh

Accumulators ($group, $bucket, $bucketAuto, $setWindowFields)

$avg
$count
$max
$min
$push
$sum

Also improves type inference to make all of these operators work.

This is work an an in-progress feature that is gated behind a feature flag, native-query-subcommand

hallettj · 2024-11-07T22:34:31Z

crates/cli/src/native_query/tests.rs

+#[googletest::test]
+fn supports_various_aggregation_operators() -> googletest::Result<()> {
+    let config = mflix_config();
+
+    let pipeline = Pipeline::new(vec![
+        Stage::Match(doc! {
+            "$expr": {
+                "$and": [
+                    { "$eq": ["{{ title }}", "$title"] },
+                    { "$or": [null, 1] },
+                    { "$not": "{{ bool_param }}" },
+                    { "$gt": ["$imdb.votes", "{{ votes }}"] },
+                ]
+            }
+        }),
+        Stage::ReplaceWith(Selection::new(doc! {
+            "abs": { "$abs": "$year" },
+            "add": { "$add": ["$tomatoes.viewer.rating", "{{ rating_inc }}"] },
+            "divide": { "$divide": ["$tomatoes.viewer.rating", "{{ rating_div }}"] },
+            "multiply": { "$multiply": ["$tomatoes.viewer.rating", "{{ rating_mult }}"] },
+            "subtract": { "$subtract": ["$tomatoes.viewer.rating", "{{ rating_sub }}"] },
+            "arrayElemAt": { "$arrayElemAt": ["$genres", "{{ idx }}"] },
+            "title_words": { "$split": ["$title", " "] }
+        })),
+    ]);
+
+    let native_query =
+        native_query_from_pipeline(&config, "operators_test", Some("movies".into()), pipeline)?;
+
+    expect_eq!(
+        native_query.arguments,
+        object_fields([
+            ("title", Type::Scalar(BsonScalarType::String)),
+            ("bool_param", Type::Scalar(BsonScalarType::Bool)),
+            ("votes", Type::Scalar(BsonScalarType::Int)),
+            ("rating_inc", Type::Scalar(BsonScalarType::Double)),
+            ("rating_div", Type::Scalar(BsonScalarType::Double)),
+            ("rating_mult", Type::Scalar(BsonScalarType::Double)),
+            ("rating_sub", Type::Scalar(BsonScalarType::Double)),
+            ("idx", Type::Scalar(BsonScalarType::Int)),
+        ])
+    );
+
+    let result_type = native_query.result_document_type;
+    expect_eq!(
+        native_query.object_types[&result_type],
+        ObjectType {
+            fields: object_fields([
+                ("abs", Type::Scalar(BsonScalarType::Int)),
+                ("add", Type::Scalar(BsonScalarType::Double)),
+                ("divide", Type::Scalar(BsonScalarType::Double)),
+                ("multiply", Type::Scalar(BsonScalarType::Double)),
+                ("subtract", Type::Scalar(BsonScalarType::Double)),
+                (
+                    "arrayElemAt",
+                    Type::Nullable(Box::new(Type::Scalar(BsonScalarType::String)))
+                ),
+                (
+                    "title_words",
+                    Type::ArrayOf(Box::new(Type::Scalar(BsonScalarType::String)))
+                ),
+            ]),
+            description: None,
+        }
+    );
+
+    Ok(())
+}


This test demonstrates the new functionality

danieljharvey · 2024-11-13T12:30:29Z

crates/cli/src/native_query/type_solver/simplify.rs

-    } else {
-        Err((C::Scalar(a), C::Scalar(b)))
+    match variance {
+        Variance::Covariant => {


What is the purpose of tracking the difference between Covariant and Contravariant? Seems like we try the subtyping check both ways regardless?

Oh you're right - I want to return the supertype in the covariant case, and the subtype in the contravariant case. But that's not actually how I wrote it is it?

danieljharvey · 2024-11-13T12:32:58Z

crates/cli/src/native_query/type_constraint.rs

    Predicate {
        object_type_name: ObjectTypeName,
    },

+    // Complex types
+    Union(BTreeSet<TypeConstraint>),
+    OneOf(BTreeSet<TypeConstraint>), // unlike union type should be only one of these, but we don't know which one


Could you provide a concrete example here in a comment perhaps? This may be entirely a reflection of my lack of understanding around Mongo's semantics, but I can't get my head around when this would be used.

That's a good idea, I'll add more details in a comment.

OneOf is helpful mostly for numeric inputs, where we're expecting a numeric type, but we don't know specifically which one. We probably want to infer a specific numeric type instead of a union of all numeric types, which would present as ExtendedJSON (the top type) in the API which isn't as helpful.

OTOH my thinking is Union is for cases where we have specific evidence a type variable is associated with multiple distinct concrete types. I'm mainly using Union to indicate nullability.

Another way to handle OneOf that could be less handwavy could be to use SubtypeOf(Union(...)). But I'm not sure exactly how the semantics of that would play out.

danieljharvey · 2024-11-13T12:35:27Z

crates/cli/src/native_query/type_solver/constraint_to_type.rs

+
+        C::Union(_) => Some(Type::ExtendedJSON),
+
+        t @ C::OneOf(_) if t.is_numeric() => {


Definitely not a thing for now, but I wonder how hard it would be to optionally include the user's argument types here to help guide us through ambiguity here. Choosing double seems absolutely acceptable for now though.

Yes, good point! Type annotations will show up as another constraint in the set of constraints for the type variable. So before we get to constraint_to_type we'll see a call to simplify_pair with OneOf(...) and Scalar(_) (or whatever concrete type is given by the annotation) which will simplify to the annotation type if that overlaps with one of the types in the OneOf set. If there is no overlap we'll either report an error, or have some way of signalling that annotations suppress type mismatches.

danieljharvey · 2024-11-13T14:07:36Z

crates/cli/src/native_query/type_solver/mod.rs

    loop {
        let prev_type_variables = type_variables.clone();
        let prev_solutions = solutions.clone();
+        let prev_substitutions = substitutions.clone();


I'm glad your simplification was instead to make a clone of the substitutions for backtracking.

danieljharvey · 2024-11-13T14:13:52Z

crates/cli/src/native_query/aggregation_expression.rs

+    operand_type_hint: Option<TypeConstraint>,
+    operator: &str,
+    operand: Bson,
+) -> Result<TypeConstraint> {


So, broadly, forgive my massive handwaving, do I understand correctly that TypeConstraint contains either a concrete type or information we have about an unknown type (ie, that it's Numeric)?

And that we use subtyping to move constraints towards types when we combine them?

Yes, that's pretty much it with the addendum that a TypeConstraint may reference a type variable. We have a type variable for the query result type, and another type variable for each parameter, and each variable is associated with a set of TypeConstraints. Since TypeConstraint can reference variables that's a way for variables to indirectly reference each other.

Subtyping is part of it. I'm considering the query result variable to be covariant so multiple constraints converge on the supertype. Parameters are contravariant so those constraints should converge on the subtype, or lead to a mismatch error. But constraints can also relate variables in ways that are not related to subtyping. For example we've got FieldOf which is a way to state that the type of a variable is the same as the type of a given field of a given object type.

danieljharvey · 2024-11-13T14:14:45Z

crates/cli/src/native_query/aggregation-operator-progress.md

@@ -0,0 +1,280 @@
+Arithmetic Expression Operators
+
+- [x] $abs - Returns the absolute value of a number.


I assume that Mongo has no deep concept of introspection for these functions, so it's just a case of special casing each one we want to support?

Unfortunately MongoDB doesn't have much of a concept of typing at all. I don't think there is any way to introspect for signatures for functions like these.

It's possible we could build our own function signature language, and write signatures ourselves. But I thought it would be easier to hard-code this stuff, especially since the query language has a closed, limited set of functions.

I think this approach is absolutely fine, it's very clear what is going on which is great.

danieljharvey

I've left some questions to clarify my understanding, however I do not think they should stop us merging this work.

hallettj · 2024-11-18T02:15:33Z

I've left some questions to clarify my understanding, however I do not think they should stop us merging this work.

Thanks very much for the review and the feedback!

hallettj requested a review from codedmart November 7, 2024 22:33

hallettj self-assigned this Nov 7, 2024

hallettj commented Nov 7, 2024

View reviewed changes

hallettj requested review from daniel-chambers and BenoitRanque November 7, 2024 22:34

danieljharvey reviewed Nov 13, 2024

View reviewed changes

danieljharvey approved these changes Nov 13, 2024

View reviewed changes

hallettj force-pushed the jessehallett/eng-1269-when-re-generating-native-query-ignore-existing-config-for branch from b05e1ac to cf42e89 Compare November 18, 2024 02:23

Base automatically changed from jessehallett/eng-1269-when-re-generating-native-query-ignore-existing-config-for to main November 19, 2024 17:22

hallettj added 14 commits November 19, 2024 09:25

another iteration of refining simplify_constraint_pair

f1c87f5

wip

0c67178

introduce type unions, type hints in expressions

8c81a1b

revert expanded scalar supertypes

a2ae1da

tests for binary comparison inference

74d9f00

solve binary comparison with parameter

0827543

solve binary comparison without hack of ignoring variables

56594ac

record progress on operators

5cf5fda

unify parameter constraint with nested object field reference

b864a5a

other arithmetic operators, arrayElemAt

146acd5

and, or, not

fd3cd8a

$and and $or are not be homogeneous

e588e04

output errors when looking for non-existent object fields

cc05424

infer non-double type when comparing numbers for inequality

0b52183

hallettj force-pushed the jessehallett/eng-1102-support-more-aggregation-expression-operators-when branch from 4094237 to 0b52183 Compare November 19, 2024 17:26

hallettj added 2 commits November 19, 2024 10:58

fix scalar constraint resolution in contravariant case

daaee47

add more explanation for OneOf constraint variant

9f3bd7a

hallettj merged commit 1577927 into main Nov 19, 2024
1 check passed

hallettj deleted the jessehallett/eng-1102-support-more-aggregation-expression-operators-when branch November 19, 2024 19:26


		C::Union(_) => Some(Type::ExtendedJSON),

		t @ C::OneOf(_) if t.is_numeric() => {

		@@ -0,0 +1,280 @@
		Arithmetic Expression Operators

		- [x] $abs - Returns the absolute value of a number.

support more aggregation operators when generating native query configurations #120

support more aggregation operators when generating native query configurations #120

Uh oh!

Conversation

hallettj commented Nov 7, 2024

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!

Choose a reason for hiding this comment

Uh oh!

danieljharvey left a comment

Choose a reason for hiding this comment

Uh oh!

hallettj commented Nov 18, 2024

Uh oh!

Uh oh!

Uh oh!