laskoviymishka commented on code in PR #1627:
URL: https://github.com/apache/iceberg-go/pull/1627#discussion_r3719812551
##########
manifest.go:
##########
@@ -1671,8 +1694,12 @@ func (m *ManifestListWriter) AddManifests(files
[]ManifestFile) error {
if wrapped.FirstRowIDValue == nil {
if m.nextRowID != nil {
firstRowID :=
*m.nextRowID
+ nextRowID, err :=
advanceRowID(firstRowID, wrapped.ExistingRowsCount, wrapped.AddedRowsCount)
Review Comment:
The overflow rejection is right, but I think this guard hard-aborts a
spec-legal path. `toFile()` sets `AddedRowsCount` and `ExistingRowsCount` to -1
when the Avro count was null, which is how pre-1.4 v1/v2 manifests decode.
Carrying one of those into a v3 list — the upgrade-without-rewrite path the
comment right above even calls out — now trips `existingRows < 0 || addedRows <
0` and returns `ErrInvalidArgument`, so the whole write aborts on a count the
caller never set.
The old code was also wrong here (it advanced the cursor by -2), so this
surfaces the bug rather than introducing it, but a hard error on old tables is
a worse failure mode than the silent one. I'd normalize the -1 sentinel to 0
for the cursor math and keep the hard error for genuinely-negative counts.
Can you confirm the `toFile` -1 path actually reaches here? That's the one
link I couldn't fully trace.
##########
manifest_test.go:
##########
@@ -2430,6 +2446,47 @@ func (m *ManifestTestSuite)
TestV3ManifestListWriterRowIDTracking() {
m.Require().NoError(err)
}
+func (m *ManifestTestSuite)
TestV3ManifestListWriterRejectsInvalidRowIDRanges() {
+ m.Run("negative first row ID", func() {
+ var buf bytes.Buffer
+ writer, err := NewManifestListWriterV3(&buf, snapshotID, 1, -1,
nil)
+ m.Nil(writer)
+ m.Require().ErrorIs(err, ErrInvalidArgument)
+ m.Require().ErrorContains(err, "first row ID must be
non-negative")
+ })
+
+ m.Run("negative row count", func() {
+ var buf bytes.Buffer
+ writer, err := NewManifestListWriterV3(&buf, snapshotID, 1, 0,
nil)
+ m.Require().NoError(err)
+ manifest := NewManifestFile(3, "negative.avro", 100, 1,
snapshotID).AddedRows(-2).Build()
+ err = writer.AddManifests([]ManifestFile{manifest})
+ m.Require().ErrorIs(err, ErrInvalidArgument)
+ m.Require().ErrorContains(err, "row counts must be
non-negative")
+ })
+
+ m.Run("overflow", func() {
Review Comment:
This subtest and the negative-count one above only drive `AddedRows`, so the
existing-rows branch of the guard (`existingRows > MaxInt64-firstRowID`) never
actually runs. A swap or drop of that clause would sail through green.
I'd add `ExistingRows` parallels for both — one with `ExistingRows(-1)`, one
with `ExistingRows(1)` at `firstRowID = math.MaxInt64` — so both overflow
clauses are pinned.
##########
manifest.go:
##########
@@ -1594,6 +1598,23 @@ func NewManifestListWriterV3(out io.Writer, snapshotId,
sequenceNumber, firstRow
})
}
+func advanceRowID(firstRowID, existingRows, addedRows int64) (int64, error) {
Review Comment:
Both callers guard `firstRowID < 0` before this runs, but `advanceRowID`
itself assumes it and the arithmetic quietly breaks if that assumption ever
slips — `math.MaxInt64 - firstRowID` wraps for a negative `firstRowID`, so the
overflow check would misfire. I'd move the `firstRowID < 0` check in here as
the first clause.
That also lets us drop the duplicated `first row ID must be non-negative`
string from both call sites — right now two copies feed two `ErrorContains`
assertions, and if one drifts a test passes against the wrong text. One guard,
one message, self-contained. wdyt?
##########
manifest_test.go:
##########
@@ -2430,6 +2446,47 @@ func (m *ManifestTestSuite)
TestV3ManifestListWriterRowIDTracking() {
m.Require().NoError(err)
}
+func (m *ManifestTestSuite)
TestV3ManifestListWriterRejectsInvalidRowIDRanges() {
+ m.Run("negative first row ID", func() {
+ var buf bytes.Buffer
+ writer, err := NewManifestListWriterV3(&buf, snapshotID, 1, -1,
nil)
+ m.Nil(writer)
+ m.Require().ErrorIs(err, ErrInvalidArgument)
+ m.Require().ErrorContains(err, "first row ID must be
non-negative")
+ })
+
+ m.Run("negative row count", func() {
+ var buf bytes.Buffer
+ writer, err := NewManifestListWriterV3(&buf, snapshotID, 1, 0,
nil)
+ m.Require().NoError(err)
+ manifest := NewManifestFile(3, "negative.avro", 100, 1,
snapshotID).AddedRows(-2).Build()
+ err = writer.AddManifests([]ManifestFile{manifest})
+ m.Require().ErrorIs(err, ErrInvalidArgument)
+ m.Require().ErrorContains(err, "row counts must be
non-negative")
+ })
+
+ m.Run("overflow", func() {
+ var buf bytes.Buffer
+ writer, err := NewManifestListWriterV3(&buf, snapshotID, 1,
math.MaxInt64, nil)
+ m.Require().NoError(err)
+ manifest := NewManifestFile(3, "overflow.avro", 100, 1,
snapshotID).AddedRows(1).Build()
+ err = writer.AddManifests([]ManifestFile{manifest})
+ m.Require().ErrorIs(err, ErrInvalidArgument)
+ m.Require().ErrorContains(err, "overflows int64")
+ m.EqualValues(math.MaxInt64, *writer.NextRowID())
+ })
+
+ m.Run("later validation failure leaves cursor unchanged", func() {
Review Comment:
This nicely covers the cursor staying put when the first manifest fails, but
the deferral you built is really there for the batch case: early manifests
succeed and advance, a later one overflows, and the cursor has to stay at where
the batch started. That's the path a shared-state or advance-outside-the-loop
bug would slip through, and it isn't exercised yet.
I'd add a `[valid, overflow]` case — writer at 10, first manifest adds 5 and
succeeds, second overflows — then assert the error comes back and
`*writer.NextRowID()` is still 10. That's the assertion that actually proves
the deferral.
##########
manifest.go:
##########
@@ -1810,7 +1844,10 @@ func WriteManifestV3(
raw := mf.(*manifestFile)
v := firstRowID
raw.FirstRowIDValue = &v
- nextFirstRowID = firstRowID + raw.AddedRowsCount + raw.ExistingRowsCount
+ nextFirstRowID, err = advanceRowID(firstRowID, raw.ExistingRowsCount,
raw.AddedRowsCount)
Review Comment:
`AddManifests` wraps this as `manifest %q: %w` so the caller knows which
file blew up, but here the bare `err` comes back with no filename. Since
`WriteManifestV3` already takes the path, I'd wrap it the same way for parity.
--
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]
