laskoviymishka commented on code in PR #1587:
URL: https://github.com/apache/iceberg-go/pull/1587#discussion_r3680740605
##########
puffin/puffin_test.go:
##########
@@ -347,6 +348,28 @@ func TestWriterValidation(t *testing.T) {
assert.ErrorContains(t, err, "not a valid non-negative integer")
})
+ t.Run("deletion vector maximum cardinality", func(t *testing.T) {
Review Comment:
While we're adding these subtests — the `ParseUint` comments on the two
sibling cases just above (~328 and ~344) are now stale. The impl moved to
`ParseInt` + an explicit `< 0` guard, so "ParseUint covers both" / "ParseUint
catches this via the same invalid-syntax path" is describing a function that's
no longer called.
Behavior described is still correct, just the mechanism name drifted. I'd
update them to `ParseInt` in the same pass.
##########
puffin/puffin_writer.go:
##########
@@ -158,15 +158,17 @@ func (w *Writer) AddBlob(input BlobMetadataInput, data
[]byte) (BlobMetadata, er
if properties["cardinality"] == "" {
return BlobMetadata{}, errors.New("puffin:
deletion-vector-v1 requires a cardinality property")
}
- // Reject non-numeric or negative values at write time too —
otherwise
- // a writer could emit "cardinality": "abc" or "-1" that the
reader
- // hard-rejects later. ParseUint covers both: "-1" fails as
invalid
- // syntax (the minus is rejected before any value is parsed), so
- // non-numeric and negative collapse into one error path.
- if _, err := strconv.ParseUint(properties["cardinality"], 10,
64); err != nil {
+ // Parse the same signed range used by deletion-vector readers
and
+ // manifest record counts so every emitted value is readable.
+ cardinality, err := strconv.ParseInt(properties["cardinality"],
10, 64)
+ if err != nil {
return BlobMetadata{}, fmt.Errorf("puffin:
deletion-vector-v1 cardinality property %q is not a valid non-negative integer:
%w",
properties["cardinality"], err)
}
+ if cardinality < 0 {
+ return BlobMetadata{}, fmt.Errorf("puffin:
deletion-vector-v1 cardinality property %q is not a valid non-negative integer",
Review Comment:
The parse-error path above wraps with `%w` and this one doesn't — which is
right, there's no underlying error to wrap here. Only thing is it's easy to
skim and read as an accidental `%w` drop. A trailing `// no underlying error to
wrap` on the `fmt.Errorf` would make the asymmetry intentional at a glance.
Non-blocking.
--
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]
