zeroshade commented on code in PR #1384:
URL: https://github.com/apache/iceberg-go/pull/1384#discussion_r3523577586
##########
view/metadata.go:
##########
@@ -173,6 +173,28 @@ func NewVersion(id int64, schemaID int, representations
[]Representation, defaul
return nil, errors.New("invalid view version: must have at
least one representation")
}
+ seenDialects := make(map[string]struct{})
+ for _, repr := range representations {
+ if repr.Type != "sql" {
+ return nil, errors.New("invalid view representation:
type should be \"sql\"")
+ }
+
Review Comment:
These new representation validation errors should wrap
ErrInvalidViewMetadata, like the duplicate-dialect error below and the rest of
metadata validation. As written, errors.Is(err, ErrInvalidViewMetadata) is
inconsistent for invalid representation cases. Please wrap the type/sql/dialect
errors with ErrInvalidViewMetadata.
##########
view/metadata.go:
##########
@@ -173,6 +173,28 @@ func NewVersion(id int64, schemaID int, representations
[]Representation, defaul
return nil, errors.New("invalid view version: must have at
least one representation")
}
+ seenDialects := make(map[string]struct{})
+ for _, repr := range representations {
+ if repr.Type != "sql" {
Review Comment:
The NewVersion checks are a good addition, but this leaves the public
builder path able to bypass them. Callers can still construct a *Version
directly and pass it to MetadataBuilder.AddVersion; addVersion only checks
empty representations/duplicate dialects and metadata.validate only calls
checkDialectsUnique, so non-sql types or blank/whitespace SQL/dialect values
can still be built. Please extract shared representation validation and call it
from NewVersion, MetadataBuilder.addVersion, and metadata.validate.
--
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]
