[QB] Optimize scalar cast maps #1268

CarsonF · 2025-04-21T15:15:00Z

`scalarCastableFrom`/`scalarAssignableBy`

This changes the scalarCastableFrom/scalarAssignableBy types to just pick the type based on the given type's FQN.
This is much easier for TS, esp with generated enums, as their members are statically written as well and TS was duck checking all of these.

- export type scalarAssignableBy<T extends $.ScalarType> =
- T extends _std.number ? _std.number :
- T extends _std.int64 ? _std.int64 :
- ...
+ export interface ScalarAssignableByMap {
+   "std::number": _std.$number;
+   "std::int64": _std.$int64;
+   ...
+ }

`getSharedParentScalar`

This also changes getSharedParentScalar to compare against the types' FQN.
This should be much more performant and still safe since these are scalar types with unique names.
We can also skip enumerating every case if A & B are the same.
This saves having to declare mapping for 80% of the scalars that can't be cast into another.

This removes 79 branches with my generated client, leaving only 6 specific scalar branches.
Before:

type getSharedParentScalar<A, B> =
  A extends _cal.$local_date ?
    B extends _cal.$local_datetime ?
      B
    :
    B extends _cal.$local_date ?
      B
    :
    never
  :
  A extends _std.$number ?
    B extends _std.$number ?
      B
    :
    never
  :
  A extends _sys.$VersionStage ?
    B extends _sys.$VersionStage ?
      B
    :
    never
  :
  ...

After:

type getSharedParentScalar<A, B> =
  AType extends $.ScalarType<infer AName> ?
  BType extends $.ScalarType<infer BName> ?
    AName extends "cal::local_date" ?
      BName extends "cal::local_datetime" ?
        B
      :
      BName extends "cal::local_date" ?
        B
      :
      never
    :
    ...
    AName extends BName ? A : never

Having scalars castable into others is the edge case. Optimize this type by checking if the two types are equal before checking the edge cases. This saves having to declare mapping for 80% of the scalars that can't be cast into another.

CarsonF · 2025-04-21T15:15:56Z

packages/generate/src/edgeql-js/generateCastMaps.ts


  const returnTypes = new Set<string>();

+  const uncastables = new Set(


Like uncrustables but saltier.
Open to different names.

Nope, this is the perfect name 👨‍🍳

CarsonF · 2025-04-21T15:17:24Z

packages/generate/src/edgeql-js/generateCastMaps.ts


    const outerCastableTo = casting(outer.id);
-    staticMap.writeln([t`  A extends ${getRef(outer.name)} ?`]);
+    if (!uncastables.has(outer)) {


All of these ifs feel dirty, but I understand wanting to keep the runtime logic next to compile time. But I'm not sure now if we should deviate since the compile time is changing its logic somewhat.

Oh, I'm perfectly happy to make this division more clear here vs in the generated code: we should prioritize the maintenance of this code over the understandability of the generated code. No change needed, but I'm 100% with you on wanting to branch harder here instead of littering if statements everywhere.

packages/generate/src/edgeql-js/generateCastMaps.ts

CarsonF · 2025-04-21T17:13:48Z

This is to prevent an excessive depth error, and it appears that the current TS benchmarks do not measure these depth calculations.

Copilot

Pull Request Overview

This PR optimizes the getSharedParentScalar type generation for the common "80%" case by short-circuiting comparisons when the two scalar types are equal and by excluding uncastable scalars.

Moves the equality check (IsEqual) to the top of the generated type definition.
Introduces an uncastables set to conditionally generate type branches and reduces the overall branch count.

packages/generate/src/edgeql-js/generateCastMaps.ts

CarsonF · 2025-04-21T17:33:26Z

Just tried v6 on my codebase again with this. This does fix the depth problem with this type alias, but assignableBy & castableFrom are still a problem.

scotttrinh

👑

scotttrinh · 2025-04-21T17:41:09Z

packages/generate/src/edgeql-js/generateCastMaps.ts


  const returnTypes = new Set<string>();

+  const uncastables = new Set(


Nope, this is the perfect name 👨‍🍳

scotttrinh · 2025-04-21T17:43:27Z

packages/generate/src/edgeql-js/generateCastMaps.ts


    const outerCastableTo = casting(outer.id);
-    staticMap.writeln([t`  A extends ${getRef(outer.name)} ?`]);
+    if (!uncastables.has(outer)) {


Oh, I'm perfectly happy to make this division more clear here vs in the generated code: we should prioritize the maintenance of this code over the understandability of the generated code. No change needed, but I'm 100% with you on wanting to branch harder here instead of littering if statements everywhere.

CarsonF · 2025-04-21T18:09:17Z

Thanks for the review @scotttrinh.
I think the remaining issues for me with assignableBy & castableFrom come from the need for TS look at each member of all my enums.

export type scalarCastableFrom<T extends $.ScalarType> =
  T extends _Project.$Status ? _Project.$Status :
  ...;

export type $Status = {
  "InDevelopment": $.$expr_Literal<$Status>;
  "Active": $.$expr_Literal<$Status>;
  "Terminated": $.$expr_Literal<$Status>;
  "Completed": $.$expr_Literal<$Status>;
  "DidNotDevelop": $.$expr_Literal<$Status>;
} & $.EnumType<"Project::Status", ["InDevelopment", "Active", "Terminated", "Completed", "DidNotDevelop"]>;

What do you think about only checking the FQNs instead?

export type scalarCastableFrom<T extends $.ScalarType> =
  T extends $.ScalarType<infer N> ?
    N extends "Project::Status" ? _Project.$Status :
    ...
  : never;

Actually following this train of thought we could just use a mapping instead.
Thoughts?
Is there a use case I'm not thinking of where only checking the name is insufficient?

This is `O(1)` compared to the previous `O(N)`. This is easier on TS esp for enums, as enums have all of their members declared statically and the previous impl was having TS check all of these members for duck compatibility.

CarsonF · 2025-04-21T19:17:04Z

Alright well if this truly is the direction, I should probably change getSharedParentScalar to just check names as well.

CarsonF · 2025-04-21T20:18:20Z

Ok I'm done here. These changes do fix all related problems in my codebase, blocking my v6 upgrade.
Perhaps a future PR could untangle what's here more.

Optimize getSharedParentScalar for the "80%" case

49f975e

Having scalars castable into others is the edge case. Optimize this type by checking if the two types are equal before checking the edge cases. This saves having to declare mapping for 80% of the scalars that can't be cast into another.

CarsonF force-pushed the optimize-cast-maps-shared-parent branch from 4c9de16 to 49f975e Compare April 21, 2025 15:15

CarsonF commented Apr 21, 2025

View reviewed changes

CarsonF requested review from Copilot and scotttrinh April 21, 2025 17:14

CarsonF marked this pull request as ready for review April 21, 2025 17:14

Copilot AI reviewed Apr 21, 2025

View reviewed changes

packages/generate/src/edgeql-js/generateCastMaps.ts Show resolved Hide resolved

packages/generate/src/edgeql-js/generateCastMaps.ts Show resolved Hide resolved

scotttrinh approved these changes Apr 21, 2025

View reviewed changes

CarsonF changed the title ~~Optimize getSharedParentScalar for the "80%" case~~ [QB] Optimize scalar cast maps Apr 21, 2025

CarsonF requested a review from scotttrinh April 21, 2025 19:00

CarsonF force-pushed the optimize-cast-maps-shared-parent branch 2 times, most recently from 0a34451 to dbddbfa Compare April 21, 2025 19:56

Refactor getSharedParentScalar to use type FQNs

50d5652

CarsonF force-pushed the optimize-cast-maps-shared-parent branch from dbddbfa to 50d5652 Compare April 21, 2025 20:05

scotttrinh approved these changes Apr 21, 2025

View reviewed changes

scotttrinh merged commit b43cdb8 into geldata:master Apr 21, 2025
10 checks passed

CarsonF deleted the optimize-cast-maps-shared-parent branch April 21, 2025 20:47


		const returnTypes = new Set<string>();

		const uncastables = new Set(

[QB] Optimize scalar cast maps #1268

[QB] Optimize scalar cast maps #1268

Uh oh!

Conversation

CarsonF commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

scalarCastableFrom/scalarAssignableBy

getSharedParentScalar

Uh oh!

CarsonF Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

scotttrinh Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

CarsonF Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

scotttrinh Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CarsonF commented Apr 21, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

CarsonF commented Apr 21, 2025

Uh oh!

scotttrinh left a comment

Choose a reason for hiding this comment

Uh oh!

scotttrinh Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

scotttrinh Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

CarsonF commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CarsonF commented Apr 21, 2025

Uh oh!

CarsonF commented Apr 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

CarsonF commented Apr 21, 2025 •

edited

Loading

`scalarCastableFrom`/`scalarAssignableBy`

`getSharedParentScalar`

CarsonF commented Apr 21, 2025 •

edited

Loading