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

* Add OrderedAggregationClause with foo:bar properties * Update part of aggregate.test.ts Change the Exclude in the part to use keyof * Factor out ValidAggregationKeys * Update AggregationResultsWithoutGroups to handle new syntax - Replace AggregatableKeys | with ValidAggregationKeys * Move ValidAggregationKeys to AggregatableKeys.ts * Revert adding keyof to the Exclude in AOThatErrors * Add OrderedAggregateOpts to the union * Disallow ordered $select with multiple $groupBy in the type system 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? * Update modernToLegacyAggregationClause to handle new format * Expect TS errors in legacyToModernSingleAggregationResult Fix later * Migrate AggregationsResults.test.ts to new syntax * Migrate more files to new syntax * Treat decimal and long as numeric properties instead of string properties * Remove long comment in aggregate.test.ts The issue the comment was describing was fixed in f1ca1e1. * Test mock request body for aggregate without grouping * Add orderBy with and without groupBy tests * Allow ordered select with empty groupBy * Add test for type errors in the case of multiple groupBys - no ordering on select allowed * Don't export AggregateOptsThatErrors * Revert "Expect TS errors in legacyToModernSingleAggregationResult" This reverts commit 519fd7b. * Restrict the types in legacyToModernSingleAggregationResult.ts as much as I can Leaving as is - it doesn't seem any worse than before this PR. * Rename k, v in modernToLegacyAggClause * Reorder fields in modernToLegacyAggClause to be consistent (micro-optimization) * Replace infer _ with string in AggResultsWOGroups * Fix broken tests after the order of fields was changed * Refactor AggregationResultsWithoutGroups with helper types * Remove AggregationCountResult type * Refactor test setup into a beforeEach * Add changeset * Use beforeAll instead of beforeEach in aggregate.test.ts * Remove outdated comment
palantir · Jun 18, 2024 · 02c65c5 · 02c65c5
1 parent 7adf5c7
commit 02c65c5
Show file tree

Hide file tree

Showing 23 changed files with 474 additions and 198 deletions.
diff --git a/.changeset/chilly-singers-compare.md b/.changeset/chilly-singers-compare.md
@@ -0,0 +1,5 @@
+---
+"@osdk/client": minor
+---
+
+Rework $select syntax in aggregations to add support for ordering by metrics
diff --git a/examples-extra/basic/cli/src/legacy-examples/fetchAggregationForEmployees.ts b/examples-extra/basic/cli/src/legacy-examples/fetchAggregationForEmployees.ts
@@ -25,9 +25,11 @@ export async function fetchAggregationForEmployees(
 ) {
   const result = await client(Employee).aggregate({
     $select: {
-      locationCity: "approximateDistinct",
-      locationName: "approximateDistinct",
-      employeeNumber: ["avg", "max", "min"],
+      "locationCity:approximateDistinct": "unordered",
+      "locationName:approximateDistinct": "unordered",
+      "employeeNumber:avg": "unordered",
+      "employeeNumber:max": "unordered",
+      "employeeNumber:min": "unordered",
     },
     $groupBy: undefined,
   });

diff --git a/examples-extra/basic/cli/src/legacy-examples/fetchAggregationForEmployeesGrouped.ts b/examples-extra/basic/cli/src/legacy-examples/fetchAggregationForEmployeesGrouped.ts
@@ -25,9 +25,11 @@ export async function fetchAggregationForEmployeesGrouped(
 ) {
   const result = await client(Employee).aggregate({
     $select: {
-      locationCity: "approximateDistinct",
-      locationName: "approximateDistinct",
-      employeeNumber: ["avg", "max", "min"],
+      "locationCity:approximateDistinct": "unordered",
+      "locationName:approximateDistinct": "unordered",
+      "employeeNumber:avg": "unordered",
+      "employeeNumber:max": "unordered",
+      "employeeNumber:min": "unordered",
     },
     $groupBy: {
       locationType: "exact",

diff --git a/examples-extra/basic/cli/src/runAggregationGroupByDatesTest.ts b/examples-extra/basic/cli/src/runAggregationGroupByDatesTest.ts
@@ -19,17 +19,17 @@ import { client } from "./client.js";
 
 export async function runAggregationGroupByDatesTest() {
   const groupedTimestamps = await client(BuilderDeploymentState).aggregate({
-    $select: { $count: true },
+    $select: { $count: "unordered" },
     $groupBy: { currentTimestamp: { $duration: [10, "seconds"] } },
   });
 
   const groupedDates = await client(BuilderDeploymentState).aggregate({
-    $select: { $count: true },
+    $select: { $count: "unordered" },
     $groupBy: { date: { $duration: [10, "days"] } },
   });
 
   const rangedDates = await client(BuilderDeploymentState).aggregate({
-    $select: { $count: true },
+    $select: { $count: "unordered" },
     $groupBy: {
       date: {
         $ranges: [["2008-03-01", "2009-11-05"], ["2015-10-01", "2018-11-05"]],
@@ -38,7 +38,7 @@ export async function runAggregationGroupByDatesTest() {
   });
 
   const rangedTimestamps = await client(BuilderDeploymentState).aggregate({
-    $select: { $count: true },
+    $select: { $count: "unordered" },
     $groupBy: {
       currentTimestamp: {
         $ranges: [["2023-04-02T17:28:00Z", "2023-04-03T18:28:00Z"], [

diff --git a/examples-extra/basic/cli/src/runAggregationsTest.ts b/examples-extra/basic/cli/src/runAggregationsTest.ts
@@ -29,7 +29,12 @@ export async function runAggregationsTest() {
 
   const testAggregateCountNoGroup = await client(BoundariesUsState)
     .aggregate({
-      $select: { $count: true, latitude: ["min", "max", "avg"] },
+      $select: {
+        $count: "unordered",
+        "latitude:max": "unordered",
+        "latitude:min": "unordered",
+        "latitude:avg": "unordered",
+      },
     });
 
   // Should be 51 because it includes DC
@@ -41,7 +46,12 @@ export async function runAggregationsTest() {
   );
   const testAggregateCountWithGroups = await client(BoundariesUsState)
     .aggregate({
-      $select: { $count: true, latitude: ["min", "max", "avg"] },
+      $select: {
+        $count: "unordered",
+        "latitude:max": "unordered",
+        "latitude:min": "unordered",
+        "latitude:avg": "unordered",
+      },
       $groupBy: {
         usState: "exact",
         longitude: {
@@ -52,7 +62,12 @@ export async function runAggregationsTest() {
 
   const testAggregateCountWithFixedGroups = await client(BoundariesUsState)
     .aggregate({
-      $select: { $count: true, latitude: ["min", "max", "avg"] },
+      $select: {
+        $count: "unordered",
+        "latitude:max": "unordered",
+        "latitude:min": "unordered",
+        "latitude:avg": "unordered",
+      },
       $groupBy: {
         longitude: {
           $exactWithLimit: 40,
@@ -62,7 +77,9 @@ export async function runAggregationsTest() {
 
   const testAggregateCountWithRangeGroups = await client(BoundariesUsState)
     .aggregate({
-      $select: { $count: true },
+      $select: {
+        $count: "unordered",
+      },
       $groupBy: {
         latitude: {
           $ranges: [[34, 39], [

diff --git a/examples-extra/basic/cli/src/typeChecks.ts b/examples-extra/basic/cli/src/typeChecks.ts
@@ -48,11 +48,11 @@ export async function typeChecks(client: Client) {
     const q = await client(Ontology.objects.ObjectTypeWithAllPropertyTypes)
       .aggregate({
         $select: {
-          integer: "sum",
-          float: "sum",
-          decimal: "sum",
-          short: ["max"],
-          string: "approximateDistinct",
+          "integer:sum": "unordered",
+          "float:sum": "unordered",
+          "decimal:sum": "unordered",
+          "short:max": "unordered",
+          "string:approximateDistinct": "unordered",
         },
         $groupBy: {
           string: "exact",

diff --git a/packages/client.api/package.json b/packages/client.api/package.json
@@ -33,6 +33,7 @@
     "@osdk/api": "workspace:^",
     "@osdk/internal.foundry": "workspace:^",
     "@types/geojson": "^7946.0.14",
+    "type-fest": "^4.18.2",
     "typescript": "^5.4.5"
   },
   "publishConfig": {

diff --git a/packages/client.api/src/aggregate/AggregatableKeys.ts b/packages/client.api/src/aggregate/AggregatableKeys.ts
@@ -15,6 +15,32 @@
  */
 
 import type { ObjectOrInterfaceDefinition } from "@osdk/api";
+import type { PropertyValueClientToWire } from "@osdk/client.api";
+
+export type StringAggregateOption = "approximateDistinct";
+export type NumericAggregateOption =
+  | "min"
+  | "max"
+  | "sum"
+  | "avg"
+  | "approximateDistinct";
+
+type AGG_FOR_TYPE<T> = number extends T ? NumericAggregateOption
+  : string extends T ? StringAggregateOption
+  : never;
+
+export type ValidAggregationKeys<
+  Q extends ObjectOrInterfaceDefinition,
+> = keyof (
+  & {
+    [
+      KK in AggregatableKeys<Q> as `${KK & string}:${AGG_FOR_TYPE<
+        PropertyValueClientToWire[Q["properties"][KK]["type"]]
+      >}`
+    ]?: any;
+  }
+  & { $count?: any }
+);
 
 export type AggregatableKeys<
   Q extends ObjectOrInterfaceDefinition,

diff --git a/packages/client.api/src/aggregate/AggregateOpts.ts b/packages/client.api/src/aggregate/AggregateOpts.ts
@@ -16,15 +16,16 @@
 
 import type { ObjectOrInterfaceDefinition } from "@osdk/api";
 import type { GroupByClause } from "../groupby/GroupByClause.js";
-import type { AggregatableKeys } from "./AggregatableKeys.js";
-import type { AggregationClause } from "./AggregationsClause.js";
+import type {
+  OrderedAggregationClause,
+  UnorderedAggregationClause,
+} from "./AggregationsClause.js";
 import type { WhereClause } from "./WhereClause.js";
 
-export type AggregateOpts<
-  Q extends ObjectOrInterfaceDefinition,
-  KK extends AggregatableKeys<Q> = AggregatableKeys<Q>,
-> = {
-  $select: AggregationClause<Q, KK>;
+export type AggregateOpts<Q extends ObjectOrInterfaceDefinition> = {
+  $select:
+    | UnorderedAggregationClause<Q>
+    | OrderedAggregationClause<Q>;
   $where?: WhereClause<Q>;
   $groupBy?: GroupByClause<Q>;
 };
diff --git a/packages/client.api/src/aggregate/AggregateOptsThatErrors.ts b/packages/client.api/src/aggregate/AggregateOptsThatErrors.ts
@@ -15,10 +15,27 @@
  */
 
 import type { ObjectOrInterfaceDefinition } from "@osdk/api";
+import type { SingleKeyObject } from "type-fest";
 import type { GroupByClause } from "../groupby/GroupByClause.js";
 import type { AggregateOpts } from "./AggregateOpts.js";
+import type { UnorderedAggregationClause } from "./AggregationsClause.js";
 
-export type AggregateOptsThatErrors<
+export type AggregateOptsThatErrorsAndDisallowsOrderingWithMultipleGroupBy<
+  Q extends ObjectOrInterfaceDefinition,
+  AO extends AggregateOpts<Q>,
+> = SingleKeyObject<AO["$groupBy"]> extends never ? (
+    AO["$select"] extends UnorderedAggregationClause<Q>
+      ? AggregateOptsThatErrors<Q, AO>
+      : {} extends AO["$groupBy"] ? AggregateOptsThatErrors<Q, AO>
+      : {
+        $groupBy: AO["$groupBy"];
+        $select: UnorderedAggregationClause<Q>;
+        $where?: AO["$where"];
+      }
+  )
+  : AggregateOptsThatErrors<Q, AO>;
+
+type AggregateOptsThatErrors<
   Q extends ObjectOrInterfaceDefinition,
   AO extends AggregateOpts<Q>,
 > =

diff --git a/packages/client.api/src/aggregate/AggregationResultsWithGroups.ts b/packages/client.api/src/aggregate/AggregationResultsWithGroups.ts
@@ -17,15 +17,15 @@
 import type { ObjectOrInterfaceDefinition } from "@osdk/api";
 import type { OsdkObjectPropertyType } from "../Definitions.js";
 import type { GroupByClause, GroupByRange } from "../groupby/GroupByClause.js";
+import type { AggregationResultsWithoutGroups } from "./AggregationResultsWithoutGroups.js";
 import type {
-  AggregationCountResult,
-  AggregationResultsWithoutGroups,
-} from "./AggregationResultsWithoutGroups.js";
-import type { AggregationClause } from "./AggregationsClause.js";
+  OrderedAggregationClause,
+  UnorderedAggregationClause,
+} from "./AggregationsClause.js";
 
 export type AggregationResultsWithGroups<
   Q extends ObjectOrInterfaceDefinition<any, any>,
-  A extends AggregationClause<Q>,
+  A extends UnorderedAggregationClause<Q> | OrderedAggregationClause<Q>,
   G extends GroupByClause<Q> | undefined,
 > = (
   & {
@@ -35,6 +35,5 @@ export type AggregationResultsWithGroups<
         : OsdkObjectPropertyType<Q["properties"][P], true>;
     };
   }
-  & AggregationCountResult<Q, A>
   & AggregationResultsWithoutGroups<Q, A>
 )[];
diff --git a/packages/client.api/src/aggregate/AggregationResultsWithoutGroups.ts b/packages/client.api/src/aggregate/AggregationResultsWithoutGroups.ts
@@ -15,26 +15,29 @@
  */
 
 import type { ObjectOrInterfaceDefinition } from "@osdk/api";
-import type { AggregationClause } from "@osdk/client.api";
+import type {
+  OrderedAggregationClause,
+  UnorderedAggregationClause,
+} from "@osdk/client.api";
 import type { OsdkObjectPropertyType } from "../Definitions.js";
-import type { StringArrayToUnion } from "../util/StringArrayToUnion.js";
 
-type SubselectKeys<AC extends AggregationClause<any>, P extends keyof AC> =
-  AC[P] extends readonly string[] | string ? P : never;
+type ExtractPropName<T extends string> = T extends `${infer PropName}:${string}`
+  ? PropName
+  : T extends "$count" ? T
+  : never;
+
+type ExtractMetricNameForPropName<T, PropName extends string> = T extends
+  `${PropName}:${infer MetricName}` ? MetricName : never;
 
 export type AggregationResultsWithoutGroups<
   Q extends ObjectOrInterfaceDefinition<any, any>,
-  AC extends AggregationClause<Q>,
+  AC extends UnorderedAggregationClause<Q> | OrderedAggregationClause<Q>,
 > = {
-  [P in keyof Q["properties"] as SubselectKeys<AC, P>]: AC[P] extends
-    readonly string[] | string ? {
-      [Z in StringArrayToUnion<AC[P]>]: Z extends "approximateDistinct" ? number
-        : OsdkObjectPropertyType<Q["properties"][P]>;
-    }
-    : never;
+  [PropName in ExtractPropName<keyof AC & string>]: PropName extends "$count"
+    ? number
+    : {
+      [MetricName in ExtractMetricNameForPropName<keyof AC & string, PropName>]:
+        MetricName extends "approximateDistinct" ? number
+          : OsdkObjectPropertyType<Q["properties"][PropName]>;
+    };
 };
-
-export type AggregationCountResult<
-  Q extends ObjectOrInterfaceDefinition<any, any>,
-  A extends AggregationClause<Q>,
-> = "$count" extends keyof A ? { $count: number } : {};
diff --git a/packages/client.api/src/aggregate/AggregationsClause.test.ts b/packages/client.api/src/aggregate/AggregationsClause.test.ts
@@ -16,7 +16,7 @@
 
 import { describe, it } from "vitest";
 import type { AggregatableKeys } from "./AggregatableKeys.js";
-import type { AggregationClause } from "./AggregationsClause.js";
+import type { UnorderedAggregationClause } from "./AggregationsClause.js";
 
 export type huh = AggregatableKeys<
   {
@@ -43,7 +43,7 @@ export type huh = AggregatableKeys<
   }["objects"]["Todo"]
 >;
 
-export type Q = AggregationClause<
+export type Q = UnorderedAggregationClause<
   {
     metadata: any;
     objects: {

diff --git a/packages/client.api/src/aggregate/AggregationsClause.ts b/packages/client.api/src/aggregate/AggregationsClause.ts
@@ -15,28 +15,11 @@
  */
 
 import type { ObjectOrInterfaceDefinition } from "@osdk/api";
-import type { AggregatableKeys } from "@osdk/client.api";
+import type { ValidAggregationKeys } from "@osdk/client.api";
 
-type StringAggregateOption = "approximateDistinct";
-type NumericAggregateOption =
-  | "min"
-  | "max"
-  | "sum"
-  | "avg"
-  | "approximateDistinct";
+export type UnorderedAggregationClause<Q extends ObjectOrInterfaceDefinition> =
+  { [AK in ValidAggregationKeys<Q>]?: "unordered" };
 
-type totalCountOption = { $count?: true };
-
-export type AggregationClause<
-  Q extends ObjectOrInterfaceDefinition,
-  K extends AggregatableKeys<Q> = AggregatableKeys<Q>,
-> =
-  & {
-    [P in K]?: Q["properties"][P]["type"] extends "string"
-      ? StringAggregateOption | StringAggregateOption[]
-      : Q["properties"][P]["type"] extends
-        "double" | "integer" | "float" | "decimal" | "byte" | "long" | "short"
-        ? NumericAggregateOption | NumericAggregateOption[]
-      : never;
-  }
-  & totalCountOption;
+export type OrderedAggregationClause<Q extends ObjectOrInterfaceDefinition> = {
+  [AK in ValidAggregationKeys<Q>]?: "unordered" | "asc" | "desc";
+};
diff --git a/packages/client.api/src/aggregate/AggregationsResults.test.ts b/packages/client.api/src/aggregate/AggregationsResults.test.ts
@@ -43,8 +43,8 @@ type T_AGG_RESULTS_TEST_1 = AggregationsResults<
   }["objects"]["Todo"],
   {
     $select: {
-      locationCity: "approximateDistinct";
-      text: "approximateDistinct";
+      "locationCity:approximateDistinct": "unordered";
+      "text:approximateDistinct": "unordered";
     };
     $groupBy: {
       text: "exact";
@@ -76,9 +76,10 @@ type Q = AggregationResultsWithoutGroups<
     queries: {};
   }["objects"]["Todo"],
   {
-    locationCity: "approximateDistinct";
-    id: ["max", "sum"];
-    text: "approximateDistinct";
+    "locationCity:approximateDistinct": "unordered";
+    "id:max": "unordered";
+    "id:sum": "unordered";
+    "text:approximateDistinct": "unordered";
   }
 >;