You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "twuebi (via GitHub)" <gi...@apache.org> on 2026/06/11 10:21:13 UTC
[PR] fix(table): do not claim sort_order_id on files the writer did not fully sort [iceberg-go]
twuebi opened a new pull request, #1184:
URL: https://github.com/apache/iceberg-go/pull/1184
The per-file sort_order_id declares that a file's rows are *fully* sorted by that order; a missing id means unsorted (https://iceberg.apache.org/spec/#sorting). Since #875 every writer stamped the table's default sort order id unconditionally, but #1157's sort-on-write only sorts each record batch individually, so the claim was false for any multi-batch file. Position delete files were stamped too, which the spec forbids outright, their order is (file_path, pos) and writers must set the field to null (https://iceberg.apache.org/spec/#data-file-fields). This contradicts the community consensus: Spark stamps the id *only* when its distribution+ordering globally sorted the whole task input, never inferring it from the table default (https://github.com/apache/iceberg/pull/15150).
This change makes writers claim nothing by default: data files, eq-delete files, pos-delete files, and registered files (add_files) leave sort_order_id absent, matching PyIceberg and iceberg-rust. The per-batch sort is kept as a pure layout optimization (tighter page statistics, better encodings). WriteTask.SortOrderID remains as an explicit caller claim for producers that guarantee fully sorted batches, mirroring how Java/Spark stamps the id only when the engine enforced the ordering. Leaving the field absent is fully spec-compliant, it was always advisory and never assumed applied to all files (https://github.com/apache/iceberg/issues/317). The only incorrect behavior is stamping a claim that isn't true, which a reader could trust to wrongly skip a re-sort.
References: https://github.com/apache/iceberg/issues/13634, https://github.com/apache/iceberg/pull/15150, https://github.com/apache/iceberg/issues/317, https://iceberg.apache.org/spec/#sorting
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): do not claim sort_order_id on files the writer did not fully sort [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1184:
URL: https://github.com/apache/iceberg-go/pull/1184#discussion_r3395349624
##########
table/internal/utils.go:
##########
@@ -330,7 +337,11 @@ func (d *DataFileStatistics) ToDataFile(opts DataFileOpts) iceberg.DataFile {
bldr.EqualityFieldIDs(d.EqualityFieldIDs)
}
- bldr.SortOrderID(opts.SortOrderID)
+ // Position deletes are ordered by (file_path, pos), never by a table sort
+ // order; the spec requires their sort order id to stay null.
+ if opts.SortOrderID != unsortedSortOrderID && opts.Content != iceberg.EntryContentPosDeletes {
Review Comment:
This one `&&` is folding two invariants that want different handling. The `!= unsortedSortOrderID` arm is "caller made no claim" — fine to skip silently. But `Content == EntryContentPosDeletes && SortOrderID != 0` is a spec violation, and right now it gets silently swallowed at the lowest layer rather than surfaced.
I'd split it into two sequential guards: first, if the content is pos-delete, skip stamping (and ideally assert `SortOrderID == 0`, since a non-zero claim here means a bug upstream); then, separately, if the order is non-unsorted, stamp it. That way the prohibited-claim case is detected instead of masked. wdyt?
##########
table/rolling_data_writer_test.go:
##########
@@ -313,8 +313,8 @@ func (s *RollingDataWriterTestSuite) TestWriterFactoryPropagatesSortOrderID() {
}
s.Require().Len(dataFiles, 1)
- s.Require().NotNil(dataFiles[0].SortOrderID(), "SortOrderID must be set on the emitted DataFile")
- s.Equal(expectedSortOrderID, *dataFiles[0].SortOrderID())
+ s.Nil(dataFiles[0].SortOrderID(),
Review Comment:
These inversions correctly assert the defaults don't stamp, but nothing exercises the explicit-claim path the other direction. Nothing constructs a `WriteTask` with a non-zero `SortOrderID`, runs it through `defaultDataFileWriter.writeFile`, and asserts `DataFile.SortOrderID()` equals the input.
That's the one path where the field is actually load-bearing, and it's currently untested — if the `writeFile` wiring broke, no test would catch it. I'd add that integration test before merge.
##########
table/writer.go:
##########
@@ -38,6 +38,9 @@ type WriteTask struct {
FileCount int // FileCount is a sequential counter for files written by this task.
Schema *iceberg.Schema
Batches []arrow.RecordBatch
+ // SortOrderID, when non-zero, is recorded on the resulting file as its
Review Comment:
This comment overpromises. It reads as unconditional, but it's not: the field is honored on the `defaultDataFileWriter.writeFile` path and completely ignored on the `writerFactory`/`RollingDataWriter` path (`openFileWriter` builds its own `WriteFileInfo` and never reads it), and it's also suppressed for pos-delete content by the `ToDataFile` guard.
A caller going through `WriteRecords` who sets this field gets a silent no-op. I'd qualify the doc to say it only takes effect on the batch/`writeFile` path and that pos-delete content drops it per spec, so the next person doesn't wire it up expecting it to work everywhere. Fix the doc and this is good to land.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): do not claim sort_order_id on files the writer did not fully sort [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on PR #1184:
URL: https://github.com/apache/iceberg-go/pull/1184#issuecomment-4680547035
@laskoviymishka ci failure seems unrelated
```
../gopath/pkg/mod/github.com/apache/arrow-go/v18@v18.6.1-0.20260608200437-88561cf0d192/arrow/internal/flatbuf/Binary.go:22:2: github.com/google/flatbuffers@v25.12.19+incompatible: read "https://proxy.golang.org/github.com/google/flatbuffers/@v/v25.12.19+incompatible.zip": stream error: stream ID 683; INTERNAL_ERROR; received from peer
FAIL github.com/apache/iceberg-go [setup failed]
# github.com/apache/iceberg-go/catalog
../gopath/pkg/mod/github.com/apache/arrow-go/v18@v18.6.1-0.20260608200437-88561cf0d192/arrow/internal/flatbuf/Binary.go:22:2: github.com/google/flatbuffers@v25.12.19+incompatible: read "https://proxy.golang.org/github.com/google/flatbuffers/@v/v25.12.19+incompatible.zip": stream error: stream ID 683; INTERNAL_ERROR; received from peer
# github.com/apache/iceberg-go/catalog/glue
../gopath/pkg/mod/github.com/apache/arrow-go/v18@v18.6.1-0.20260608200437-88561cf0d192/arrow/internal/flatbuf/Binary.go:22:2: github.com/google/flatbuffers@v25.12.19+incompatible: read "https://proxy.golang.org/github.com/google/flatbuffers/@v/v25.12.19+incompatible.zip": stream error: stream ID 683; INTERNAL_ERROR; received from peer
# github.com/apache/iceberg-go/catalog/hadoop
../gopath/pkg/mod/github.com/apache/arrow-go/v18@v18.6.1-0.20260608200437-88561cf0d192/arrow/internal/flatbuf/Binary.go:22:2: github.com/google/flatbuffers@v25.12.19+incompatible: read "https://proxy.golang.org/github.com/google/flatbuffers/@v/v25.12.19+incompatible.zip": stream error: stream ID 683; INTERNAL_ERROR; received from peer
# github.com/apache/iceberg-go/catalog/hive
../gopath/pkg/mod/github.com/apache/arrow-go/v18@v18.6.1-0.20260608200437-88561cf0d192/arrow/internal/flatbuf/Binary.go:22:2: github.com/google/flatbuffers@v25.12.19+incompatible: read "https://proxy.golang.org/github.com/google/flatbuffers/@v/v25.12.19+incompatible.zip": stream error: stream ID 683; INTERNAL_ERROR; received from peer
FAIL github.com/apache/iceberg-go/catalog [setup failed]
FAIL github.com/apache/iceberg-go/catalog/glue [setup failed]
FAIL github.com/apache/iceberg-go/catalog/hadoop [setup failed]
# github.com/apache/iceberg-go/catalog/internal
../gopath/pkg/mod/github.com/apache/arrow-go/v18@v18.6.1-0.20260608200437-88561cf0d192/arrow/internal/flatbuf/Binary.go:22:2: github.com/google/flatbuffers@v25.12.19+incompatible: read "https://proxy.golang.org/github.com/google/flatbuffers/@v/v25.12.19+incompatible.zip": stream error: stream ID 683; INTERNAL_ERROR; received from peer
# github.com/apache/iceberg-go/catalog/rest
../gopath/pkg/mod/github.com/apache/arrow-go/v18@v18.6.1-0.20260608200437-88561cf0d192/arrow/internal/flatbuf/Binary.go:22:2: github.com/google/flatbuffers@v25.12.19+incompatible: read "https://proxy.golang.org/github.com/google/flatbuffers/@v/v25.12.19+incompatible.zip": stream error: stream ID 683; INTERNAL_ERROR; received from peer
# github.com/apache/iceberg-go/catalog/sql
../gopath/pkg/mod/github.com/apache/arrow-go/v18@v18.6.1-0.20260608200437-88561cf0d192/arrow/internal/flatbuf/Binary.go:22:2: github.com/google/flatbuffers@v25.12.19+incompatible: read "https://proxy.golang.org/github.com/google/flatbuffers/@v/v25.12.19+incompatible.zip": stream error: stream ID 683; INTERNAL_ERROR; received from peer
# github.com/apache/iceberg-go/cmd/iceberg
../gopath/pkg/mod/github.com/apache/arrow-go/v18@v18.6.1-0.20260608200437-88561cf0d192/arrow/internal/flatbuf/Binary.go:22:2: github.com/google/flatbuffers@v25.12.19+incompatible: read "https://proxy.golang.org/github.com/google/flatbuffers/@v/v25.12.19+incompatible.zip": stream error: stream ID 683; INTERNAL_ERROR; received from peer
# github.com/apache/iceberg-go/codec
../gopath/pkg/mod/github.com/apache/arrow-go/v18@v18.6.1-0.20260608200437-88561cf0d192/arrow/internal/flatbuf/Binary.go:22:2: github.com/google/flatbuffers@v25.12.19+incompatible: read "https://proxy.golang.org/github.com/google/flatbuffers/@v/v25.12.19+incompatible.zip": stream error: stream ID 683; INTERNAL_ERROR; received from peer
FAIL github.com/apache/iceberg-go/catalog/hive [setup failed]
FAIL github.com/apache/iceberg-go/catalog/internal [setup failed]
FAIL github.com/apache/iceberg-go/catalog/rest [setup failed]
FAIL github.com/apache/iceberg-go/catalog/sql [setup failed]
FAIL github.com/apache/iceberg-go/cmd/iceberg [setup failed]
FAIL github.com/apache/iceberg-go/codec [setup failed]
# github.com/apache/iceberg-go/puffin
FAIL github.com/apache/iceberg-go/puffin [setup failed]
FAIL github.com/apache/iceberg-go/table [setup failed]
../gopath/pkg/mod/github.com/apache/arrow-go/v18@v18.6.1-0.20260608200437-88561cf0d192/arrow/internal/flatbuf/Binary.go:22:2: github.com/google/flatbuffers@v25.12.19+incompatible: read "https://proxy.golang.org/github.com/google/flatbuffers/@v/v25.12.19+incompatible.zip": stream error: stream ID 683; INTERNAL_ERROR; received from peer
# github.com/apache/iceberg-go/table
../gopath/pkg/mod/github.com/apache/arrow-go/v18@v18.6.1-0.20260608200437-88561cf0d192/arrow/internal/flatbuf/Binary.go:22:2: github.com/google/flatbuffers@v25.12.19+incompatible: read "https://proxy.golang.org/github.com/google/flatbuffers/@v/v25.12.19+incompatible.zip": stream error: stream ID 683; INTERNAL_ERROR; received from peer
# github.com/apache/iceberg-go/table/compaction
../gopath/pkg/mod/github.com/apache/arrow-go/v18@v18.6.1-0.20260608200437-88561cf0d192/arrow/internal/flatbuf/Binary.go:22:2: github.com/google/flatbuffers@v25.12.19+incompatible: read "https://proxy.golang.org/github.com/google/flatbuffers/@v/v25.12.19+incompatible.zip": stream error: stream ID 683; INTERNAL_ERROR; received from peer
# github.com/apache/iceberg-go/table/dv
../gopath/pkg/mod/github.com/apache/arrow-go/v18@v18.6.1-0.20260608200437-88561cf0d192/arrow/internal/flatbuf/Binary.go:22:2: github.com/google/flatbuffers@v25.12.19+incompatible: read "https://proxy.golang.org/github.com/google/flatbuffers/@v/v25.12.19+incompatible.zip": stream error: stream ID 683; INTERNAL_ERROR; received from peer
# github.com/apache/iceberg-go/table/internal
../gopath/pkg/mod/github.com/apache/arrow-go/v18@v18.6.1-0.20260608200437-88561cf0d192/arrow/internal/flatbuf/Binary.go:22:2: github.com/google/flatbuffers@v25.12.19+incompatible: read "https://proxy.golang.org/github.com/google/flatbuffers/@v/v25.12.19+incompatible.zip": stream error: stream ID 683; INTERNAL_ERROR; received from peer
# github.com/apache/iceberg-go/table/substrait
../gopath/pkg/mod/github.com/apache/arrow-go/v18@v18.6.1-0.20260608200437-88561cf0d192/arrow/internal/flatbuf/Binary.go:22:2: github.com/google/flatbuffers@v25.12.19+incompatible: read "https://proxy.golang.org/github.com/google/flatbuffers/@v/v25.12.19+incompatible.zip": stream error: stream ID 683; INTERNAL_ERROR; received from peer
FAIL github.com/apache/iceberg-go/table/compaction [setup failed]
FAIL github.com/apache/iceberg-go/table/dv [setup failed]
FAIL github.com/apache/iceberg-go/table/internal [setup failed]
FAIL github.com/apache/iceberg-go/table/substrait [setup failed]
# github.com/apache/iceberg-go/view
../gopath/pkg/mod/github.com/apache/arrow-go/v18@v18.6.1-0.20260608200437-88561cf0d192/arrow/internal/flatbuf/Binary.go:22:2: github.com/google/flatbuffers@v25.12.19+incompatible: read "https://proxy.golang.org/github.com/google/flatbuffers/@v/v25.12.19+incompatible.zip": stream error: stream ID 683; INTERNAL_ERROR; received from peer
FAIL github.com/apache/iceberg-go/view [setup failed]
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): do not claim sort_order_id on files the writer did not fully sort [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1184:
URL: https://github.com/apache/iceberg-go/pull/1184
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): do not claim sort_order_id on files the writer did not fully sort [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #1184:
URL: https://github.com/apache/iceberg-go/pull/1184#discussion_r3403429534
##########
table/internal/utils.go:
##########
@@ -330,7 +337,11 @@ func (d *DataFileStatistics) ToDataFile(opts DataFileOpts) iceberg.DataFile {
bldr.EqualityFieldIDs(d.EqualityFieldIDs)
}
- bldr.SortOrderID(opts.SortOrderID)
+ // Position deletes are ordered by (file_path, pos), never by a table sort
+ // order; the spec requires their sort order id to stay null.
+ if opts.SortOrderID != unsortedSortOrderID && opts.Content != iceberg.EntryContentPosDeletes {
Review Comment:
thanks, fixed
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): do not claim sort_order_id on files the writer did not fully sort [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #1184:
URL: https://github.com/apache/iceberg-go/pull/1184#discussion_r3403426839
##########
table/rolling_data_writer_test.go:
##########
@@ -313,8 +313,8 @@ func (s *RollingDataWriterTestSuite) TestWriterFactoryPropagatesSortOrderID() {
}
s.Require().Len(dataFiles, 1)
- s.Require().NotNil(dataFiles[0].SortOrderID(), "SortOrderID must be set on the emitted DataFile")
- s.Equal(expectedSortOrderID, *dataFiles[0].SortOrderID())
+ s.Nil(dataFiles[0].SortOrderID(),
Review Comment:
added the test
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): do not claim sort_order_id on files the writer did not fully sort [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on PR #1184:
URL: https://github.com/apache/iceberg-go/pull/1184#issuecomment-4679665365
@laskoviymishka
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): do not claim sort_order_id on files the writer did not fully sort [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #1184:
URL: https://github.com/apache/iceberg-go/pull/1184#discussion_r3403428181
##########
table/writer.go:
##########
@@ -38,6 +38,9 @@ type WriteTask struct {
FileCount int // FileCount is a sequential counter for files written by this task.
Schema *iceberg.Schema
Batches []arrow.RecordBatch
+ // SortOrderID, when non-zero, is recorded on the resulting file as its
Review Comment:
updated
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): do not claim sort_order_id on files the writer did not fully sort [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1184:
URL: https://github.com/apache/iceberg-go/pull/1184#discussion_r3412827182
##########
table/writer.go:
##########
@@ -38,6 +38,13 @@ type WriteTask struct {
FileCount int // FileCount is a sequential counter for files written by this task.
Schema *iceberg.Schema
Batches []arrow.RecordBatch
+ // SortOrderID, when non-zero, is recorded on the resulting file as its
+ // sort order. Set it only when Batches are fully sorted by that order —
+ // the writer does not verify or enforce the claim. Only the bin-packed
+ // task path (defaultDataFileWriter.writeFile) honors it; the rolling
+ // data writer builds its own file info and ignores it, and position
+ // delete content must leave it zero (the spec requires a null sort
+ // order id there).
SortOrderID int
Review Comment:
This is an exported field whose behavior depends on which internal writer happens to process the task — honored by the bin-packed path, silently dropped by the rolling path. The doc is honest about it, but "set this field and it may or may not do anything depending on internal routing you can't see" is a pretty fragile contract for a public type.
I'd either move `SortOrderID` onto a narrower task type that only the bin-packed path constructs, or add a guard in `openFileWriter`/`newRollingDataWriter` that errors (don't panic — see the goroutine concern) on a non-zero claim it's about to ignore, mirroring the pos-delete invariant. Right now a downstream caller that pre-sorts and sets this through the rolling path believes the manifest is stamped when it isn't. Thoughts on splitting the type?
##########
table/internal/utils.go:
##########
@@ -330,7 +337,18 @@ func (d *DataFileStatistics) ToDataFile(opts DataFileOpts) iceberg.DataFile {
bldr.EqualityFieldIDs(d.EqualityFieldIDs)
}
- bldr.SortOrderID(opts.SortOrderID)
+ // Position deletes are ordered by (file_path, pos), never by a table sort
+ // order; the spec requires their sort order id to stay null. This is an
+ // internal invariant, not user-input validation: no exported API accepts
+ // a WriteTask or reaches this package, and the pos-delete producers never
+ // set a claim — a non-zero value here is a library bug.
+ if opts.Content == iceberg.EntryContentPosDeletes && opts.SortOrderID != unsortedSortOrderID {
Review Comment:
I think the invariant is right but the `panic` is in a risky spot. `ToDataFile` is reached through `ParquetFileWriter.Close()`, which runs in the rolling writer's `stream` goroutine — and that goroutine has no `recover()`, unlike `recordsToDataFiles` and `equalityDeleteRecordsToDataFiles`, which both convert panics to errors on their error channel. So if this ever fires from that path it takes down the process instead of surfacing an error.
It's unreachable from the public API today, agreed. But I'd rather not leave a process-killing panic on a code path a future refactor could wander into. Two options: validate the content/claim combination earlier on the calling goroutine (in `writeFile` before `WriteDataFile`) where the content type is already known, or have `ToDataFile` return `(DataFile, error)` and let callers propagate it.
If we keep the panic, I'd at least add a `recover()` in `stream`'s top-level defer that routes to `r.errorCh`, mirroring the sibling writers, and a comment stating the panic is an unreachable last-resort guard. wdyt?
##########
table/write_records_sort_test.go:
##########
@@ -138,8 +138,9 @@ func (s *WriteRecordsSortTestSuite) TestPartitionedTableSortedPerFile() {
}
for _, df := range dataFiles {
- s.NotNil(df.SortOrderID(), "data file must carry the sort order id")
- s.Equal(meta.DefaultSortOrder(), *df.SortOrderID())
+ // The writer only sorts per batch, so it never claims the table sort
+ // order on the file (the spec's sort_order_id is a per-file claim).
+ s.Nil(df.SortOrderID(), "data file must not claim a sort order id")
Review Comment:
While we're tightening sort assertions: `TestUnpartitionedTableMultipleBatchesEachSorted` (further down in this file) doesn't actually distinguish per-batch from global sort. Its expected sequence `[1,2,3,4,5,6]` passes whether the writer sorts each batch independently or sorts globally, because the batches happen to be monotonic — so the only real regression guard there is the nil `sort_order_id` check, not the ordering.
I'd feed it non-monotonic, overlapping batches (e.g. `[5,3,1]` then `[4,2,0]`) and assert the locally-sorted-but-not-global result `[1,3,5,0,2,4]`. That fails the moment someone introduces a global sort, which is exactly the behavior this whole PR is drawing a line under.
##########
table/rolling_data_writer_test.go:
##########
@@ -313,8 +313,8 @@ func (s *RollingDataWriterTestSuite) TestWriterFactoryPropagatesSortOrderID() {
}
s.Require().Len(dataFiles, 1)
- s.Require().NotNil(dataFiles[0].SortOrderID(), "SortOrderID must be set on the emitted DataFile")
- s.Equal(expectedSortOrderID, *dataFiles[0].SortOrderID())
+ s.Nil(dataFiles[0].SortOrderID(),
Review Comment:
This nicely pins that the rolling writer doesn't stamp the file. What's still missing is a test for the other half of the `WriteTask.SortOrderID` contract: a caller setting a non-zero claim and routing it through the path that ignores it.
I'd add a case here that constructs a `WriteTask` with a non-zero `SortOrderID`, runs it through the rolling/factory path on a sorted batch, and asserts `DataFile.SortOrderID()` is still nil. That's the only thing that pins "rolling writer ignores the claim" — otherwise a future change that starts honoring it goes unnoticed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): do not claim sort_order_id on files the writer did not fully sort [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on PR #1184:
URL: https://github.com/apache/iceberg-go/pull/1184#issuecomment-4705424729
@laskoviymishka
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org