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

feat(driver-adapters): fix bigint handling on creation and filter #4648

Merged
merged 7 commits into from
Jan 22, 2024

Conversation

jkomyno
Copy link
Contributor

@jkomyno jkomyno commented Jan 16, 2024

@jkomyno jkomyno added this to the 5.9.0 milestone Jan 16, 2024
@jkomyno jkomyno self-assigned this Jan 16, 2024
@jkomyno jkomyno marked this pull request as ready for review January 16, 2024 11:59
@jkomyno jkomyno requested a review from a team as a code owner January 16, 2024 11:59
@jkomyno jkomyno requested review from miguelff and Weakky and removed request for a team January 16, 2024 11:59
Copy link
Contributor

github-actions bot commented Jan 16, 2024

WASM Size

Engine This PR Base branch Diff
WASM 2.338MiB 2.338MiB 2.000B
WASM (gzip) 907.885KiB 907.788KiB 100.000B

Copy link
Contributor

github-actions bot commented Jan 16, 2024

✅ WASM query-engine: no benchmarks have regressed

Full benchmark report
DATABASE_URL="postgresql://postgres:postgres@localhost:5432/bench?schema=imdb_bench&sslmode=disable" \
node --experimental-wasm-modules query-engine/driver-adapters/executor/dist/bench.mjs

After changes in 57c5acb

Copy link

codspeed-hq bot commented Jan 16, 2024

CodSpeed Performance Report

Merging #4648 will not alter performance

Comparing fix/driver-adapters-bigint (57c5acb) with main (9c3f205)

Summary

✅ 11 untouched benchmarks

@jkomyno
Copy link
Contributor Author

jkomyno commented Jan 16, 2024

Note: planetscale (wasm) is failing for reasons seemingly independent of this PR.

@jkomyno
Copy link
Contributor Author

jkomyno commented Jan 16, 2024

Note: planetscale (wasm) is failing for reasons seemingly independent of this PR.

I understood the problem, for which I don't have yet a solution: on PlanetScale, bigint values are placed into queries as strings, resulting in SQL snippets like

LIMIT '1' OFFSET '0'

which obviously trigger syntax errors.

@SevInf
Copy link
Contributor

SevInf commented Jan 16, 2024

Note: planetscale (wasm) is failing for reasons seemingly independent of this PR.

I understood the problem, for which I don't have yet a solution: on PlanetScale, bigint values are placed into queries as strings, resulting in SQL snippets like

LIMIT '1' OFFSET '0'

which obviously trigger syntax errors.

Interesting. Should we report it to planetscale maybe?

Copy link
Contributor

@SevInf SevInf left a comment

Choose a reason for hiding this comment

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

LGTM when tests are green.
Maybe, skip Plantescale tests back and open follow-up issue?

@jkomyno
Copy link
Contributor Author

jkomyno commented Jan 17, 2024

LGTM when tests are green. Maybe, skip Plantescale tests back and open follow-up issue?

Yeah, I'm thinking the same. I've created https://github.com/prisma/team-orm/issues/836, and excluded PlanetScale. I'll let the CI run, and merge afterwards if it's all good

@jkomyno
Copy link
Contributor Author

jkomyno commented Jan 17, 2024

LGTM when tests are green. Maybe, skip Plantescale tests back and open follow-up issue?

Yeah, I'm thinking the same. I've created prisma/team-orm#836, and excluded PlanetScale. I'll let the CI run, and merge afterwards if it's all good

PlanetScale still failing, I'll keep exploring why.

SevInf pushed a commit that referenced this pull request Jan 18, 2024
Needs #4648 to be merged into main before, but all raw queries isses
are already fixed either by me or Alberto, so we have to just unskip the
tests.

Close prisma/team-orm#659
@jkomyno
Copy link
Contributor Author

jkomyno commented Jan 18, 2024

I've opened a PR on PlanetScale to fix this: planetscale/database-js#159.

@jkomyno jkomyno merged commit 53daea5 into main Jan 22, 2024
114 checks passed
@jkomyno jkomyno deleted the fix/driver-adapters-bigint branch January 22, 2024 11:04
SevInf pushed a commit that referenced this pull request Jan 23, 2024
Needs #4648 to be merged into main before, but all raw queries isses
are already fixed either by me or Alberto, so we have to just unskip the
tests.

Close prisma/team-orm#659
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants