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.

Sign up for GitHub

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

Jump to bottom

Implement faster JSON decoder compatible with original one #23352

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Implement faster JSON decoder compatible with original one #23352

wants to merge 10 commits into from
+1,279 −37

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Sep 10, 2024

No description provided.

@cla-bot cla-bot bot added the cla-signed label Sep 10, 2024
@wendigo wendigo force-pushed the serafin/faster-json branch 3 times, most recently from 230e206 to 90aaf26 Compare September 11, 2024 07:13
electrum
electrum reviewed Sep 11, 2024
View reviewed changes
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

Can you add a test that has some literal JSON, so that it's easy to see what the JSON looks like?

client/trino-client/src/main/java/io/trino/client/spooling/encoding/JsonDecodingUtils.java Show resolved Hide resolved
@wendigo wendigo force-pushed the serafin/faster-json branch 2 times, most recently from a2755c9 to 1b5c6fb Compare October 1, 2024 15:36
@wendigo wendigo changed the title Improve spooled JSON format decoding/encoding Implement faster JSON decoder compatible with original one Oct 1, 2024
@wendigo
Duplicate FixJsonDataUtils
83a1a10 
This is a preparation step to change the way spooled JSON is
produced and than decoded.
electrum
electrum reviewed Oct 2, 2024
View reviewed changes
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

Looks good overall

client/trino-client/src/main/java/io/trino/client/QueryDataDecoder.java Outdated Show resolved Hide resolved
client/trino-client/src/main/java/io/trino/client/spooling/encoding/JsonDecodingUtils.java Outdated Show resolved Hide resolved
client/trino-client/src/main/java/io/trino/client/spooling/encoding/JsonDecodingUtils.java Outdated Show resolved Hide resolved
client/trino-client/src/main/java/io/trino/client/spooling/encoding/JsonQueryDataAccess.java Outdated Show resolved Hide resolved
client/trino-client/src/main/java/io/trino/client/spooling/encoding/JsonQueryDataAccess.java Outdated Show resolved Hide resolved
client/trino-client/src/main/java/io/trino/client/spooling/encoding/JsonDecodingUtils.java Outdated Show resolved Hide resolved
client/trino-client/src/main/java/io/trino/client/spooling/encoding/JsonDecodingUtils.java Outdated Show resolved Hide resolved
@github-actions github-actions bot added the jdbc Relates to Trino JDBC driver label Oct 2, 2024
@wendigo
Copy link
Contributor Author

wendigo commented Oct 2, 2024 •
edited
Loading

Benchmarks: fetching and decoding 4M rows over spooled protocol locally.

Before:

Average time for 20 runs: 12648ms

After:

Average time for 20 runs: 9361ms

In comparison non-spooled protocol runs in ~30s

losipiuk
losipiuk reviewed Oct 2, 2024
View reviewed changes
Copy link
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

LGTM % comment about iterable

@wendigo
Implement JsonParser based JSON decoder
9b7482a 
Current JSON decoding logic requires two passes to decode JSON values:
- reading List<Object> using ObjectMapper
- applying type information on top of the primitive JSON values

Instead, the new decoder does the reading of the primitive JSON values
and type application in a single pass that is both faster and reduces
the amount of memory allocation which results in 30% improvement in the
spooled protocol.
@wendigo
Run PostgreSQL tests with spooled protocol
a0b06ed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@electrum electrum electrum left review comments

@pettyjamesm pettyjamesm pettyjamesm left review comments

@losipiuk losipiuk losipiuk approved these changes

Assignees
No one assigned
Labels
cla-signed jdbc Relates to Trino JDBC driver
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wendigo @electrum @pettyjamesm @losipiuk