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: include field native type attributes data to DMMF #5009

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

pasha-vuiko
Copy link

@pasha-vuiko pasha-vuiko commented Sep 29, 2024

Hi!
This PR exposes schema field native type data to DMMF.

Very long awaited feature for many people and me 🙂

@pasha-vuiko pasha-vuiko requested a review from a team as a code owner September 29, 2024 20:23
@pasha-vuiko pasha-vuiko requested review from laplab and removed request for a team September 29, 2024 20:23
@CLAassistant
Copy link

CLAassistant commented Sep 29, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

codspeed-hq bot commented Nov 4, 2024

CodSpeed Performance Report

Merging #5009 will not alter performance

Comparing pasha-vuiko:feat/include-field-native-type-data-to-dmmf (f616ccc) with main (29bc8ca)

Summary

✅ 11 untouched benchmarks

@aqrln aqrln added this to the 6.0.0 milestone Nov 4, 2024
@aqrln aqrln requested review from aqrln and removed request for laplab November 4, 2024 11:52
@jkomyno
Copy link
Contributor

jkomyno commented Nov 5, 2024

Hi @pasha-vuiko, thanks for this PR!
Can we please ask you to fix the clippy tests?

@pasha-vuiko
Copy link
Author

Yep, I'll try to fix them today

@pasha-vuiko pasha-vuiko force-pushed the feat/include-field-native-type-data-to-dmmf branch from 0e75748 to 923cd6b Compare November 5, 2024 20:33
@pasha-vuiko
Copy link
Author

jkomyno I fixed Clippy warnings and DMMF generation related unit tests

@pasha-vuiko
Copy link
Author

sweeeet, so now I'm supposed to update 137mb JSON to make the fallen unit tests work?😅

@aqrln
Copy link
Member

aqrln commented Nov 14, 2024

sweeeet, so now I'm supposed to update 137mb JSON to make the fallen unit tests work?😅

You just need to run the tests with UPDATE_EXPECT=1 env var set

@aqrln
Copy link
Member

aqrln commented Nov 14, 2024

UPDATE_EXPECT=1 cargo test -p dmmf -F psl/all — something like this

@pasha-vuiko
Copy link
Author

@aqrln thank you so much for your suggestion, I pushed the new commit

@aqrln
Copy link
Member

aqrln commented Nov 15, 2024

Nice, this looks good to me. Would you be willing to update the TypeScript side as well (type definitions + snapshots in tests)?

@pasha-vuiko
Copy link
Author

yes, I already created the PR in the Prisma repo
prisma/prisma#25324
Currently it has some type related errors on test run, because it generates JSON files based on old version of prisma-engines (without the DMMF native type exposing) so what should I do next? Is it possible to somehow 'point' the prisma TS part to Rust part from the current PR?

@aqrln aqrln mentioned this pull request Nov 15, 2024
@aqrln
Copy link
Member

aqrln commented Nov 15, 2024

Is it possible to somehow 'point' the prisma TS part to Rust part from the current PR?

It's a bit convoluted with forks. I created #5045 to release an integration version of the engines that you can use. An automated draft PR updating the engines to this version should appear in the main repo in a bit.

We should probably make the /engines-branch command in client PRs support forked repos, as that is a much better workflow.

@pasha-vuiko
Copy link
Author

excuse me, I'm a bit confused, what should I do next to make the tests work with new version prisma-engines locally and in the pipeline?

@aqrln
Copy link
Member

aqrln commented Nov 15, 2024

You can cherry-pick the commit from prisma/prisma#25653 in the client repo.

Locally you can also set export PRISMA_QUERY_ENGINE_LIBRARY=/absolute/path/to/target/debug/libquery_engine.so and export PRISMA_SCHEMA_ENGINE_BINARY=/.../target/debug/schema-engine instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants