vbhanuchander-lang commented on issue #15465:
URL: https://github.com/apache/iceberg/issues/15465#issuecomment-5251185860
There is a mechanical detail in the planner that I think answers
@RussellSpitzer's question about whether this should be automatic, and also
supports @lirui-apache's point that `output-spec-id` is not sufficient.
Files whose spec is not the current spec are not grouped by their own
partition. They are all collapsed into a single bucket:
```java
// BinPackRewriteFilePlanner.groupByPartition
StructLike emptyStruct = GenericRecord.create(partitionType);
...
// If a task uses an incompatible partition spec the data inside could
contain values
// which belong to multiple partitions in the current spec. Treating all
such files as
// un-partitioned and grouping them together helps to minimize new files
made.
StructLike taskPartition =
task.file().specId() == table.spec().specId() ? task.file().partition()
: emptyStruct;
filesByPartition.computeIfAbsent(taskPartition, unused ->
Lists.newArrayList()).add(task);
```
and the output spec defaults to the current one:
```java
// SizeBasedFileRewritePlanner
PropertyUtil.propertyAsInt(options, RewriteDataFiles.OUTPUT_SPEC_ID,
table.spec().specId());
```
Two consequences worth having in the thread:
**1. This is the mechanism behind the report, and it is not just a default
gone wrong.** In @lirui-apache's example the `dt`-only files land in the
`emptyStruct` bucket and are then written with the current `(dt, id)` spec. One
input group fans out to one output file per distinct `(dt, id)`. The comment
above the grouping is accurate about its own intent — minimising new files
*within the current spec* — but when the spec has become finer, minimising
groups maximises output files.
**2. `output-spec-id` cannot express "keep the input spec", for a stronger
reason than the extra lookup cost.** The `emptyStruct` bucket holds files from
*every* non-current spec at once, so with three specs a single group can
contain files from two different old specs. `output-spec-id` is one value per
rewrite, and `RewriteFileGroup.outputSpecId()` is one value per group, so there
is no value that means "each file keeps its own spec" — the grouping has
already destroyed that information.
So this is not purely a question of defaults or of user intent. Expressing
"compact within the input spec" needs the planner to group by `(specId,
partition)` rather than partition-with-an-`emptyStruct`-fallback, after which a
`rewrite-within-input-spec` style option becomes representable and the current
behaviour stays reachable as the default.
That reframes the choice: rather than "should we do this for users", it is
"should the planner keep spec identity so that either behaviour can be asked
for". Keeping the current grouping as the default preserves today's semantics
for everyone who wants convergence onto the new spec.
I have not written any of this — flagging it because the thread had stalled
on a design question that the grouping code partly answers. Happy to prototype
the `(specId, partition)` grouping if that direction seems right to you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
