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(typedsql): support column & param nullability #4979

Merged
merged 24 commits into from
Aug 23, 2024

Conversation

Weakky
Copy link
Contributor

@Weakky Weakky commented Aug 12, 2024

Overview

Closes https://github.com/prisma/team-orm/issues/1258

  • Infer column and param nullability when possible, for Postgres, MySQL and SQLite.
  • Allow overriding nullability for params & columns
    • For params: with -- @param $1:alias? (param alias must be suffixed with ?)
    • For columns: with select id as "id?" from table(column alias must be suffixed with ?)
  • On Postgres, selected expressions that lead to ?column? as column name are forbidden and require an alias to be specified. This avoids clashing with the ? suffix heuristic to override nullability.

Included but not related to nullability:

  • Enum names can be referenced in -- @param {Type} magic docs to override inferred type
  • When SQLite does not infer a column type and we fallback to SQLx, we always treat integers as i64's

Copy link

codspeed-hq bot commented Aug 12, 2024

CodSpeed Performance Report

Merging #4979 will not alter performance

Comparing feat/typed-sql-nullability (aad0c68) with main (b5beb48)

Summary

✅ 11 untouched benchmarks

Copy link
Contributor

github-actions bot commented Aug 12, 2024

WASM Query Engine file Size

Engine This PR Base branch Diff
Postgres 2.063MiB 2.061MiB 2.001KiB
Postgres (gzip) 823.739KiB 822.827KiB 934.000B
Mysql 2.033MiB 2.032MiB 1.167KiB
Mysql (gzip) 811.288KiB 810.526KiB 780.000B
Sqlite 1.924MiB 1.926MiB -2.240KiB
Sqlite (gzip) 768.297KiB 769.614KiB -1.317KiB

let documentation = parse_rest(input)?;

let mut param = ParsedParameterDoc::default();

param.set_typ(typ);
param.set_nullable(nullable.then_some(true));
Copy link
Contributor Author

@Weakky Weakky Aug 23, 2024

Choose a reason for hiding this comment

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

We don't need nullable to be an Option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up keeping it as an Option. The rationale is that I'd like to keep the parser unopinionated. If it finds a ?, it's Some(true). Else, it's None. It's up to the introspect_sql implementation to give semantic value to this information, and .unwrap_or(default) with the correct default. I did simplify things by always returning None when ? is not present. Previously, we returned Some(false) when ? was not present.

Copy link
Member

@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.

Good job!

@Weakky Weakky marked this pull request as ready for review August 23, 2024 13:06
@Weakky Weakky requested a review from a team as a code owner August 23, 2024 13:06
@Weakky Weakky requested review from laplab and removed request for a team August 23, 2024 13:06
@Weakky Weakky added this to the 5.19.0 milestone Aug 23, 2024
@Weakky Weakky merged commit 8a3982b into main Aug 23, 2024
361 of 369 checks passed
@Weakky Weakky deleted the feat/typed-sql-nullability branch August 23, 2024 13:31
@Weakky Weakky changed the title feat(typedsql): support nullability feat(typedsql): support column & param nullability Aug 23, 2024
jkomyno added a commit that referenced this pull request Sep 5, 2024
* fix(driver-adapters): fix transaction ordering for ISOLATION LEVEL, impacting PlanetScale

* chore(driver-adapters): uncomment fixed PlanetScale tests

* chore: retrigger CI

* feat(driver-adapters): add support for TransactionContext

* test(connector-test-kit-rs): uncomment succeeding "basic_serializable" test

* chore(driver-adapters): fix types in executor

* DRIVER_ADAPTERS_BRANCH=feat-driver-adapters-transaction-context retrigger CI/CD

* feat(driver-adapters): impl Send on JsTransactionContext on wasm32

* DRIVER_ADAPTERS_BRANCH=feat-driver-adapters-transaction-context retrigger CI/CD

* feat(driver-adapters): impl FromJsValue for JsTransactionContext on wasm32

* DRIVER_ADAPTERS_BRANCH=feat-driver-adapters-transaction-context retrigger CI/CD

* feat(driver-adapters): impl Send for TransactionContextProxy on wasm32

* DRIVER_ADAPTERS_BRANCH=feat-driver-adapters-transaction-context retrigger CI/CD

* chore: attempt Send/Sync-compatibility for JS transactions

* chore: add "parse_raw_query" to "JsTransactionContext"

* fix(driver-adapters): enable wasm32 compilation of "JsQueryable::start_transaction"

* chore: remove commented-out method

* test(connector-test-kit-rs): uncomment "interactive_tx" tests

* DRIVER_ADAPTERS_BRANCH=feat/driver-adapters-transaction-context chore: retrigger CI/CD

* tmp(connector-test-kit-rs): add (failing) concurrent_create_select regression test

* Revert "tmp(connector-test-kit-rs): add (failing) concurrent_create_select regression test"

This reverts commit cc0baab.

* DRIVER_ADAPTERS_BRANCH=feat/driver-adapters-transaction-context chore: retrigger CI/CD

* feat(driver-adapters): rename "parse_raw_query" to "describe_query" after #4979

* DRIVER_ADAPTERS_BRANCH=feat/driver-adapters-transaction-context chore: retrigger CI/CD

* chore: don't leak concrete UnsafeFuture types

* chore(driver-adapters): nit, replace "::napi" with "napi"

* Revert "chore(driver-adapters): nit, replace "::napi" with "napi""

This reverts commit 808fdbe.

* DRIVER_ADAPTERS_BRANCH=feat/driver-adapters-transaction-context chore: retrigger CI/CD

* chore: unbox JsTransactionContext

* DRIVER_ADAPTERS_BRANCH=feat/driver-adapters-transaction-context chore: retrigger CI/CD
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.

2 participants