Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix legacy long aggregations #523

Merged
merged 12 commits into from
Jul 25, 2024
8 changes: 8 additions & 0 deletions .changeset/silent-carpets-kick.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"@osdk/e2e.generated.1.1.x": minor
"@osdk/legacy-client": minor
"@osdk/shared.test": minor
"@osdk/generator": minor
---

Fix long aggregations in legacy-client
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type {
GeoShape,
LocalDate,
OntologyObject,
StringLong,
TimeSeries,
Timestamp,
} from '@osdk/legacy-client';
Expand Down Expand Up @@ -42,8 +43,8 @@ export interface ObjectTypeWithAllPropertyTypes extends OntologyObject {
readonly id: number | undefined;
readonly integer: number | undefined;
readonly integerArray: number[] | undefined;
readonly long: string | undefined;
readonly longArray: string[] | undefined;
readonly long: StringLong | undefined;
readonly longArray: StringLong[] | undefined;
readonly numericTimeseries: TimeSeries<number> | undefined;
readonly short: number | undefined;
readonly shortArray: number[] | undefined;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export function wireObjectTypeV2ToObjectInterfaceStringV1(
a !== objectTypeWithLinks.objectType.apiName
),
);
return `import type { OntologyObject, LocalDate, Timestamp, GeoShape, GeoPoint, Attachment, TimeSeries, MultiLink, SingleLink } from "@osdk/legacy-client";
return `import type { OntologyObject, LocalDate, Timestamp, GeoShape, GeoPoint, Attachment, TimeSeries, MultiLink, SingleLink, StringLong } from "@osdk/legacy-client";
${
Array.from(uniqueLinkTargets).map(linkTarget =>
`import type { ${linkTarget} } from "./${linkTarget}${importExt}";`
Expand Down Expand Up @@ -128,7 +128,7 @@ function wirePropertyTypeV2ToTypeScriptType(
case "geoshape":
return "GeoShape";
case "long":
return "string";
return "StringLong";
case "short":
return "number";
case "timestamp":
Expand Down
3 changes: 2 additions & 1 deletion packages/legacy-client/src/client/OsdkLegacyObject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import type {
Timestamp,
} from "./baseTypes/index.js";
import type { reservedKeywordsList } from "./utils/reservedKeywords.js";
import type { StringLong } from "./utils/StringLong.js";

export interface ValidLegacyPropertyTypes {
string: string;
Expand All @@ -45,7 +46,7 @@ export interface ValidLegacyPropertyTypes {
integer: number;
timestamp: Timestamp;
short: number;
long: string;
long: StringLong;
float: number;
decimal: string;
byte: number;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ describe("Aggregations", () => {
class: StringGroupBy<"class">;
class_: StringGroupBy<"class_">;
tags: StringGroupBy<"tags">;
unixTimestamp: NumericGroupBy<"unixTimestamp">;
}>().toMatchTypeOf<ObjectSetGroupByArg<Todo>>();

expectTypeOf<{
Expand All @@ -51,6 +52,7 @@ describe("Aggregations", () => {
points: AggregatableProperty<number>;
class: AggregatableProperty<number>;
class_: AggregatableProperty<number>;
unixTimestamp: AggregatableProperty<number>;
}>().toMatchTypeOf<ObjectSetAggregateArg<Todo>>();

expectTypeOf<{
Expand All @@ -61,6 +63,7 @@ describe("Aggregations", () => {
class: ApproximateDistinctCountAggregatableProperty;
class_: ApproximateDistinctCountAggregatableProperty;
count: () => CountOperation;
unixTimestamp: MultipleAggregatableProperty<number>;
}>().toMatchTypeOf<ObjectSetMultipleAggregateArg<Todo>>();
});
});
7 changes: 5 additions & 2 deletions packages/legacy-client/src/client/interfaces/aggregations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import type {
StringGroupBy,
TimestampGroupBy,
} from "../objectSets/aggregations/index.js";
import type { StringLong } from "../utils/StringLong.js";
import type { OmitMetadataProperties } from "./utils/OmitProperties.js";

export declare type ObjectTypesGroupByFunction<
Expand Down Expand Up @@ -111,15 +112,17 @@ export type GroupByFromType<T, N extends string> = NonNullable<T> extends number
: T extends Array<infer U> ? GroupByFromType<U, N>
: never;

export type AggregationFromType<T> = NonNullable<T> extends number
export type AggregationFromType<T> = NonNullable<T> extends StringLong
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main change that adds numerical aggregation support back to long-type properties

? AggregatableProperty<Double>
: NonNullable<T> extends number ? AggregatableProperty<Double>
: NonNullable<T> extends boolean ? AggregatableProperty<Double>
: NonNullable<T> extends LocalDate ? AggregatableProperty<LocalDate>
: NonNullable<T> extends Timestamp ? AggregatableProperty<Timestamp>
: AggregatableProperty<never>;

export type MultipleAggregationFromType<T> = NonNullable<T> extends number
export type MultipleAggregationFromType<T> = NonNullable<T> extends StringLong
? MultipleAggregatableProperty<Double>
: NonNullable<T> extends number ? MultipleAggregatableProperty<Double>
: NonNullable<T> extends boolean ? MultipleAggregatableProperty<Double>
: NonNullable<T> extends LocalDate ? MultipleAggregatableProperty<LocalDate>
: NonNullable<T> extends Timestamp ? MultipleAggregatableProperty<Timestamp>
Expand Down
27 changes: 22 additions & 5 deletions packages/legacy-client/src/client/objectSets/OsdkObjectSet.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,20 @@ describe("OsdkObjectSet", () => {
);
});

it("creates the max aggregation for a long property", async () => {
const os = createBaseObjectSet(client, "Todo");
mockAggregateResponse({ data: [], accuracy: "ACCURATE" });
await os.max(s => s.unixTimestamp).compute();
expect(fetch).toHaveBeenCalledOnce();
expect(fetch).toHaveBeenCalledWith(
...expectedJestResponse("Ontology/objectSets/aggregate", {
objectSet: { type: "base", objectType: "Todo" },
groupBy: [],
aggregation: [{ type: "max", name: "max", field: "unixTimestamp" }],
}),
);
});

it("creates the groupBy clauses", async () => {
const os = createBaseObjectSet(client, "Todo");
mockAggregateResponse({ data: [], accuracy: "ACCURATE" });
Expand All @@ -198,17 +212,20 @@ describe("OsdkObjectSet", () => {
await (os.aggregate(b => ({
foo: b.complete.approximateDistinct(),
bar: b.body.approximateDistinct(),
baz: b.unixTimestamp.avg(),
qux: b.unixTimestamp.approximateDistinct(),
})).compute());
expect(fetch).toHaveBeenCalledOnce();
expect(fetch).toHaveBeenCalledWith(
...expectedJestResponse("Ontology/objectSets/aggregate", {
objectSet: { type: "base", objectType: "Todo" },
groupBy: [],
aggregation: [{
type: "approximateDistinct",
name: "foo",
field: "complete",
}, { type: "approximateDistinct", name: "bar", field: "body" }],
aggregation: [
{ type: "approximateDistinct", name: "foo", field: "complete" },
{ type: "approximateDistinct", name: "bar", field: "body" },
{ type: "avg", name: "baz", field: "unixTimestamp" },
{ type: "approximateDistinct", name: "qux", field: "unixTimestamp" },
],
}),
);
});
Expand Down
24 changes: 24 additions & 0 deletions packages/legacy-client/src/client/utils/StringLong.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* Copyright 2024 Palantir Technologies, Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

declare const representsLong: unique symbol;

/** Represents a long property value converted to a string. We use a
* tagged type to distinguish from a regular string property value.
* This type is not used in `ValidLegacyBaseQueryDataTypes` or
* `ValidLegacyActionParameterTypes` because it is just for result
* long values, not long values that are passed into queries and actions. */
export type StringLong = string & { [representsLong]: true };
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used a tagged type to disambiguate between string-type properties and strings that represent long-type properties

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we leave a comment in the code here to explain this?

1 change: 1 addition & 0 deletions packages/legacy-client/src/client/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,5 @@
export type { IsEmptyRecord } from "./IsEmptyRecord.js";
export type { NonNullableKeys, NullableKeys } from "./NullableKeys.js";
export type { reservedKeywordsList } from "./reservedKeywords.js";
export type { StringLong } from "./StringLong.js";
export type { ValuesOfMap } from "./ValuesOfMap.js";
1 change: 1 addition & 0 deletions packages/legacy-client/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,7 @@ export type {
SingleLink,
StartsWithWhereClause,
StaticObjectSetDefinition,
StringLong,
SubtractObjectSetDefinition,
ThreeDimensionalAggregation,
ThreeDimensionalAggregationType,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import type {
OntologyObject,
Timestamp,
} from "../../client/baseTypes/index.js";
import type { StringLong } from "../../client/index.js";

export interface ObjectTypeWithAllPropertyTypes extends OntologyObject {
readonly __apiName: "ObjectTypeWithAllPropertyTypes";
Expand All @@ -33,7 +34,7 @@ export interface ObjectTypeWithAllPropertyTypes extends OntologyObject {
readonly dateTime: Timestamp | undefined;
readonly decimal: string | undefined;
readonly integer: number | undefined;
readonly long: string | undefined;
readonly long: StringLong | undefined;
readonly short: number | undefined;
readonly float: number | undefined;
readonly double: number | undefined;
Expand Down
2 changes: 2 additions & 0 deletions packages/legacy-client/src/util/test/TodoObject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import type {
OntologyObject,
SingleLink,
} from "../../client/baseTypes/index.js";
import type { StringLong } from "../../client/index.js";
import type { Task } from "./TaskObject.js";

/**
Expand All @@ -42,4 +43,5 @@ export interface Todo extends OntologyObject {
readonly points: number | undefined;
readonly tags: string[] | undefined;
readonly linkedTask: SingleLink<Task>;
readonly unixTimestamp: StringLong | undefined;
}
2 changes: 2 additions & 0 deletions packages/shared.test/src/mock-ontology/mockOntology.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ const Todo: TodoDef = {
complete: { type: "boolean", nullable: true },
tags: { type: "string", multiplicity: true, nullable: true },
points: { type: "integer", nullable: true },
unixTimestamp: { type: "long", nullable: true },
},
links: {
linkedTask: {
Expand All @@ -83,6 +84,7 @@ interface TodoDef extends ObjectTypeDefinition<"Todo">, VersionBound<"0.15.0"> {
complete: { type: "boolean"; nullable: true };
tags: { type: "string"; multiplicity: true; nullable: true };
points: { type: "integer"; nullable: true };
unixTimestamp: { type: "long"; nullable: true };
};
links: {
linkedTask: ObjectTypeLinkDefinition<TaskDef, false>;
Expand Down