Repository navigation
Conversation
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
2bb9140 to
5da8a60
Compare
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
The Hive change is a real fix: CreateView now keeps the chosen representation's dialect instead of rewriting it to default, and view_dialect_test.go pins it. Requiring an explicit dialect also removes a hidden default. Merged with current main, vet (including -tags integration) and the view, Hive and SQL catalog tests pass.
The PR moves NewRepresentation to named fields because SQL and dialect are both strings and can be swapped, but then adds new positional viewSQL, viewDialect string pairs to view.CreateView, CreateViewWithIOProperties, NewVersionFromSQL and sql.Catalog.CreateView, which is the same swap hazard (details inline). Since these are breaking changes anyway, please make them consistent. Also, the two compatibility notes from 5da8a60 were dropped by the "sync release notes with main" commit; these APIs are in v0.7.0, so please restore them.
| viewIdent table.Identifier, | ||
| schema *iceberg.Schema, | ||
| viewSQL string, | ||
| viewDialect string, |
There was a problem hiding this comment.
These two adjacent string parameters are the swap hazard this PR removes from NewRepresentation, and since both are non-empty, validation can't catch a swap. Could CreateView and CreateViewWithIOProperties take a view.Representation ({SQL: ..., Dialect: ...}) instead?
| func NewVersionFromSQL(id int64, schemaID int, sql string, defaultNS table.Identifier, opts ...VersionOpt) (*Version, error) { | ||
| return NewVersion(id, schemaID, Representations{NewRepresentation(sql, "default")}, defaultNS, opts...) | ||
| // using the provided SQL and dialect. | ||
| func NewVersionFromSQL(id int64, schemaID int, sql, dialect string, defaultNS table.Identifier, opts ...VersionOpt) (*Version, error) { |
There was a problem hiding this comment.
Same here: NewVersionFromSQL(id, schemaID, sql, dialect string, ...) puts the pair back as positional strings. Taking a Representation would match NewRepresentation.
|
|
||
| // CreateView creates a new view in the catalog. | ||
| func (c *Catalog) CreateView(ctx context.Context, identifier table.Identifier, schema *iceberg.Schema, viewSQL string, props iceberg.Properties) error { | ||
| func (c *Catalog) CreateView(ctx context.Context, identifier table.Identifier, schema *iceberg.Schema, viewSQL, viewDialect string, props iceberg.Properties) error { |
There was a problem hiding this comment.
Same for sql.Catalog.CreateView: viewSQL, viewDialect string can be swapped silently.
| } | ||
| // NewRepresentation creates a SQL view representation from named fields. | ||
| // The SQL and Dialect fields are validated by NewVersion. | ||
| func NewRepresentation(repr Representation) Representation { |
There was a problem hiding this comment.
Non-blocking: with named fields, this only sets Type = "sql" on its argument. It's fine to keep as a helper, but NewVersion could also treat an empty Type as "sql" so callers can use the struct literal directly. Your call.
Preserve current catalog changes and view migration notes.
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Thanks, this is the consistent shape: the creation APIs and NewVersionFromSQL now take a view.Representation, so SQL and dialect can't be swapped anywhere, Hive passes the selected representation through intact, and both compatibility notes are back. Your merge of main (6c8a240) merges cleanly with current main, and go vet ./... (including -tags integration) and the view, Hive and SQL catalog tests pass on it. LGTM.
apache#2324's two notes landed separated by blank lines, splitting the list.
What
Why
Implementation
Refs #2062.