-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Improve VariableWidthBlock encoding #27377
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
base: master
Are you sure you want to change the base?
Improve VariableWidthBlock encoding #27377
Conversation
a842e17 to
f1d3e60
Compare
core/trino-spi/src/main/java/io/trino/spi/block/EncoderUtil.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/main/java/io/trino/spi/block/VariableWidthBlockEncoding.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/main/java/io/trino/spi/block/EncoderUtil.java
Outdated
Show resolved
Hide resolved
10a9db5 to
e08afb0
Compare
| @Language("SQL") String createTableSql = | ||
| """ | ||
| CREATE TABLE test_table_writer_skew_mitigation WITH (%s = ARRAY['returnflag']) AS | ||
| SELECT orderkey, partkey, suppkey, linenumber, quantity, extendedprice, discount, tax, linestatus, shipdate, commitdate, receiptdate, shipinstruct, shipmode, comment, returnflag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want to delete column comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is highly dependent on the serialized page size, so adding 4 extra bytes per serialized page breaks the test. I'm duplicating the workaround from #15760 but this is indeed a hack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, hacky part is from the test setup itself though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually going to avoid changing the serialized size- we don't need to add the extra 4 bytes anyway since we can just send the non-null ending offsets and not send the starting offset of 0 in the serialized representation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...and it didn't work. Reintroducing the hack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think this is not a good fix to the spurious test, since we will probably always change block serde, another code change may fail on the case where comment is deleted, but pass on the case where comment is added back. I think we should change the assertion here instead https://github.com/trinodb/trino/blob/master/testing/trino-faulttolerant-tests/src/test/java/io/trino/faulttolerant/BaseFaultTolerantExecutionTest.java#L62C20-L62C39, the two numbers just happen to be equal and is not guaranteed to be equal on all cases.
b076058 to
85afc34
Compare
|
Started benchmark workflow for this PR with test type =
|
|
Started benchmark workflow for this PR with test type =
|
Avoids converting between offsets and lengths when serializing and deserializing VariableWidthBlock instances, which enables a fast-path conversion for blocks without nulls present. When nulls are present, the compaction and expansion of offsets still outperforms the length to offset conversion.
85afc34 to
dcded0b
Compare
Description
Avoids converting between offsets and lengths when serializing and deserializing VariableWidthBlock instances, which enables a fast-path conversion for blocks without nulls present. When nulls are present, the compaction and expansion of offsets still outperforms the length to offset conversion.
Benchmarks
jmh.morethan.io - Significant improvements
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: