laskoviymishka commented on code in PR #1735:
URL: https://github.com/apache/iceberg-go/pull/1735#discussion_r3850550730
##########
manifest_test.go:
##########
@@ -3416,23 +3416,41 @@ func (m *ManifestTestSuite)
TestV3ManifestListAcceptsV1AndV2Manifests() {
m.Nil(v2Entry.FirstRowID(), "delete manifests must not be assigned
first_row_id")
}
-func (m *ManifestTestSuite)
TestV3ManifestListAssignsZeroForV1ManifestWithUnknownRowCounts() {
- legacy := *(manifestFileRecordsV1[0].(*manifestFile))
- legacy.AddedRowsCount = -1
- legacy.ExistingRowsCount = -1
+func (m *ManifestTestSuite)
TestV3ManifestListRejectsV1ManifestWithUnknownRowCounts() {
+ tests := []struct {
+ name string
+ addedRows int64
+ existingRows int64
+ }{
+ {name: "both row counts unknown", addedRows: -1, existingRows:
-1},
Review Comment:
Small naming thing: "existing row count unknown" and "added row count
unknown" read as if only that field is set, but the point of these two cases is
that the sibling is a known positive count and the guard still fires. "only
existing row count unknown" / "only added row count unknown" captures that
intent. While we're here, the loop var is `test` where the rest of this file
uses `tt`.
##########
manifest.go:
##########
@@ -1631,11 +1631,9 @@ func advanceRowID(firstRowID, existingRows, addedRows
int64) (int64, error) {
if firstRowID < 0 {
return 0, fmt.Errorf("%w: first row ID must be non-negative:
%d", ErrInvalidArgument, firstRowID)
}
- if existingRows == -1 {
- existingRows = 0
- }
- if addedRows == -1 {
- addedRows = 0
+ if existingRows == -1 || addedRows == -1 {
Review Comment:
Rejecting here is the right call and matches Java's behavior (it NPEs on the
same input). The trade-off worth naming: the spec says a null count "is assumed
to be non-zero", so a V1 table whose manifests legally omit these counts now
can't produce a V3 snapshot after upgrade, with no recovery short of rewriting
those manifests. The spec does describe a fallback (open the manifest and sum
`record_count` for files whose `first_row_id` is null), but that's expensive
and probably out of scope here.
I'd keep the rejection and just make the dead-end recoverable in the
operator's eyes, either in the error text or a doc comment: the way out is to
rewrite or compact those manifests before upgrading. wdyt, reject-and-document,
or is the fallback worth implementing?
##########
manifest.go:
##########
@@ -1631,11 +1631,9 @@ func advanceRowID(firstRowID, existingRows, addedRows
int64) (int64, error) {
if firstRowID < 0 {
return 0, fmt.Errorf("%w: first row ID must be non-negative:
%d", ErrInvalidArgument, firstRowID)
}
- if existingRows == -1 {
- existingRows = 0
- }
- if addedRows == -1 {
- addedRows = 0
+ if existingRows == -1 || addedRows == -1 {
+ return 0, fmt.Errorf("%w: cannot assign row-lineage IDs with
unknown row counts: existing=%d added=%d",
Review Comment:
In the partial cases (one count is -1, the other a real value) the message
reads "unknown row counts" plural while printing a legitimate count right next
to it, so an operator looking at `existing=-1 added=1` has to work out which
one is the sentinel. I'd either say "at least one row count unknown" or split
into two guards, one message per field. The split also lets the test's
`ErrorContains` assert on which field specifically it's rejecting.
##########
manifest_test.go:
##########
@@ -3416,23 +3416,41 @@ func (m *ManifestTestSuite)
TestV3ManifestListAcceptsV1AndV2Manifests() {
m.Nil(v2Entry.FirstRowID(), "delete manifests must not be assigned
first_row_id")
}
-func (m *ManifestTestSuite)
TestV3ManifestListAssignsZeroForV1ManifestWithUnknownRowCounts() {
- legacy := *(manifestFileRecordsV1[0].(*manifestFile))
- legacy.AddedRowsCount = -1
- legacy.ExistingRowsCount = -1
+func (m *ManifestTestSuite)
TestV3ManifestListRejectsV1ManifestWithUnknownRowCounts() {
+ tests := []struct {
+ name string
+ addedRows int64
+ existingRows int64
+ }{
+ {name: "both row counts unknown", addedRows: -1, existingRows:
-1},
+ {name: "existing row count unknown", addedRows: 1,
existingRows: -1},
+ {name: "added row count unknown", addedRows: -1, existingRows:
1},
+ }
- var v1Buf bytes.Buffer
- m.Require().NoError(WriteManifestList(1, &v1Buf, snapshotID, nil, nil,
0, []ManifestFile{&legacy}))
- manifests, err := ReadManifestList(&v1Buf)
- m.Require().NoError(err)
- m.Require().Len(manifests, 1)
+ for _, test := range tests {
+ m.Run(test.name, func() {
+ legacy := *(manifestFileRecordsV1[0].(*manifestFile))
+ legacy.AddedRowsCount = test.addedRows
+ legacy.ExistingRowsCount = test.existingRows
- var v3Buf bytes.Buffer
- writer, err := NewManifestListWriterV3(&v3Buf, snapshotID, 1, 1000, nil)
- m.Require().NoError(err)
- m.Require().NoError(writer.AddManifests(manifests))
- m.EqualValues(1000, *writer.NextRowID())
- m.Require().NoError(writer.Close())
+ var v1Buf bytes.Buffer
+ m.Require().NoError(
+ WriteManifestList(1, &v1Buf, snapshotID, nil,
nil, 0, []ManifestFile{&legacy}))
+ manifests, err := ReadManifestList(&v1Buf)
+ m.Require().NoError(err)
+ m.Require().Len(manifests, 1)
+
+ var v3Buf bytes.Buffer
+ writer, err := NewManifestListWriterV3(&v3Buf,
snapshotID, 1, 1000, nil)
+ m.Require().NoError(err)
+ err = writer.AddManifests(manifests)
+ m.Require().ErrorIs(err, ErrInvalidArgument)
+ m.Require().ErrorContains(err, "cannot assign
row-lineage IDs with unknown row counts")
+ m.Require().ErrorContains(err, legacy.Path)
+ m.EqualValues(1000, *writer.NextRowID())
Review Comment:
This assertion is bare `m.EqualValues` while the rest of the block uses
`Require()`, so if the cursor-rollback check fails, execution falls through to
`Close()` and you get an ambiguous second failure. I'd make it
`m.Require().EqualValues` to stop right there. Old test had the same bare
pattern, so no worse than before, just tidier while we're in here.
##########
manifest_test.go:
##########
@@ -3416,23 +3416,41 @@ func (m *ManifestTestSuite)
TestV3ManifestListAcceptsV1AndV2Manifests() {
m.Nil(v2Entry.FirstRowID(), "delete manifests must not be assigned
first_row_id")
}
-func (m *ManifestTestSuite)
TestV3ManifestListAssignsZeroForV1ManifestWithUnknownRowCounts() {
- legacy := *(manifestFileRecordsV1[0].(*manifestFile))
- legacy.AddedRowsCount = -1
- legacy.ExistingRowsCount = -1
+func (m *ManifestTestSuite)
TestV3ManifestListRejectsV1ManifestWithUnknownRowCounts() {
+ tests := []struct {
+ name string
+ addedRows int64
+ existingRows int64
+ }{
+ {name: "both row counts unknown", addedRows: -1, existingRows:
-1},
+ {name: "existing row count unknown", addedRows: 1,
existingRows: -1},
+ {name: "added row count unknown", addedRows: -1, existingRows:
1},
+ }
- var v1Buf bytes.Buffer
- m.Require().NoError(WriteManifestList(1, &v1Buf, snapshotID, nil, nil,
0, []ManifestFile{&legacy}))
- manifests, err := ReadManifestList(&v1Buf)
- m.Require().NoError(err)
- m.Require().Len(manifests, 1)
+ for _, test := range tests {
+ m.Run(test.name, func() {
+ legacy := *(manifestFileRecordsV1[0].(*manifestFile))
+ legacy.AddedRowsCount = test.addedRows
+ legacy.ExistingRowsCount = test.existingRows
- var v3Buf bytes.Buffer
- writer, err := NewManifestListWriterV3(&v3Buf, snapshotID, 1, 1000, nil)
- m.Require().NoError(err)
- m.Require().NoError(writer.AddManifests(manifests))
- m.EqualValues(1000, *writer.NextRowID())
- m.Require().NoError(writer.Close())
+ var v1Buf bytes.Buffer
+ m.Require().NoError(
+ WriteManifestList(1, &v1Buf, snapshotID, nil,
nil, 0, []ManifestFile{&legacy}))
+ manifests, err := ReadManifestList(&v1Buf)
+ m.Require().NoError(err)
+ m.Require().Len(manifests, 1)
+
+ var v3Buf bytes.Buffer
+ writer, err := NewManifestListWriterV3(&v3Buf,
snapshotID, 1, 1000, nil)
+ m.Require().NoError(err)
+ err = writer.AddManifests(manifests)
Review Comment:
All three cases send a single-manifest batch, so `NextRowID() == 1000` and
`Close()` succeeding hold trivially here: nothing was encoded before the error
fired. The case that actually exercises the risk is a mixed batch,
`AddManifests([validV1, unknownCountV1])`, where the first manifest is already
Avro-encoded when the second is rejected.
I'd add that case, assert the error, then read the output back and confirm
we didn't leave a manifest with a colliding `first_row_id` range. That's also
the case that tells us whether the writer is safe to reuse after a partial
failure, so I'd pin the contract down here too: either poison the writer after
a partial-batch error, or document that it isn't reusable.
--
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]
