[graphql] Replace `Int` with `BigInt` for overflow #26757

deepyaman · 2024-12-30T16:21:30Z

Summary & Motivation

Stems from an internal issue to identify and triage any of the places we use an int as an ID/cursor in Graphene (there was a situation in internal testing where the ID exceeded int limits, even if no users have hit this).

The issue mentions, "In general, any use case of Graphene.Integer is suspect", so I've gone ahead and commented on all current uses of graphene.Int (except in limits, which discussed with @OwenKephart should never go that high). The comments are all part of a separate commit fca93a3 and will be reverted before merging; they're just here to explain the decision for not changing things.

How I Tested These Changes

BK

deepyaman · 2024-12-30T20:42:34Z

python_modules/dagster-graphql/dagster_graphql/schema/instance.py

@@ -114,7 +114,7 @@ class GraphenePendingConcurrencyStep(graphene.ObjectType):
    stepKey = graphene.NonNull(graphene.String)
    enqueuedTimestamp = graphene.NonNull(graphene.Float)
    assignedTimestamp = graphene.Float()
-    priority = graphene.Int()
+    priority = graphene.Int()  # Can't imagine anybody needs to set BigInt priorities


Maybe should be a BigInt; sometimes people spam 9 key

deepyaman · 2024-12-30T20:42:55Z

python_modules/dagster-graphql/dagster_graphql/schema/asset_graph.py

@@ -225,7 +225,7 @@ class GrapheneAssetNode(graphene.ObjectType):
        non_null_list(GrapheneMaterializationEvent),
        partitions=graphene.List(graphene.NonNull(graphene.String)),
        beforeTimestampMillis=graphene.String(),
-        limit=graphene.Int(),
+        limit=graphene.BigInt(),


All limits should be ints, since they're what are returned to the frontend

deepyaman · 2024-12-30T20:43:52Z

python_modules/dagster-graphql/dagster_graphql/schema/instigation.py

@@ -536,25 +538,25 @@ class GrapheneInstigationState(graphene.ObjectType):
    typeSpecificData = graphene.Field(GrapheneInstigationTypeSpecificData)
    runs = graphene.Field(
        non_null_list("dagster_graphql.schema.pipelines.pipeline.GrapheneRun"),
-        limit=graphene.Int(),
+        limit=graphene.BigInt(),


Suggested change

limit=graphene.BigInt(),

limit=graphene.Int(),

deepyaman · 2024-12-30T20:45:03Z

python_modules/dagster-graphql/dagster_graphql/schema/instigation.py

    )
-    runsCount = graphene.NonNull(graphene.Int)
+    runsCount = graphene.NonNull(graphene.BigInt)


Won't be bigint number of runs on a single persons org

deepyaman · 2024-12-30T20:46:26Z

python_modules/dagster-graphql/dagster_graphql/schema/pipelines/config.py

@@ -26,7 +26,7 @@


 class GrapheneEvaluationStackListItemEntry(graphene.ObjectType):
-    list_index = graphene.NonNull(graphene.Int)
+    list_index = graphene.NonNull(graphene.BigInt)


Maybe a list index doesn't need billions? Hard to imagine

deepyaman · 2024-12-30T20:47:10Z

python_modules/dagster-graphql/dagster_graphql/schema/runs.py


    class Meta:
        name = "PipelineRuns"


 class GrapheneRuns(graphene.ObjectType):
    results = non_null_list("dagster_graphql.schema.pipelines.pipeline.GrapheneRun")
-    count = graphene.Int()
+    count = graphene.BigInt()


Won't be billions on a single account

deepyaman · 2024-12-30T20:47:17Z

python_modules/dagster-graphql/dagster_graphql/schema/runs.py

@@ -103,15 +103,15 @@ class Meta:

 class GraphenePipelineRuns(graphene.Interface):
    results = non_null_list("dagster_graphql.schema.pipelines.pipeline.GrapheneRun")
-    count = graphene.Int()
+    count = graphene.BigInt()


Won't be billions on a single account

deepyaman · 2024-12-30T20:47:31Z

python_modules/dagster-graphql/dagster_graphql/schema/runs_feed.py

@@ -41,7 +41,7 @@ class GrapheneRunsFeedCount(graphene.ObjectType):
    class Meta:
        name = "RunsFeedCount"

-    count = graphene.NonNull(graphene.Int)
+    count = graphene.NonNull(graphene.BigInt)


Won't be billions on a single account

deepyaman · 2024-12-31T16:59:39Z

python_modules/dagster-graphql/dagster_graphql/schema/asset_graph.py

+        startIdx=graphene.BigInt(),
+        endIdx=graphene.BigInt(),


Indices probably can't get into billions

This commit is only for review; will revert before merging anything.

github-actions · 2025-01-01T14:45:42Z

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-rhpvfph6t-elementl.vercel.app
https://deepyaman-chore-use-bigint.core-storybook.dagster-docs.io

Built with commit ce989dc.
This pull request is being automatically deployed with vercel-action

deepyaman · 2025-01-01T15:15:51Z

js_modules/dagster-ui/packages/ui-core/src/graphql/schema.graphql

  runId: String!
  timestamp: Float!
 }

+scalar BigInt


I feel like should be using something like https://the-guild.dev/graphql/scalars/docs/scalars/big-int (the automatically-generated typing seems too broad/wrong?), but most of the files say to not modify them directly, so I've just left the autogenerated types. Maybe somebody knowledgeable about the UI module and GraphQL can weigh in?

deepyaman · 2025-01-01T15:22:32Z

@gibsondan I added you as a suggested reviewer, and somebody who had recently touched these files; if there's somebody better to add who is familiar with the UI side, please feel free to assign them instead!

gibsondan

I think BigInt essentially does not work in Javascript, you run into integer rounding issues causing it to change values and start rounding to the nearest 1000

So IMO the mission here is to change these to graphene.ID where needed (which is a string), not graphene.BigInt

gibsondan · 2025-01-02T16:34:26Z

python_modules/dagster-graphql/dagster_graphql/schema/instance.py

-    priority = graphene.Int()
+    priority = (
+        graphene.BigInt()
+    )  # Maybe somebody will spam hold down the 9 key on their keyboard ¯\_(ツ)_/¯


we actually enforce this in Python, see https://github.com/dagster-io/dagster/pull/25172/files

deepyaman · 2025-01-02T19:34:59Z

I think BigInt essentially does not work in Javascript, you run into integer rounding issues causing it to change values and start rounding to the nearest 1000

So IMO the mission here is to change these to graphene.ID where needed (which is a string), not graphene.BigInt

As in e9e61f1?

@gibsondan Makes sense. Then, I'll not change priority, but for storageId I'll use graphene.ID, and for the GrapheneCursor... I'll need to look into it, but maybe that may also be graphene.ID?

gibsondan · 2025-01-02T19:40:34Z

Yeah the tricky thing there is that it then becomes a string, which could be a breaking change for some APIs

…

On Thu, Jan 2, 2025 at 1:35 PM Deepyaman Datta ***@***.***> wrote: I think BigInt essentially does not work in Javascript, you run into integer rounding issues causing it to change values and start rounding to the nearest 1000 So IMO the mission here is to change these to graphene.ID where needed (which is a string), not graphene.BigInt As in e9e61f1 <e9e61f1> ? @gibsondan <https://github.com/gibsondan> Makes sense. Then, I'll not change priority, but for storageId I'll use graphene.ID, and for the GrapheneCursor... I'll need to look into it, but maybe that may also be graphene.ID? — Reply to this email directly, view it on GitHub <#26757 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACAPJC72TA4E7AFAM7UJWD32IWIHTAVCNFSM6AAAAABUMIRYMOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRYGI3TINJVGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

deepyaman commented Dec 30, 2024

View reviewed changes

deepyaman commented Dec 31, 2024

View reviewed changes

deepyaman force-pushed the deepyaman/chore/use-bigint branch 2 times, most recently from 4d11713 to 2f130cb Compare December 31, 2024 17:13

[graphql] Replace Int with BigInt for overflow

66611c4

deepyaman force-pushed the deepyaman/chore/use-bigint branch from 2f130cb to 09ab231 Compare December 31, 2024 17:16

Add explanations (except limits are always Ints)

fca93a3

This commit is only for review; will revert before merging anything.

deepyaman force-pushed the deepyaman/chore/use-bigint branch from 09ab231 to fca93a3 Compare December 31, 2024 17:18

Generate after making changes to GraphQL fragments

ce989dc

deepyaman commented Jan 1, 2025

View reviewed changes

deepyaman requested review from OwenKephart and gibsondan January 1, 2025 15:17

deepyaman marked this pull request as ready for review January 1, 2025 15:21

gibsondan requested changes Jan 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[graphql] Replace `Int` with `BigInt` for overflow #26757

[graphql] Replace `Int` with `BigInt` for overflow #26757

deepyaman commented Dec 30, 2024 •

edited

Loading

deepyaman Dec 30, 2024

deepyaman Dec 30, 2024

deepyaman Dec 30, 2024

deepyaman Dec 30, 2024

deepyaman Dec 30, 2024

deepyaman Dec 30, 2024

deepyaman Dec 30, 2024

deepyaman Dec 30, 2024

deepyaman Dec 31, 2024

github-actions bot commented Jan 1, 2025

deepyaman Jan 1, 2025

deepyaman commented Jan 1, 2025

gibsondan left a comment

gibsondan Jan 2, 2025

deepyaman commented Jan 2, 2025

gibsondan commented Jan 2, 2025 via email

[graphql] Replace Int with BigInt for overflow #26757

Are you sure you want to change the base?

[graphql] Replace Int with BigInt for overflow #26757

Conversation

deepyaman commented Dec 30, 2024 • edited Loading

Summary & Motivation

How I Tested These Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 1, 2025

Choose a reason for hiding this comment

deepyaman commented Jan 1, 2025

gibsondan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepyaman commented Jan 2, 2025

gibsondan commented Jan 2, 2025 via email

[graphql] Replace `Int` with `BigInt` for overflow #26757

[graphql] Replace `Int` with `BigInt` for overflow #26757

deepyaman commented Dec 30, 2024 •

edited

Loading