zeroshade commented on code in PR #2378:
URL: https://github.com/apache/iceberg-go/pull/2378#discussion_r4469869495
##########
catalog/sql/sql.go:
##########
@@ -1566,34 +1569,44 @@ 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, representation view.Representation, props
iceberg.Properties) error {
+func (c *Catalog) CreateView(ctx context.Context, identifier table.Identifier,
version *view.Version, schema *iceberg.Schema, opts ...catalog.CreateViewOpt)
(*view.View, error) {
if c.isV0() {
- return errViewsUnsupportedOnV0
+ return nil, errViewsUnsupportedOnV0
}
if err := catalog.ValidateViewIdentifier(identifier); err != nil {
- return err
+ return nil, err
}
nsIdent := catalog.NamespaceFromIdent(identifier)
viewIdent := catalog.ObjectNameFromIdent(identifier)
ns, exists, err := c.resolveNamespaceKey(ctx, nsIdent)
if err != nil {
- return err
+ return nil, err
}
if !exists {
- return fmt.Errorf("%w: %s", catalog.ErrNoSuchNamespace, ns)
+ return nil, fmt.Errorf("%w: %s", catalog.ErrNoSuchNamespace, ns)
+ }
+
+ cfg := catalog.NewCreateViewCfg()
+ for _, opt := range opts {
+ opt(&cfg)
}
loc, err := internal.ResolveTableLocationWithNamespace(
- ctx, "", nsIdent, ns, viewIdent, c.props,
c.LoadNamespaceProperties)
+ ctx, cfg.Location, nsIdent, ns, viewIdent, c.props,
c.LoadNamespaceProperties)
if err != nil {
- return err
+ return nil, err
}
- createdView, err := view.CreateView(ctx, c.name, identifier, schema,
representation, nsIdent, loc, props)
+ ioProps := maps.Clone(c.props)
+ if ioProps == nil {
+ ioProps = make(iceberg.Properties)
+ }
+ maps.Copy(ioProps, cfg.Properties)
+ createdView, err := view.CreateViewFromVersion(ctx, identifier, schema,
version, loc, ioProps, cfg.Properties)
Review Comment:
The caller's version goes through unchanged, so `default-catalog` stays
empty when the caller doesn't set it. Previously SQL set it to `c.name`, and
REST still does when it's empty. Suggest filling it in from `c.name` before
this call, on a clone so the caller's version isn't modified.
##########
website/src/releases.md:
##########
@@ -42,6 +42,8 @@ Per-release notes (highlights, breaking changes,
contributors) are published on
### Unreleased compatibility notes
+- **SQL view APIs align with `catalog.ViewCatalog`.** `CreateView` takes a
`*view.Version`, schema and options, and returns `*view.View`. `LoadView`
returns `*view.View`; use `Metadata()` to access its metadata.
Review Comment:
The blank line after this bullet splits the list in two. If
`default-catalog` is intentionally left empty, please say so here, as the Hive
note does.
##########
catalog/sql/sql_test.go:
##########
@@ -3674,3 +3674,43 @@ func (s *SqliteCatalogTestSuite) TestLoadView() {
func TestSqlCatalog(t *testing.T) {
suite.Run(t, new(SqliteCatalogTestSuite))
}
+
+func createTestView(cat *sqlcat.Catalog, ctx context.Context, identifier
table.Identifier, schema *iceberg.Schema, representation view.Representation,
props iceberg.Properties) error {
Review Comment:
Nit: Go convention puts `ctx` first (`createTestView(ctx, cat, …)`); the
same applies to the copy in `drop_namespace_internal_test.go`.
--
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]
