impr(quaint/qe): PostgreSQL `DISTINCT ON` #4223

Druue · 2023-09-12T10:46:53Z

This PR is looking to introduce the structure for further in-db distinct work and the base case for PostgreSQL's distinct on

Added quaint rendering for DISTINCT ON
Hooked up QE to to use the DISTINCT ON api from quaint
in-memory-record-processor now allows for performing distinct separately in db
// Add support for more granularly opting in to db-level distinct per query args
// in-memory-builder was removed

Follow-up steps involve

basic orderBy support
extended orderBy support
CockroachDB
MySQL
SQLite

contributes prisma/prisma#14765
closes https://github.com/prisma/team-orm/issues/774

relevant discovery work

quaint/src/visitor/postgres.rs

codspeed-hq · 2023-09-12T11:05:18Z

CodSpeed Performance Report

Merging #4223 will not alter performance

_{Comparing impr/distinct-on (a948112) with main (5c4707e)}

Summary

✅ 11 untouched benchmarks

query-engine/connectors/sql-query-connector/src/query_builder/read.rs

query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs

nkovacic · 2023-11-08T11:50:38Z

@Druue any progress on this PR? Would be very beneficial for performance on distinct queries.

Druue · 2023-11-09T13:05:07Z

@Druue any progress on this PR? Would be very beneficial for performance on distinct queries.

Hey @nkovacic, yeah! I'm picking this PR back up today; ran into some issues that brought up the need to do some more discovery work

queries::distinct::distinct::with_duplicates

Add capability for db distinct in: - read_many - m2m - one2m

- inmemory distinct will actually be used Added - capability for distinct inmem when orderby

query-engine/query-structure/src/query_arguments.rs

Weakky

Good first step. Some things could be improved. Also, it looks like a lot of tests are failing atm.

query-engine/core/src/interpreter/query_interpreters/inmemory_record_processor.rs

query-engine/core/src/interpreter/query_interpreters/read.rs

query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs

query-engine/query-structure/src/query_arguments.rs

psl/builtin-connectors/src/postgres_datamodel_connector.rs

query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs

using builder

query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs

- track current in-mem snapshots - track in-db pg snapshots

query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs

Weakky

Let's revisit the tests in the next bits of work, LGTM for now, nice job 👍

Weakky

LGTM 👍

janpio · 2023-12-04T18:28:12Z

query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs

+    #[connector_test(only(Postgres))]
+    async fn no_panic_pg(runner: Runner) -> TestResult<()> {
        test_user(&runner, r#"{ id: 1, first_name: "Joe", last_name: "Doe", email: "1" }"#).await?;
        test_user(&runner, r#"{ id: 2, first_name: "Doe", last_name: "Joe", email: "2" }"#).await?;

-        assert_query!(
-            runner,
-            "query { findManyUser(distinct: [first_name, last_name]) { id } }",
-            r#"{"data":{"findManyUser":[{"id":1},{"id":2}]}}"#
+        insta::assert_snapshot!(
+            run_query!(
+                &runner,
+                indoc!("{
+                    findManyUser(distinct: [first_name, last_name])
+                    { id } 
+                }")
+            ),
+            @r###"{"data":{"findManyUser":[{"id":2},{"id":1}]}}"###
+        );
+
+        Ok(())
+    }
+
+    /// Regression test for not selecting the fields the distinct is performed on: https://github.com/prisma/prisma/issues/5969
+    #[connector_test(exclude(Postgres))]
+    async fn no_panic_other(runner: Runner) -> TestResult<()> {
+        test_user(&runner, r#"{ id: 1, first_name: "Joe", last_name: "Doe", email: "1" }"#).await?;
+        test_user(&runner, r#"{ id: 2, first_name: "Doe", last_name: "Joe", email: "2" }"#).await?;
+
+        insta::assert_snapshot!(
+            run_query!(
+                &runner,
+                indoc!("{
+                    findManyUser(distinct: [first_name, last_name])
+                    { id } 
+                }")
+            ),
+            @r###"{"data":{"findManyUser":[{"id":1},{"id":2}]}}"###
        );


Can we not differentiate the snapshots by "connector" here and avoid the duplication?

Druue requested a review from a team as a code owner September 12, 2023 10:46

Druue added this to the 5.4.0 milestone Sep 12, 2023

Druue commented Sep 12, 2023

View reviewed changes

quaint/src/visitor/postgres.rs Show resolved Hide resolved

Druue changed the title ~~impr(quaint/qe): DISTINCT ON~~ WIP -- impr(quaint/qe): DISTINCT ON Sep 12, 2023

Druue marked this pull request as draft September 12, 2023 10:49

Druue commented Sep 12, 2023

View reviewed changes

query-engine/connectors/sql-query-connector/src/query_builder/read.rs Show resolved Hide resolved

Druue commented Sep 12, 2023

View reviewed changes

query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs Outdated Show resolved Hide resolved

Druue marked this pull request as ready for review September 19, 2023 08:10

Druue force-pushed the impr/distinct-on branch from 04a77e2 to 5104698 Compare November 9, 2023 13:41

Druue changed the title ~~WIP -- impr(quaint/qe): DISTINCT ON~~ WIP -- impr(quaint/qe): PostgreSQL DISTINCT ON Nov 9, 2023

janpio marked this pull request as draft November 9, 2023 14:30

janpio assigned Druue Nov 9, 2023

Druue force-pushed the impr/distinct-on branch from 5104698 to 105e979 Compare November 10, 2023 16:47

Druue added 9 commits November 28, 2023 14:25

add distinct db capability

57bd41f

wip distinct typing

16ec526

distinct_on rendering in quaint

4f2e7ea

rendering

deee44d

Breaking tests

4c5e39a

Test Highlight:

67cbbbc

queries::distinct::distinct::with_duplicates

Add builder for inmemproc

143988e

Add capability for db distinct in: - read_many - m2m - one2m

Update tests (wip)

40a9917

Fixed

611cc4e

- inmemory distinct will actually be used Added - capability for distinct inmem when orderby

Druue force-pushed the impr/distinct-on branch from 105e979 to 611cc4e Compare November 29, 2023 00:40

Druue marked this pull request as ready for review November 29, 2023 00:42

Druue requested a review from a team as a code owner November 29, 2023 00:42

Druue requested review from miguelff and removed request for a team November 29, 2023 00:42

Druue requested a review from jkomyno November 29, 2023 00:42

Druue commented Nov 29, 2023

View reviewed changes

query-engine/query-structure/src/query_arguments.rs Outdated Show resolved Hide resolved

Clean up distinct builder

2e7c863

Weakky requested changes Nov 29, 2023

View reviewed changes

move apply_distinct back into apply

90b35f3

Druue marked this pull request as draft November 30, 2023 12:48

Druue added 2 commits November 30, 2023 14:39

Fixed skip bug

8401c12

using builder

rename distinct capability distincton

c836bcc

Druue changed the title ~~WIP -- impr(quaint/qe): PostgreSQL DISTINCT ON~~ impr(quaint/qe): PostgreSQL DISTINCT ON Nov 30, 2023

Druue commented Nov 30, 2023

View reviewed changes

query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs Outdated Show resolved Hide resolved

Druue added 3 commits November 30, 2023 17:13

revert back to using inm_processor directly

0223800

Updated tests to:

64b8841

- track current in-mem snapshots - track in-db pg snapshots

remove nl

66f919a

Weakky reviewed Nov 30, 2023

View reviewed changes

query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs Outdated Show resolved Hide resolved

Weakky reviewed Nov 30, 2023

View reviewed changes

query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs Outdated Show resolved Hide resolved

Druue marked this pull request as ready for review November 30, 2023 17:06

Weakky reviewed Nov 30, 2023

View reviewed changes

query-engine/connector-test-kit-rs/query-engine-tests/tests/queries/distinct.rs Outdated Show resolved Hide resolved

Weakky approved these changes Nov 30, 2023

View reviewed changes

apolanc modified the milestones: 5.4.0, 5.7.0 Nov 30, 2023

Druue added 4 commits December 1, 2023 15:34

Add DistinctOn preview feature

439eeb0

fixed req-in-mem-distinct logic

14c206b

Fix aggregation tests

20a8420

Update preview feature test

a948112

Weakky approved these changes Dec 4, 2023

View reviewed changes

Druue merged commit a0c366a into main Dec 4, 2023
67 checks passed

Druue deleted the impr/distinct-on branch December 4, 2023 12:12

janpio reviewed Dec 4, 2023

View reviewed changes

Druue mentioned this pull request Dec 6, 2023

impr(qe): PostgreSQL DISTINCT ON w/ ORDER BY #4525

Open

Druue mentioned this pull request Dec 20, 2023

feat(qe): Add DistinctOn to crdb connector capabilities #4597

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

impr(quaint/qe): PostgreSQL `DISTINCT ON` #4223

impr(quaint/qe): PostgreSQL `DISTINCT ON` #4223

Druue commented Sep 12, 2023 •

edited

codspeed-hq bot commented Sep 12, 2023 •

edited

nkovacic commented Nov 8, 2023

Druue commented Nov 9, 2023

Weakky left a comment •

edited

Weakky left a comment

Weakky left a comment

janpio Dec 4, 2023

impr(quaint/qe): PostgreSQL DISTINCT ON #4223

impr(quaint/qe): PostgreSQL DISTINCT ON #4223

Conversation

Druue commented Sep 12, 2023 • edited

codspeed-hq bot commented Sep 12, 2023 • edited

CodSpeed Performance Report

Merging #4223 will not alter performance

Summary

nkovacic commented Nov 8, 2023

Druue commented Nov 9, 2023

Weakky left a comment • edited

Choose a reason for hiding this comment

Weakky left a comment

Choose a reason for hiding this comment

Weakky left a comment

Choose a reason for hiding this comment

janpio Dec 4, 2023

Choose a reason for hiding this comment

impr(quaint/qe): PostgreSQL `DISTINCT ON` #4223

impr(quaint/qe): PostgreSQL `DISTINCT ON` #4223

Druue commented Sep 12, 2023 •

edited

codspeed-hq bot commented Sep 12, 2023 •

edited

Weakky left a comment •

edited