laskoviymishka commented on code in PR #1387:
URL: https://github.com/apache/iceberg-go/pull/1387#discussion_r3536301370
##########
view/metadata.go:
##########
@@ -518,16 +518,24 @@ func NewMetadataWithUUID(version *Version, sc
*iceberg.Schema, location string,
viewUUID = uuid.New()
}
+ var inputProps iceberg.Properties
formatVersion := DefaultViewFormatVersion
if props != nil {
- verStr, ok := props["format-version"]
+ inputProps = maps.Clone(props)
+ verStr, ok := inputProps[table.PropertyFormatVersion]
if ok {
var err error
if formatVersion, err = strconv.Atoi(verStr); err !=
nil {
- formatVersion = DefaultViewFormatVersion
+ return nil, fmt.Errorf("%w: %s",
iceberg.ErrInvalidFormatVersion, verStr)
}
- delete(props, "format-version")
+ delete(inputProps, table.PropertyFormatVersion)
}
+ } else {
+ inputProps = props
+ }
+
+ if formatVersion != DefaultViewFormatVersion {
Review Comment:
I'd make this a range check rather than an equality check — `if
formatVersion > SupportedViewFormatVersion`. Right now it happens to be
equivalent since both constants are 1, but once view V2 lands and they bump,
this rejects a valid `"1"` and every other still-supported version below the
max. The table path deliberately accepts anything in `[1, supported]` and lets
the builder do range validation; I'd mirror that here so the two constructors
don't drift. wdyt?
##########
view/metadata.go:
##########
@@ -518,16 +518,24 @@ func NewMetadataWithUUID(version *Version, sc
*iceberg.Schema, location string,
viewUUID = uuid.New()
}
+ var inputProps iceberg.Properties
formatVersion := DefaultViewFormatVersion
if props != nil {
- verStr, ok := props["format-version"]
+ inputProps = maps.Clone(props)
+ verStr, ok := inputProps[table.PropertyFormatVersion]
if ok {
var err error
if formatVersion, err = strconv.Atoi(verStr); err !=
nil {
- formatVersion = DefaultViewFormatVersion
+ return nil, fmt.Errorf("%w: %s",
iceberg.ErrInvalidFormatVersion, verStr)
Review Comment:
Heads up that the view package already has its own
`ErrInvalidViewMetadataFormatVersion`, used by `validateViewMetadata` on the
deserialization path. Wrapping `iceberg.ErrInvalidFormatVersion` here means the
same package now has two sentinels for the same kind of failure, so anyone
doing `errors.Is(err, view.ErrInvalidViewMetadataFormatVersion)` will miss
constructor errors. I'd lean toward reusing the existing view sentinel here for
consistency — thoughts?
##########
table/metadata.go:
##########
@@ -2205,16 +2205,20 @@ func NewMetadataWithUUID(sc *iceberg.Schema, partitions
*iceberg.PartitionSpec,
if tableUuid == uuid.Nil {
tableUuid = uuid.New()
}
+ var inputProps iceberg.Properties
var err error
formatVersion := DefaultFormatVersion
if props != nil {
- verStr, ok := props[PropertyFormatVersion]
+ inputProps = maps.Clone(props)
+ verStr, ok := inputProps[PropertyFormatVersion]
if ok {
if formatVersion, err = strconv.Atoi(verStr); err !=
nil {
- formatVersion = DefaultFormatVersion
+ return nil, fmt.Errorf("%w: %s",
iceberg.ErrInvalidFormatVersion, verStr)
}
- delete(props, PropertyFormatVersion)
+ delete(inputProps, PropertyFormatVersion)
}
+ } else {
Review Comment:
This `else` is a no-op — `inputProps` is already the nil zero value and we
only get here when `props == nil`, so it assigns nil to nil. I'd drop the whole
branch and just do `inputProps := maps.Clone(props)` up top (`maps.Clone(nil)`
returns nil), gating the key lookup on `props != nil`. Same dead `else` over in
`view/metadata.go` around line 534.
##########
table/metadata.go:
##########
@@ -2205,16 +2205,20 @@ func NewMetadataWithUUID(sc *iceberg.Schema, partitions
*iceberg.PartitionSpec,
if tableUuid == uuid.Nil {
tableUuid = uuid.New()
}
+ var inputProps iceberg.Properties
var err error
formatVersion := DefaultFormatVersion
if props != nil {
- verStr, ok := props[PropertyFormatVersion]
+ inputProps = maps.Clone(props)
+ verStr, ok := inputProps[PropertyFormatVersion]
if ok {
if formatVersion, err = strconv.Atoi(verStr); err !=
nil {
- formatVersion = DefaultFormatVersion
+ return nil, fmt.Errorf("%w: %s",
iceberg.ErrInvalidFormatVersion, verStr)
Review Comment:
This rejects non-numeric strings but a numeric-but-out-of-range value like
`"0"` or `"99"` still sails past here and only gets caught later by
`NewMetadataBuilder`. Both wrap the same sentinel so `errors.Is` is fine, but
the error origin and message differ depending on the failure mode. Is that
intentional? If we want the constructor to be the single validation point I'd
add a range check right after the `Atoi`.
##########
table/metadata_internal_test.go:
##########
@@ -780,6 +780,22 @@ func TestNewMetadataWithExplicitV1Format(t *testing.T) {
assert.Truef(t, expected.Equals(actual), "expected: %s\ngot: %s",
expected, actual)
}
+func TestNewMetadataRejectInvalidFormatVersion(t *testing.T) {
+ props := iceberg.Properties{
+ "format-version": "banana",
Review Comment:
This only exercises the non-numeric case. The numeric out-of-range path
(`"0"`, `"99"`) goes through a different branch — the builder rather than this
constructor — so it's worth a case here too, especially if we tighten the
constructor validation. The view test is already table-driven; might be worth
matching that shape so both add the extra case in one spot.
##########
view/metadata_test.go:
##########
@@ -88,6 +89,46 @@ func TestNewMetadata(t *testing.T) {
assert.Equal(t, []VersionLogEntry{{TimestampMS: 1000, VersionID: 1}},
md.VersionLog())
}
+func TestNewMetadataRejectInvalidFormatVersion(t *testing.T) {
+ tests := []struct {
+ name string
+ formatVer string
+ versionValue string
Review Comment:
`versionValue` is always equal to `formatVer` in both cases — I'd drop it
and just assert against `tc.formatVer`. Separately, this file mixes the new
`table.PropertyFormatVersion` constant with the raw `"format-version"` literal
the other ~30 cases use; I'd pick one (raw literal avoids the `table` import
entirely) so the file isn't half-and-half.
--
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]
