tanmayrauth commented on code in PR #1384:
URL: https://github.com/apache/iceberg-go/pull/1384#discussion_r3523603968
##########
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\"")
+ }
+
+ if strings.TrimSpace(repr.Sql) == "" {
+ return nil, errors.New("invalid view representation:
sql is required")
+ }
+
+ dialect := strings.TrimSpace(repr.Dialect)
+ if dialect == "" {
+ return nil, errors.New("invalid view representation:
dialect is required")
+ }
+
+ normalizedDialect := strings.ToLower(dialect)
Review Comment:
One thing to settle when you extract the helper: this dedup normalizes with
TrimSpace + ToLower, but addVersion (metadata_builder.go:184) and
checkDialectsUnique (metadata.go:449) only ToLower without the trim — so "
spark " vs "spark" collide here but stay distinct in the other two paths.
Picking trim+lower in the shared helper fixes that, and also lets you unify the
message (checkDialectsUnique says "version %d has duplicate dialect %s" while
these say "Cannot add multiple queries for dialect %s").
##########
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:
+1 to wrapping these with ErrInvalidViewMetadata — this line plus :183 (sql)
and :188 (dialect) use bare errors.New, so errors.Is(err,
ErrInvalidViewMetadata) is false for them while the dup-dialect error just
below at :193 wraps it. Worth pulling all of this into one
validateRepresentations(reprs) helper and calling it from NewVersion,
addVersion (metadata_builder.go:179-189), and metadata.validate — right now the
builder only checks empty/dup-dialect and validate() only calls
checkDialectsUnique, so a hand-built *Version or any metadata coming through
ParseMetadataBytes/UnmarshalJSON can still carry a non-sql type or blank
sql/dialect. The parse path is the one I'd most want covered since that's
untrusted input from disk.
--
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]
