zeroshade commented on code in PR #1384:
URL: https://github.com/apache/iceberg-go/pull/1384#discussion_r3566743546
##########
view/metadata.go:
##########
@@ -219,11 +250,11 @@ func (v *Version) Clone() *Version {
}
// sqlDialects returns a set of strings representing the SQL dialects
supported in this version.
-// Dialects are deduplicated by lowercase comparison
+// Dialects are deduplicated by trimmed, lowercase comparison.
func (v *Version) sqlDialects() internal.Set[string] {
return internal.ToSet(internal.MapSlice(
v.Representations,
- func(r Representation) string { return
strings.ToLower(r.Dialect) },
+ func(r Representation) string { return
strings.ToLower(strings.TrimSpace(r.Dialect)) },
Review Comment:
`sqlDialects()` maps over *all* representations without filtering to `Type
== "sql"`. Since unknown/non-sql representation types are now permitted on
reads, such a representation contributes its (or an empty) dialect into the set
used by the dialect-drop / uniqueness check. That can make
`MetadataBuilderFromBase(...).SetCurrentVersion(...).Build()` fail for metadata
parsed from JSON that contains a non-sql representation, even though
`checkDialectsUnique` skips them. Please filter to `Type == "sql"` before
collecting dialects, and add a round-trip regression test with a non-sql
representation present.
--
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]
