laskoviymishka commented on code in PR #1334:
URL: https://github.com/apache/iceberg-go/pull/1334#discussion_r3491453668
##########
manifest_test.go:
##########
@@ -2023,6 +2024,101 @@ func (m *ManifestTestSuite)
TestV3ManifestListWriterPersistsPerManifestFirstRowI
m.EqualValues(5022, *writer.NextRowID())
}
+func (m *ManifestTestSuite)
TestManifestListWriterMetadataPreservesInt64Values() {
+ parentSnapshot := int64(1 << 40)
+
+ tests := []struct {
+ name string
+ build func(io.Writer) (*ManifestListWriter, error)
+ expected map[string]string
+ }{
+ {
+ name: "v1",
+ build: func(w io.Writer) (*ManifestListWriter, error) {
+ return NewManifestListWriterV1(w, (1<<40)+1,
&parentSnapshot)
+ },
+ expected: map[string]string{
+ "format-version": "1",
+ "snapshot-id": "1099511627777",
Review Comment:
These string literals need mental arithmetic to tie back to the `(1<<40)+N`
inputs. The `v3_near_max_int64` subtest gets this right by computing the
expected value (`strconv.FormatInt(...)`) instead of hardcoding it. Could we
match that here so the table is self-documenting and can't silently drift from
the inputs?
##########
manifest_test.go:
##########
@@ -2023,6 +2024,101 @@ func (m *ManifestTestSuite)
TestV3ManifestListWriterPersistsPerManifestFirstRowI
m.EqualValues(5022, *writer.NextRowID())
}
+func (m *ManifestTestSuite)
TestManifestListWriterMetadataPreservesInt64Values() {
+ parentSnapshot := int64(1 << 40)
+
+ tests := []struct {
+ name string
+ build func(io.Writer) (*ManifestListWriter, error)
+ expected map[string]string
+ }{
+ {
+ name: "v1",
+ build: func(w io.Writer) (*ManifestListWriter, error) {
+ return NewManifestListWriterV1(w, (1<<40)+1,
&parentSnapshot)
+ },
+ expected: map[string]string{
+ "format-version": "1",
+ "snapshot-id": "1099511627777",
+ "parent-snapshot-id": "1099511627776",
+ },
+ },
+ {
+ name: "v2",
+ build: func(w io.Writer) (*ManifestListWriter, error) {
+ return NewManifestListWriterV2(w, (1<<40)+2,
(1<<40)+3, &parentSnapshot)
+ },
+ expected: map[string]string{
+ "format-version": "2",
+ "snapshot-id": "1099511627778",
+ "sequence-number": "1099511627779",
+ "parent-snapshot-id": "1099511627776",
+ },
+ },
+ {
+ name: "v3",
+ build: func(w io.Writer) (*ManifestListWriter, error) {
+ return NewManifestListWriterV3(w, (1<<40)+4,
(1<<40)+5, (1<<40)+6, &parentSnapshot)
+ },
+ expected: map[string]string{
+ "format-version": "3",
+ "snapshot-id": "1099511627780",
+ "sequence-number": "1099511627781",
+ "first-row-id": "1099511627782",
+ "parent-snapshot-id": "1099511627776",
+ },
+ },
+ }
+
+ for _, tt := range tests {
+ m.Run(tt.name, func() {
+ var buf bytes.Buffer
+ writer, err := tt.build(&buf)
+ m.Require().NoError(err)
+ m.Require().NoError(writer.Close())
+
+ reader, err := ocf.NewReader(&buf)
+ m.Require().NoError(err)
+ defer func() {
+ m.Require().NoError(reader.Close())
+ }()
+
+ meta := reader.Metadata()
+ for key, want := range tt.expected {
+ m.Equal(want, string(meta[key]))
Review Comment:
I think this test is a no-op on our actual CI. On a 64-bit host `int` is
64-bit, so `strconv.Itoa(int(x))` and `strconv.FormatInt(x, 10)` produce
identical bytes for every value here — including the near-MaxInt64 subtest
below. It'll pass against the pre-fix code too, and there's no GOARCH=386/wasm
step in the matrix to exercise the truncating path, so it doesn't actually
fence the bug on any supported target.
Could we make it fail without the fix? A `// +build 386`-tagged variant that
asserts the truncated vs. full output differ would document the gap, or —
simpler — a platform-independent invariant: round-trip the value through `int64
→ string → ParseInt` and assert equality, which only holds when the writer used
the 64-bit formatter. wdyt?
Separately, while we're here: the iteration over `tt.expected` is
map-ordered, so a failure cites a different key each run. Worth passing the key
as msgAndArgs — `m.Equal(want, string(meta[key]), "OCF metadata key %q", key)`
— so failures are reproducible.
##########
manifest_test.go:
##########
@@ -2023,6 +2024,101 @@ func (m *ManifestTestSuite)
TestV3ManifestListWriterPersistsPerManifestFirstRowI
m.EqualValues(5022, *writer.NextRowID())
}
+func (m *ManifestTestSuite)
TestManifestListWriterMetadataPreservesInt64Values() {
+ parentSnapshot := int64(1 << 40)
+
+ tests := []struct {
+ name string
+ build func(io.Writer) (*ManifestListWriter, error)
+ expected map[string]string
+ }{
+ {
+ name: "v1",
+ build: func(w io.Writer) (*ManifestListWriter, error) {
+ return NewManifestListWriterV1(w, (1<<40)+1,
&parentSnapshot)
+ },
+ expected: map[string]string{
+ "format-version": "1",
+ "snapshot-id": "1099511627777",
+ "parent-snapshot-id": "1099511627776",
+ },
+ },
+ {
+ name: "v2",
+ build: func(w io.Writer) (*ManifestListWriter, error) {
+ return NewManifestListWriterV2(w, (1<<40)+2,
(1<<40)+3, &parentSnapshot)
+ },
+ expected: map[string]string{
+ "format-version": "2",
+ "snapshot-id": "1099511627778",
+ "sequence-number": "1099511627779",
+ "parent-snapshot-id": "1099511627776",
+ },
+ },
+ {
+ name: "v3",
+ build: func(w io.Writer) (*ManifestListWriter, error) {
+ return NewManifestListWriterV3(w, (1<<40)+4,
(1<<40)+5, (1<<40)+6, &parentSnapshot)
+ },
+ expected: map[string]string{
+ "format-version": "3",
+ "snapshot-id": "1099511627780",
+ "sequence-number": "1099511627781",
+ "first-row-id": "1099511627782",
+ "parent-snapshot-id": "1099511627776",
+ },
+ },
+ }
+
+ for _, tt := range tests {
+ m.Run(tt.name, func() {
+ var buf bytes.Buffer
+ writer, err := tt.build(&buf)
+ m.Require().NoError(err)
+ m.Require().NoError(writer.Close())
+
+ reader, err := ocf.NewReader(&buf)
+ m.Require().NoError(err)
+ defer func() {
Review Comment:
A deferred `Require` (which does `FailNow` → `Goexit`) can surface a
`Close()` error as a confusing panic rather than a clean failure in some
testify versions. The rest of this file closes readers inline with
`m.NoError(reader.Close())` after the assertions. I'd either move the close
inline or drop to a plain `defer reader.Close()` if the close error isn't
meaningful here — applies to both subtests.
--
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]
