zeroshade commented on code in PR #2324:
URL: https://github.com/apache/iceberg-go/pull/2324#discussion_r4381401556
##########
catalog/sql/sql.go:
##########
@@ -1563,7 +1563,7 @@ func (c *Catalog) CheckNamespaceExists(ctx
context.Context, namespace table.Iden
}
// 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 {
Review Comment:
Same for `sql.Catalog.CreateView`: `viewSQL, viewDialect string` can be
swapped silently.
##########
view/view.go:
##########
@@ -137,33 +137,37 @@ func NewFromLocation(
//
// Note: This function only supports creating new views with format version 1.
// It does not support updating existing view metadata.
+// viewDialect is the SQL engine dialect to store in the view representation.
func CreateView(
ctx context.Context,
catalogName string,
viewIdent table.Identifier,
schema *iceberg.Schema,
viewSQL string,
+ viewDialect string,
Review 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?
##########
view/metadata.go:
##########
@@ -109,12 +109,12 @@ type Representation struct {
}
type Representations []Representation
-func NewRepresentation(sql string, dialect string) Representation {
- return Representation{
- Type: "sql",
- SQL: sql,
- Dialect: dialect,
- }
+// NewRepresentation creates a SQL view representation from named fields.
+// The SQL and Dialect fields are validated by NewVersion.
+func NewRepresentation(repr Representation) Representation {
Review 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.
##########
view/metadata.go:
##########
@@ -223,9 +223,9 @@ func validateRepresentation(repr Representation) (string,
error) {
}
// NewVersionFromSQL creates a new Version with a single representation
-// using the provided SQL and dialect "default".
-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) {
Review Comment:
Same here: `NewVersionFromSQL(id, schemaID, sql, dialect string, ...)` puts
the pair back as positional strings. Taking a `Representation` would match
`NewRepresentation`.
--
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]
