Add support for ordering on $select clauses in aggregations #393

bryantpark04 · 2024-06-13T19:26:39Z

Opening draft PR while I start writing tests

Change the Exclude in the part to use keyof

- Replace AggregatableKeys | with ValidAggregationKeys

This was done by wrapping the `AggregateOptionsThatErrors` in another `AggregateOptsThatErrorsAndDisallowsOrderingWithMultipleGroupBy` type. This also inadvertently fixed the broken aggregate.test.ts file. This makes no sense. Adding another layer somehow helped TypeScript correctly infer the original type again?

Fix later

…ties

packages/client/src/query/aggregations/AggregationResultsWithoutGroups.ts

bryantpark04 · 2024-06-13T19:32:13Z

packages/client/src/object/AggregateOptsThatErrors.ts

 import type { GroupByClause } from "../query/aggregations/GroupByClause.js";

+export type AggregateOptsThatErrorsAndDisallowsOrderingWithMultipleGroupBy<


I tried a few different approaches for enforcing the "can't order on $select if there are multiple $groupBy clauses" restriction in the type system, and this method of adding another wrapped type layer around the AggregateOptsThatErrors type just to check this restriction was the easiest way to do it.

@ericanderson was this an okay approach?

I need to check out the repo to play with the types/intellisense but this seems like a reasonable solution.

The issue the comment was describing was fixed in f1ca1e1.

packages/client/src/internal/conversions/legacyToModernSingleAggregationResult.ts

…ng on select allowed

This reverts commit 519fd7b.

…h as I can Leaving as is - it doesn't seem any worse than before this PR.

ericanderson · 2024-06-17T11:45:00Z

packages/client/src/internal/conversions/modernToLegacyAggregationClause.ts

+    return [
+      {
+        type: metric as StringAggregateOption | NumericAggregateOption,
+        name: `${property}.${metric}`,


Worth checking, could probably just reuse k/propAndMetric here. The existing name was just an arbitrary choice by me that I knew we could split on later.

I think the property.metric string is what gets sent to the wire as part of the request body, so the reformatting from property:metric to property.metric is necessary

It is whats in the request body, but the backend doesn't care what this value is. It will simply return a result that matches that value.

We can leave as is for now

packages/client/src/internal/conversions/modernToLegacyAggregationClause.ts

ericanderson · 2024-06-17T12:18:04Z

packages/client/src/object/AggregateOptsThatErrors.ts

 import type { GroupByClause } from "../query/aggregations/GroupByClause.js";

+export type AggregateOptsThatErrorsAndDisallowsOrderingWithMultipleGroupBy<


I need to check out the repo to play with the types/intellisense but this seems like a reasonable solution.

packages/client/src/query/aggregations/AggregationResultsWithoutGroups.ts

packages/client/src/object/aggregate.test.ts

…imization)

ericanderson

You have my blessing if you have Saurav's blessing

ssanjay1

Approved, couple of unblocking comments

packages/client/src/internal/conversions/modernToLegacyAggregationClause.ts

packages/client/src/object/aggregate.test.ts

bryantpark04 · 2024-06-18T14:37:56Z

Approved, couple of unblocking comments

What kind of cruel joke is this

Thank you Saurav!

bryantpark04 added 13 commits June 10, 2024 17:57

Add OrderedAggregationClause with foo:bar properties

9138578

Update part of aggregate.test.ts

3624aaf

Change the Exclude in the part to use keyof

Factor out ValidAggregationKeys

ae8525b

Update AggregationResultsWithoutGroups to handle new syntax

efa6fe5

- Replace AggregatableKeys | with ValidAggregationKeys

Move ValidAggregationKeys to AggregatableKeys.ts

fbd6657

Revert adding keyof to the Exclude in AOThatErrors

fd325f6

Add OrderedAggregateOpts to the union

c5e9347

Update modernToLegacyAggregationClause to handle new format

56d2714

Expect TS errors in legacyToModernSingleAggregationResult

519fd7b

Fix later

Migrate AggregationsResults.test.ts to new syntax

ba5d373

Migrate more files to new syntax

710f08d

Treat decimal and long as numeric properties instead of string proper…

1baadae

…ties

bryantpark04 self-assigned this Jun 13, 2024

bryantpark04 linked an issue Jun 13, 2024 that may be closed by this pull request

Add orderBy for aggregations #323

Closed

bryantpark04 commented Jun 13, 2024

View reviewed changes

packages/client/src/query/aggregations/AggregationResultsWithoutGroups.ts Outdated Show resolved Hide resolved

bryantpark04 commented Jun 13, 2024

View reviewed changes

Remove long comment in aggregate.test.ts

4d54b1b

The issue the comment was describing was fixed in f1ca1e1.

bryantpark04 commented Jun 13, 2024

View reviewed changes

packages/client/src/internal/conversions/legacyToModernSingleAggregationResult.ts Outdated Show resolved Hide resolved

bryantpark04 added 4 commits June 14, 2024 09:27

Test mock request body for aggregate without grouping

acf1418

Add orderBy with and without groupBy tests

7b5769c

Allow ordered select with empty groupBy

446f577

Add test for type errors in the case of multiple groupBys - no orderi…

d7b7762

…ng on select allowed

bryantpark04 force-pushed the bryantp/aggregation-order-by branch from da281f5 to d7b7762 Compare June 14, 2024 14:31

Don't export AggregateOptsThatErrors

b5f02c9

bryantpark04 requested a review from ssanjay1 June 14, 2024 14:46

bryantpark04 added 2 commits June 14, 2024 10:47

Revert "Expect TS errors in legacyToModernSingleAggregationResult"

2155547

This reverts commit 519fd7b.

Restrict the types in legacyToModernSingleAggregationResult.ts as muc…

9f5ac6e

…h as I can Leaving as is - it doesn't seem any worse than before this PR.

bryantpark04 marked this pull request as ready for review June 14, 2024 20:22

bryantpark04 requested a review from ericanderson June 14, 2024 20:22

ericanderson reviewed Jun 17, 2024

View reviewed changes

ssanjay1 reviewed Jun 17, 2024

View reviewed changes

packages/client/src/object/aggregate.test.ts Outdated Show resolved Hide resolved

bryantpark04 added 8 commits June 17, 2024 11:24

Rename k, v in modernToLegacyAggClause

913fe5e

Reorder fields in modernToLegacyAggClause to be consistent (micro-opt…

03c6fd2

…imization)

Replace infer _ with string in AggResultsWOGroups

990f944

Fix broken tests after the order of fields was changed

e03129d

Refactor AggregationResultsWithoutGroups with helper types

7840ded

Remove AggregationCountResult type

1a4a94a

Refactor test setup into a beforeEach

37f7368

Add changeset

23ca08f

ericanderson reviewed Jun 17, 2024

View reviewed changes

ssanjay1 approved these changes Jun 18, 2024

View reviewed changes

packages/client/src/internal/conversions/modernToLegacyAggregationClause.ts Outdated Show resolved Hide resolved

packages/client/src/object/aggregate.test.ts Outdated Show resolved Hide resolved

bryantpark04 added 3 commits June 18, 2024 12:46

Merge branch 'main' into bryantp/aggregation-order-by

128c65f

Thank you Saurav!

Use beforeAll instead of beforeEach in aggregate.test.ts

266e8af

Remove outdated comment

abfdb35

bryantpark04 merged commit 02c65c5 into main Jun 18, 2024
6 checks passed

bryantpark04 deleted the bryantp/aggregation-order-by branch June 18, 2024 17:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ordering on $select clauses in aggregations #393

Add support for ordering on $select clauses in aggregations #393

bryantpark04 commented Jun 13, 2024

bryantpark04 Jun 13, 2024

ericanderson Jun 17, 2024

ericanderson Jun 17, 2024

bryantpark04 Jun 17, 2024

ericanderson Jun 17, 2024

ericanderson Jun 17, 2024

ericanderson Jun 17, 2024

ericanderson left a comment

ssanjay1 left a comment

bryantpark04 commented Jun 18, 2024 •

edited

Loading

		import type { GroupByClause } from "../query/aggregations/GroupByClause.js";

		export type AggregateOptsThatErrorsAndDisallowsOrderingWithMultipleGroupBy<

Add support for ordering on $select clauses in aggregations #393

Add support for ordering on $select clauses in aggregations #393

Conversation

bryantpark04 commented Jun 13, 2024

bryantpark04 Jun 13, 2024

Choose a reason for hiding this comment

ericanderson Jun 17, 2024

Choose a reason for hiding this comment

ericanderson Jun 17, 2024

Choose a reason for hiding this comment

bryantpark04 Jun 17, 2024

Choose a reason for hiding this comment

ericanderson Jun 17, 2024

Choose a reason for hiding this comment

ericanderson Jun 17, 2024

Choose a reason for hiding this comment

ericanderson Jun 17, 2024

Choose a reason for hiding this comment

ericanderson left a comment

Choose a reason for hiding this comment

ssanjay1 left a comment

Choose a reason for hiding this comment

bryantpark04 commented Jun 18, 2024 • edited Loading

bryantpark04 commented Jun 18, 2024 •

edited

Loading