Adjust distinct aggregation expansion on Mongo #44442

lbrdnk · 2024-06-19T20:28:53Z

Description

This PR modifies expressions that are used in $addFields stage in aggregation transformation. Specifically, names that are result of :distinct aggregation are wrapped in $size to produce actual counts.

How to verify

Existing test was modified to check for the new behavior.
New test was added to check compilation of more complex case.
New test was added to ensure querying works as expected even more complex case.

Checklist

Tests have been added/updated to cover changes in this PR

replay-io · 2024-06-19T20:40:38Z

Status	In Progress ↗︎ 59 / 60
Commit	`ed977c2`
Results	⚠️ 5 Flaky ✅ 2680 Passed

lbrdnk · 2024-06-20T15:26:54Z

modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj

-(def ^:private $addFields
-  :$addFields)
-


Operators are defined as strings. This removal produces consistent compilation results in modified test, ie. also $addFields is string in compiled query.

bshepherdson

Good stuff, just some Clojure code polishing.

bshepherdson · 2024-06-21T17:22:35Z

modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj

+  If there is a need for more _pre-post_ transformations (as per results of [[expand-aggregations]]), this function
+  should be generalized."


It's not clear what this means. I'd either turn this into a TODO comment that calls out a specific weakness to be fixed later, or just remove this sentence.

By pre-post transformations I mean transformations that are necessary on :post key of expand-aggregations return value. Even if meaning of the comment was evident, its added value is questionable. Removed.

bshepherdson · 2024-06-21T17:25:10Z

modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj

+        aggr-expr' (walk/postwalk (fn [x]
+                                    (if (and (string? x)
+                                             (distinct-vals x))
+                                      {$size x}
+                                      x))
+                                  aggr-expr)]
+    [aggr-expr' mappings]))


No need for an extra let binding here; this can be inlined into the output clause.

In context of the following function, expand-aggregations, I've deliberately decided to have an explicit var aggr-expr', to signal on the first sight that returned value has a same shape as what's used in the other function. I've also decided to bind the distinct-keys var, despite it could have been omitted by moving filter into transducer composition on the next line. I, subjectively, find this form easier on eyes of future reader.

But as I'm not the only future reader I did as suggested :)

modules/drivers/mongo/src/metabase/driver/mongo/query_processor.clj

bshepherdson · 2024-06-21T17:28:03Z

modules/drivers/mongo/test/metabase/driver/mongo/query_processor_test.clj

+(deftest ^:parallel multiple-aggregations-with-distinct-count-expression-test
+  (mt/test-driver
+   :mongo
+   (testing "Should generate correct queries for `:distinct` in expressions ()"


Missing the bug number?

…r.clj Co-authored-by: Braden Shepherdson <braden@metabase.com>

This commit adds `$size` operator wrapping to `$addToSet`. That enables use of `:distinct` clause in expressions on Mongo.

lbrdnk · 2024-06-24T10:13:00Z

@metabase-bot backport release-x.49.x

github-actions · 2024-06-24T10:14:16Z

@lbrdnk something went wrong while creating a backport [Logs]

This commit adds `$size` operator wrapping to `$addToSet`. That enables use of `:distinct` clause in expressions on Mongo.

This commit adds `$size` operator wrapping to `$addToSet`. That enables use of `:distinct` clause in expressions on Mongo. Co-authored-by: lbrdnk <lbrdnk@users.noreply.github.com>

github-actions · 2024-06-25T09:40:00Z

🚀 This should also be released by v0.50.7

Update aggregation expansion

4d03290

lbrdnk added Database/Mongo backport Automatically create PR on current release branch on merge .Team/QueryProcessor :hammer_and_wrench: labels Jun 19, 2024

lbrdnk requested a review from camsaul as a code owner June 19, 2024 20:28

metabase-bot bot assigned lbrdnk Jun 19, 2024

lbrdnk added 5 commits June 20, 2024 14:24

Add reproduction into normal drivers tests

87f1415

Move adjustment after simplification

939518f

Use $addFields defined in operators and update test

6ad8e0d

Add compilation test

6d338c1

Add general test with querying

d0dd059

lbrdnk commented Jun 20, 2024

View reviewed changes

lbrdnk requested a review from a team June 20, 2024 15:32

bshepherdson approved these changes Jun 21, 2024

View reviewed changes

lbrdnk and others added 3 commits June 24, 2024 10:20

Update modules/drivers/mongo/src/metabase/driver/mongo/query_processo…

8eedebb

…r.clj Co-authored-by: Braden Shepherdson <braden@metabase.com>

Address review remarks

f617395

Get rid of let binding

ed977c2

lbrdnk merged commit ba42baa into master Jun 24, 2024
109 checks passed

lbrdnk deleted the mongo-distinct-esc branch June 24, 2024 10:10

github-automation-metabase pushed a commit that referenced this pull request Jun 24, 2024

Adjust distinct aggregation expansion on Mongo (#44442)

010892e

This commit adds `$size` operator wrapping to `$addToSet`. That enables use of `:distinct` clause in expressions on Mongo.

github-automation-metabase mentioned this pull request Jun 24, 2024

🤖 backported "Adjust distinct aggregation expansion on Mongo" #44604

Merged

lbrdnk added a commit that referenced this pull request Jun 24, 2024

Backport #44442 -- Mongo distinct aggregation

6e4e955

lbrdnk mentioned this pull request Jun 24, 2024

Backport #44442 -- Mongo distinct aggregation #44607

Merged

iethree pushed a commit that referenced this pull request Jun 25, 2024

Adjust distinct aggregation expansion on Mongo (#44442)

3958945

This commit adds `$size` operator wrapping to `$addToSet`. That enables use of `:distinct` clause in expressions on Mongo.

lbrdnk added a commit that referenced this pull request Jun 25, 2024

Backport #44442 -- Mongo distinct aggregation (#44607)

232e424

github-actions bot added this to the 0.49.18 milestone Jun 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust distinct aggregation expansion on Mongo #44442

Adjust distinct aggregation expansion on Mongo #44442

lbrdnk commented Jun 19, 2024 •

edited

Loading

replay-io bot commented Jun 19, 2024 •

edited

Loading

lbrdnk Jun 20, 2024

bshepherdson left a comment

bshepherdson Jun 21, 2024

lbrdnk Jun 24, 2024

bshepherdson Jun 21, 2024

lbrdnk Jun 24, 2024

lbrdnk Jun 24, 2024

bshepherdson Jun 21, 2024

lbrdnk Jun 24, 2024

lbrdnk commented Jun 24, 2024

github-actions bot commented Jun 24, 2024

github-actions bot commented Jun 25, 2024

		If there is a need for more _pre-post_ transformations (as per results of [[expand-aggregations]]), this function
		should be generalized."

Adjust distinct aggregation expansion on Mongo #44442

Adjust distinct aggregation expansion on Mongo #44442

Conversation

lbrdnk commented Jun 19, 2024 • edited Loading

Description

How to verify

Checklist

replay-io bot commented Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

bshepherdson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbrdnk commented Jun 24, 2024

github-actions bot commented Jun 24, 2024

github-actions bot commented Jun 25, 2024

lbrdnk commented Jun 19, 2024 •

edited

Loading

replay-io bot commented Jun 19, 2024 •

edited

Loading