laskoviymishka commented on code in PR #1384:
URL: https://github.com/apache/iceberg-go/pull/1384#discussion_r3560077531
##########
view/metadata.go:
##########
@@ -189,6 +201,23 @@ func NewVersion(id int64, schemaID int, representations
[]Representation, defaul
return version, nil
}
+func validateRepresentation(repr Representation) (string, error) {
Review Comment:
Minor: `validateRepresentation` also normalizes and returns the dialect, so
`validate` is doing double duty. It works, but a future caller could easily
call it for the check, discard the return, and keep using `repr.Dialect` —
reintroducing the exact case/whitespace inconsistency this is fixing.
Not blocking — either a rename toward `normalizeRepresentation`, or a
one-line doc on the return, would make the contract obvious.
##########
view/metadata.go:
##########
@@ -446,10 +475,14 @@ func (m *metadata) checkDialectsUnique() error {
for _, version := range m.VersionList {
seenDialects := make(map[string]bool)
for _, repr := range version.Representations {
- dialect := strings.ToLower(repr.Dialect)
+ dialect, err := validateRepresentation(repr)
+ if err != nil {
+ return fmt.Errorf("%w: version %d: %s",
ErrInvalidViewMetadata, version.VersionID, err)
+ }
+
if seenDialects[dialect] {
return fmt.Errorf("%w: version %d has duplicate
dialect %s",
- ErrInvalidViewMetadata,
version.VersionID, repr.Dialect)
+ ErrInvalidViewMetadata,
version.VersionID, dialect)
Review Comment:
Small one, but worth a note: this now prints the normalized (trimmed +
lowercased) dialect in the duplicate error, where before it was `repr.Dialect`
as-written. The `TestDuplicateDialects` change (`"SPARK"` → `" SPARK "`,
asserting `"spark"`) quietly bakes that in, so the behavior change isn't
obvious from the test.
I'd pass `repr.Dialect` to the message and keep the normalized value only as
the dedup key — closer to what someone reading the error would expect to see.
##########
view/metadata.go:
##########
@@ -446,10 +475,14 @@ func (m *metadata) checkDialectsUnique() error {
for _, version := range m.VersionList {
seenDialects := make(map[string]bool)
for _, repr := range version.Representations {
- dialect := strings.ToLower(repr.Dialect)
+ dialect, err := validateRepresentation(repr)
Review Comment:
This is the one thing I'd want to tighten before merge. Wiring
`validateRepresentation` into `checkDialectsUnique` means the read path
(`UnmarshalJSON` → `validate`) now rejects any stored metadata with a non-`sql`
type or empty sql/dialect — but Java tolerates unknown representation types via
`UnknownViewRepresentation` and only runs dialect dedup over
`SQLViewRepresentation`. So a view that Spark/Java writes today with a future
representation type loads fine there and fails to parse here.
I'd keep the strict content validation where it belongs — on the write paths
(`NewVersion`, `addVersion`) — and on the read path only dedup, guarding the
content checks behind `if repr.Type == "sql"` so we skip unknown types the way
Java does. Otherwise upgrading the library becomes a breaking change for anyone
holding a stored view we'd now refuse to open. wdyt?
##########
view/metadata_builder_test.go:
##########
@@ -97,6 +97,84 @@ func TestBuild_NullAndMissingFields(t *testing.T) {
assert.ErrorContains(t, err, "cannot set uuid to null")
}
+func TestNewVersion_RepresentationValidation(t *testing.T) {
Review Comment:
These cases are all sequential `_, err =` assignments sharing one `err`, so
if any assertion `t.Fatal`s the rest silently skip — and the happy path is
mixed in with the error cases. `TestInvalidRepresentationInJSON` right below
already does the table + `t.Run` version of this; I'd match that here.
One gap worth adding while restructuring: every invalid case uses a single
bad representation. A `[{valid}, {type: "hive"}]` case — first repr good,
second bad — would cover the loop actually reaching the second element, which
is the one that'd regress silently if the iteration order ever changed.
##########
view/metadata.go:
##########
@@ -189,6 +201,23 @@ func NewVersion(id int64, schemaID int, representations
[]Representation, defaul
return version, nil
}
+func validateRepresentation(repr Representation) (string, error) {
+ if repr.Type != "sql" {
+ return "", errors.New("invalid view representation: type should
be \"sql\"")
+ }
+
+ if strings.TrimSpace(repr.Sql) == "" {
+ return "", errors.New("invalid view representation: sql is
required")
+ }
+
+ dialect := strings.TrimSpace(repr.Dialect)
+ if dialect == "" {
+ return "", errors.New("invalid view representation: dialect is
required")
+ }
+
+ return strings.ToLower(dialect), nil
Review Comment:
While we're here — now that this canonicalizes with trim-then-lowercase,
`sqlDialects()` just below is out of step: it still does
`strings.ToLower(r.Dialect)` without the `TrimSpace`. A stored `" spark "`
normalizes to `"spark"` through this helper but stays padded in
`sqlDialects()`, and that key feeds the drop-dialect guard in
`checkIfDialectIsDropped` — so the two disagree on whether a dialect is present
and we could throw a false "dropping dialects" error.
I'd give `sqlDialects()` the same trim before lowercasing so there's one
canonical form.
--
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]
