Populate split offsets when writing orc/parquet files in iceberg #22250
Conversation
raunaqmorarka
requested review from
alexjo2144,
electrum,
findepi,
findinpath and
sopel39
raunaqmorarka
force-pushed
the
ice-offsets
branch
2 times, most recently
from
05ba753 to
d715350
Compare
| .mapToLong(ColumnStatistics::getRetainedSizeInBytes) | ||
| .sum()).orElse(0L); | ||
| stripeOffsets = closedStripes.stream() | ||
| .map(closedStripe -> closedStripe.getStripeInformation().getOffset()) |
There was a problem hiding this comment.
could you pls add to the description of the PR that it relates to:
?
Now what does this mean on plain english for Trino to use OffsetsAwareSplitScanTaskIterator ?
How does this improve the efficiency of Trino read workloads?
There was a problem hiding this comment.
It means that we get 1 split per parquet row-group or orc stripe instead of 128MB (or configured property) size split which might be mapped to 0-N row-groups depending on row group size configured in the writer. It should give us same or more parallelism and avoid any empty splits due to mis-alignment of logical split offsets with file row-group boundaries.
We were already getting the same behaviour when the data was written by some other engine/library which would populate split offsets.
There was a problem hiding this comment.
It means that we get 1 split per parquet row-group or orc stripe instead of 128MB (or configured property) size split which might be mapped to 0-N row-groups depending on row group size configured in the writer.
Do we have minimal split size if the row groups are super tiny, e.g: to avoid reading Parquet footers over and over again for tiny tiny stripes?
There was a problem hiding this comment.
We rely on iceberg library logic to split the file and it doesn't have such a fallback. There's no good reason why any writer would be producing a lot of small row-groups, it would end up nullifying all the advantage of a columnar file format. Keep in mind that other writers are already populating this field and we've already been relying on the same iceberg logic for data written in this way. If someone has such data due to some misconfiguration, they need to fix it through running OPTIMIZE or CTAS.
There was a problem hiding this comment.
There's no good reason why any writer would be producing a lot of small row-groups
Right, but small row-groups could be a byproduct of wide rows (I think I've seen that happening)
There was a problem hiding this comment.
They can be small in row count, but can't be small in size due to columns size and existing split size was not based on row count.
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergPageSink.java
Show resolved
Hide resolved
| .mapToLong(ColumnStatistics::getRetainedSizeInBytes) | ||
| .sum()).orElse(0L); | ||
| stripeOffsets = closedStripes.stream() | ||
| .map(closedStripe -> closedStripe.getStripeInformation().getOffset()) |
There was a problem hiding this comment.
It means that we get 1 split per parquet row-group or orc stripe instead of 128MB (or configured property) size split which might be mapped to 0-N row-groups depending on row group size configured in the writer.
Do we have minimal split size if the row groups are super tiny, e.g: to avoid reading Parquet footers over and over again for tiny tiny stripes?
| .withFileSizeInBytes(task.fileSizeInBytes()) | ||
| .withFormat(table.getFileFormat().toIceberg()) | ||
| .withMetrics(task.metrics().metrics()); | ||
| task.fileSplitOffsets().ifPresent(builder::withSplitOffsets); |
There was a problem hiding this comment.
how does this builder relates to IcebergPageSink?
There was a problem hiding this comment.
Builder is an existing iceberg API, we're just using it to populate the offsets that we received from workers through CommitTaskData
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/util/ParquetUtil.java
Outdated
Show resolved
Hide resolved
raunaqmorarka
force-pushed
the
ice-offsets
branch
2 times, most recently
from
51ba5ea to
4102731
Compare
Avoids creating too many splits when split offsets are populated
Offsets can be used by readers to align splits with row-group/stripe boundaries
raunaqmorarka
force-pushed
the
ice-offsets
branch
from
4102731 to
48e023a
Compare
Description
Offsets can be used by readers to align splits with row-group/stripe boundaries.
For data written this change, https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/OffsetsAwareSplitScanTaskIterator.java#L30 will be used to generate 1 split per parquet row-group or orc stripe.
Additional context and related issues
Fixes #9018
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.
(x) Release notes are required, with the following suggested text: