tanmayrauth commented on code in PR #1387:
URL: https://github.com/apache/iceberg-go/pull/1387#discussion_r3525652161
##########
view/metadata.go:
##########
@@ -518,16 +518,20 @@ 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["format-version"]
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:
This wraps ErrInvalidFormatVersion for a non-numeric value, but a
numeric-but-invalid view version doesn't get the same treatment: format-version
"2" / "0" / "-1" flow through to the builder's SetFormatVersion
(view/metadata_builder.go:322/315), which returns plain strings like
"unsupported format version 2" and "downgrading format version from 1 to 0 is
not allowed" — neither satisfies errors.Is(err, ErrInvalidFormatVersion). The
table path wraps the sentinel for those cases (via NewMetadataBuilder), so the
two constructors behave differently. Since the goal here is a uniform
ErrInvalidFormatVersion on bad versions, would you want to wrap the sentinel in
the view
builder too (or is that intentionally out of scope for this PR)?
##########
view/metadata.go:
##########
@@ -518,16 +518,20 @@ 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["format-version"]
Review Comment:
Nit: the table side keys off the PropertyFormatVersion constant while the
view side repeats the "format-version" literal (also at lines 531, 257, 298).
Not introduced by this PR, but a package-level const would avoid drift if the
key ever changes.
--
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]
