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

qe: Fix nested objects with $type key in JSON protocol #4668

Closed
wants to merge 6 commits into from

Conversation

SevInf
Copy link
Contributor

@SevInf SevInf commented Jan 23, 2024

In JSON protocol, we use $type key as a special marker for the types
that can not be represented in JSON natively. We did not want to make
$type identifier reserved, so we took some precautions: if during
encoding, at any point, $type key would be seen in user's input we
encode whole object as {"$type": "Json", "value": [Serialized object] }.

Engine handled those cases correctly, when $type: Json is encountered
at the top level, but not when it is nested. In that case,
ArgumentValue-to-JSON serialization just picked value key and
saved it as a string.

Fixed by moving Json value into it's own kind of ArgumentValue, that
are serialized to json as-is.

Fix prisma/prisma#21454

@SevInf SevInf added this to the 5.9.0 milestone Jan 23, 2024
Copy link
Contributor

github-actions bot commented Jan 23, 2024

WASM Size

Engine This PR Base branch Diff
WASM 2.337MiB 2.329MiB 8.020KiB
WASM (gzip) 909.170KiB 906.378KiB 2.792KiB

Copy link

codspeed-hq bot commented Jan 23, 2024

CodSpeed Performance Report

Merging #4668 will not alter performance

Comparing fix/nested-json-dollar-type (be1aa17) with main (c2e5bf2)

Summary

✅ 11 untouched benchmarks

Copy link
Contributor

github-actions bot commented Jan 23, 2024

✅ WASM query-engine performance won't change substantially (0.985x)

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
cpu: AMD EPYC 7763 64-Core Processor
runtime: node v18.19.0 (x64-linux)

benchmark                   time (avg)             (min … max)       p75       p99      p999
-------------------------------------------------------------- -----------------------------
• movies.findMany() (all - 25000)
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline     314 ms/iter (309.68 ms … 327.76 ms) 315.35 ms 327.76 ms 327.76 ms
Web Assembly: Latest    375.98 ms/iter (374.41 ms … 380.61 ms)  376.5 ms 380.61 ms 380.61 ms
Web Assembly: Current   371.68 ms/iter  (370.37 ms … 374.7 ms) 372.09 ms  374.7 ms  374.7 ms
Node API: Current       238.26 ms/iter (226.39 ms … 249.57 ms) 249.28 ms 249.57 ms 249.57 ms

summary for movies.findMany() (all - 25000)
  Web Assembly: Current
   1.56x slower than Node API: Current
   1.18x slower than Web Assembly: Baseline
   1.01x faster than Web Assembly: Latest

• movies.findMany({ take: 2000 })
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline   12.53 ms/iter   (12.19 ms … 13.65 ms)  12.59 ms  13.65 ms  13.65 ms
Web Assembly: Latest     15.96 ms/iter    (15.15 ms … 22.1 ms)  15.82 ms   22.1 ms   22.1 ms
Web Assembly: Current     15.1 ms/iter   (14.82 ms … 16.42 ms)  15.13 ms  16.42 ms  16.42 ms
Node API: Current        9,090 µs/iter   (8,858 µs … 9,460 µs)  9,196 µs  9,460 µs  9,460 µs

summary for movies.findMany({ take: 2000 })
  Web Assembly: Current
   1.66x slower than Node API: Current
   1.2x slower than Web Assembly: Baseline
   1.06x faster than Web Assembly: Latest

• movies.findMany({ where: {...}, take: 2000 })
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline   1,962 µs/iter   (1,826 µs … 3,353 µs)  1,926 µs  3,180 µs  3,353 µs
Web Assembly: Latest     2,398 µs/iter   (2,297 µs … 3,446 µs)  2,401 µs  2,957 µs  3,446 µs
Web Assembly: Current    2,339 µs/iter   (2,258 µs … 2,889 µs)  2,347 µs  2,627 µs  2,889 µs
Node API: Current        1,524 µs/iter   (1,425 µs … 1,894 µs)  1,534 µs  1,839 µs  1,894 µs

summary for movies.findMany({ where: {...}, take: 2000 })
  Web Assembly: Current
   1.53x slower than Node API: Current
   1.19x slower than Web Assembly: Baseline
   1.03x faster than Web Assembly: Latest

• movies.findMany({ include: { cast: true } take: 2000 }) (m2m)
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline   12.56 ms/iter    (12.19 ms … 14.2 ms)  12.61 ms   14.2 ms   14.2 ms
Web Assembly: Latest     15.32 ms/iter   (15.14 ms … 15.85 ms)   15.4 ms  15.85 ms  15.85 ms
Web Assembly: Current    15.04 ms/iter   (14.81 ms … 16.08 ms)  15.09 ms  16.08 ms  16.08 ms
Node API: Current        9,256 µs/iter   (8,839 µs … 9,857 µs)  9,439 µs  9,857 µs  9,857 µs

summary for movies.findMany({ include: { cast: true } take: 2000 }) (m2m)
  Web Assembly: Current
   1.62x slower than Node API: Current
   1.2x slower than Web Assembly: Baseline
   1.02x faster than Web Assembly: Latest

• movies.findMany({ where: {...}, include: { cast: true } take: 2000 }) (m2m)
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline   1,928 µs/iter   (1,833 µs … 2,795 µs)  1,906 µs  2,522 µs  2,795 µs
Web Assembly: Latest     2,377 µs/iter   (2,289 µs … 3,391 µs)  2,378 µs  2,872 µs  3,391 µs
Web Assembly: Current    2,355 µs/iter   (2,248 µs … 3,168 µs)  2,336 µs  3,089 µs  3,168 µs
Node API: Current        1,564 µs/iter   (1,449 µs … 2,608 µs)  1,545 µs  2,241 µs  2,608 µs

summary for movies.findMany({ where: {...}, include: { cast: true } take: 2000 }) (m2m)
  Web Assembly: Current
   1.51x slower than Node API: Current
   1.22x slower than Web Assembly: Baseline
   1.01x faster than Web Assembly: Latest

• movies.findMany({ take: 2000, include: { cast: { include: { person: true } } } })
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline   12.35 ms/iter   (12.17 ms … 12.68 ms)  12.42 ms  12.68 ms  12.68 ms
Web Assembly: Latest     15.41 ms/iter    (15.1 ms … 16.97 ms)  15.45 ms  16.97 ms  16.97 ms
Web Assembly: Current    15.06 ms/iter   (14.86 ms … 15.37 ms)  15.16 ms  15.37 ms  15.37 ms
Node API: Current        9,094 µs/iter   (8,833 µs … 9,449 µs)  9,163 µs  9,449 µs  9,449 µs

summary for movies.findMany({ take: 2000, include: { cast: { include: { person: true } } } })
  Web Assembly: Current
   1.66x slower than Node API: Current
   1.22x slower than Web Assembly: Baseline
   1.02x faster than Web Assembly: Latest

• movie.findMany({ where: { ... }, take: 2000, include: { cast: { include: { person: true } } } })
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline   1,938 µs/iter   (1,819 µs … 3,288 µs)  1,931 µs  2,805 µs  3,288 µs
Web Assembly: Latest     2,366 µs/iter   (2,290 µs … 2,965 µs)  2,366 µs  2,818 µs  2,965 µs
Web Assembly: Current    2,342 µs/iter   (2,248 µs … 2,840 µs)  2,341 µs  2,703 µs  2,840 µs
Node API: Current        1,538 µs/iter   (1,431 µs … 1,889 µs)  1,556 µs  1,840 µs  1,889 µs

summary for movie.findMany({ where: { ... }, take: 2000, include: { cast: { include: { person: true } } } })
  Web Assembly: Current
   1.52x slower than Node API: Current
   1.21x slower than Web Assembly: Baseline
   1.01x faster than Web Assembly: Latest

• movie.findMany({ where: { reviews: { author: { ... } }, take: 100 }) (to-many -> to-one)
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline  940.03 µs/iter   (878.6 µs … 1,562 µs) 938.58 µs  1,413 µs  1,562 µs
Web Assembly: Latest     1,172 µs/iter   (1,115 µs … 1,817 µs)  1,171 µs  1,492 µs  1,817 µs
Web Assembly: Current    1,180 µs/iter   (1,113 µs … 1,828 µs)  1,177 µs  1,630 µs  1,828 µs
Node API: Current       835.35 µs/iter  (749.84 µs … 1,062 µs) 853.52 µs  994.2 µs  1,062 µs

summary for movie.findMany({ where: { reviews: { author: { ... } }, take: 100 }) (to-many -> to-one)
  Web Assembly: Current
   1.41x slower than Node API: Current
   1.26x slower than Web Assembly: Baseline
   1.01x slower than Web Assembly: Latest

• movie.findMany({ where: { cast: { person: { ... } }, take: 100 }) (m2m -> to-one)
-------------------------------------------------------------- -----------------------------
Web Assembly: Baseline  898.74 µs/iter  (854.09 µs … 1,238 µs) 904.11 µs  1,179 µs  1,238 µs
Web Assembly: Latest     1,165 µs/iter   (1,114 µs … 1,941 µs)  1,164 µs  1,788 µs  1,941 µs
Web Assembly: Current    1,183 µs/iter   (1,119 µs … 1,915 µs)  1,178 µs  1,705 µs  1,915 µs
Node API: Current        832.8 µs/iter  (762.63 µs … 1,624 µs) 846.47 µs  1,018 µs  1,624 µs

summary for movie.findMany({ where: { cast: { person: { ... } }, take: 100 }) (m2m -> to-one)
  Web Assembly: Current
   1.42x slower than Node API: Current
   1.32x slower than Web Assembly: Baseline
   1.02x slower than Web Assembly: Latest

After changes in be1aa17

@SevInf SevInf requested a review from Weakky January 24, 2024 11:36
@SevInf SevInf marked this pull request as ready for review January 24, 2024 11:36
@SevInf SevInf requested a review from a team as a code owner January 24, 2024 11:36
@SevInf SevInf requested review from miguelff and removed request for a team January 24, 2024 11:36
@SevInf SevInf force-pushed the fix/nested-json-dollar-type branch from 4aafb68 to 6f1eab4 Compare January 24, 2024 11:39
SevInf pushed a commit that referenced this pull request Jan 24, 2024
Introduce another special value to JSON protocol, `"$type": "Raw"`.
When encountered, no other nested `$type` keys would be interpreted as
special and will be written to DB as is. Main usecase is JSON column
values with user-provided `$type` keys.

This is an alternative to #4668, that might look cleaner on the engine
side.

Part of the fix for prisma/prisma#21454, will require client adjustments
as well.
SevInf pushed a commit that referenced this pull request Jan 24, 2024
Introduce another special value to JSON protocol, `"$type": "Raw"`.
When encountered, no other nested `$type` keys would be interpreted as
special and will be written to DB as is. Main usecase is JSON column
values with user-provided `$type` keys.

This is an alternative to #4668, that might look cleaner on the engine
side.

Part of the fix for prisma/prisma#21454, will require client adjustments
as well.
SevInf pushed a commit that referenced this pull request Jan 24, 2024
Introduce another special value to JSON protocol, `"$type": "Raw"`.
When encountered, no other nested `$type` keys would be interpreted as
special and will be written to DB as is. Main usecase is JSON column
values with user-provided `$type` keys.

This is an alternative to #4668, that might look cleaner on the engine
side.

Part of the fix for prisma/prisma#21454, will require client adjustments
as well.
SevInf pushed a commit that referenced this pull request Jan 26, 2024
Introduce another special value to JSON protocol, `"$type": "Raw"`.
When encountered, no other nested `$type` keys would be interpreted as
special and will be written to DB as is. Main usecase is JSON column
values with user-provided `$type` keys.

This is an alternative to #4668, that might look cleaner on the engine
side.

Part of the fix for prisma/prisma#21454, will require client adjustments
as well.
Sergey Tatarintsev added 5 commits January 26, 2024 17:24
In JSON protocol, we use `$type` key as a special marker for the types
that can not be represented in JSON natively. We did not want to make
`$type` identifier reserved, so we took some precautions: if during
encoding, at any point, `$type` key would be seen in user's input we
encode whole object as `{"$type": "Json", "value": [Serialized object] }`.

Engine handled those cases correctly, when `$type: Json` is encountered
at the top level, but not when it is nested. In that case,
`ArgumentValue`-to-`JSON` serialization just picked `value` key and
saved it as a string.

Fixed by moving Json value into it's own kind of `ArgumentValue`, that
are serialized to json as-is.

Fix prisma/prisma#21454
@SevInf SevInf force-pushed the fix/nested-json-dollar-type branch from dec953d to 5619fe9 Compare January 26, 2024 16:26
Copy link
Contributor

@jkomyno jkomyno left a comment

Choose a reason for hiding this comment

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

Nice! It's a bit of a pity that this fix increases the Wasm size, though

Copy link
Contributor

@Weakky Weakky left a comment

Choose a reason for hiding this comment

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

Sad but good work, lgtm 🙏

@SevInf
Copy link
Contributor Author

SevInf commented Jan 29, 2024

We have a failing test for JSON lists here.

Unfortunately, i don't think with this approach we can fix both problems at the same time, so i am going to close this in favor of previously rejected #4670

@SevInf SevInf closed this Jan 29, 2024
SevInf pushed a commit that referenced this pull request Jan 29, 2024
Introduce another special value to JSON protocol, `"$type": "Raw"`.
When encountered, no other nested `$type` keys would be interpreted as
special and will be written to DB as is. Main usecase is JSON column
values with user-provided `$type` keys.

This is an alternative to #4668, that might look cleaner on the engine
side.

Part of the fix for prisma/prisma#21454, will require client adjustments
as well.
SevInf pushed a commit that referenced this pull request Jan 29, 2024
* qe: Fix nested objects with `$type` key in JSON protocol

Introduce another special value to JSON protocol, `"$type": "Raw"`.
When encountered, no other nested `$type` keys would be interpreted as
special and will be written to DB as is. Main usecase is JSON column
values with user-provided `$type` keys.

This is an alternative to #4668, that might look cleaner on the engine
side.

Part of the fix for prisma/prisma#21454, will require client adjustments
as well.

* Fix & move the test
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.

Nested json is transformed to string when used “$type” key in object
3 participants