zeroshade commented on code in PR #2068:
URL: https://github.com/apache/iceberg-go/pull/2068#discussion_r4158537485
##########
table/table.go:
##########
@@ -1400,6 +1408,18 @@ func WithScanPlanningIOProperties(props
iceberg.Properties) Option {
}
}
+// WithLabels attaches catalog-provided labels from a load response to the
+// table. A nil value is ignored, leaving the table's labels nil.
+func WithLabels(l *iceberg.Labels) Option {
Review Comment:
Labels are lost on every commit. `doCommit` (table/table.go:827-836) builds
the post-commit table with `New(..., withReporterState(t.reporter,
t.reporterSet), WithScanPlanningIOProperties(t.scanPlanningIOProps))` and never
passes `WithLabels`. `Transaction.StagedTable` (table/transaction.go:3321-3330)
has the same gap. So `cat.LoadTable` returns a table with labels, but the
`*Table` returned by `tbl.AppendTable(...)` or `tx.Commit(ctx)` has `Labels()
== nil` until the caller does a `Refresh`/`LoadTable`. Java keeps them on the
same `BaseTable` across commits (`private final transient Labels labels`). The
`UpdateTable` nil only covers the REST UpdateTable return value, not this path.
Fix: add `WithLabels(t.labels)` to the `New(...)` in `doCommit` and
`WithLabels(t.tbl.labels)` in `StagedTable`. Also add a table-package test:
load a table with labels from a stub catalog, commit, and assert `Labels()` is
still set on the returned table.
##########
table/table.go:
##########
@@ -239,6 +246,7 @@ func (t *Table) Refresh(ctx context.Context) error {
t.manifestCache = newSnapshotManifestCacheForMetadata(fresh.metadata)
t.planner = fresh.planner
t.scanPlanningIOProps = maps.Clone(fresh.scanPlanningIOProps)
+ t.labels = fresh.labels
Review Comment:
Nothing tests this line. Please add a stub-catalog test in the table
package, like the Refresh tests in `metrics_wiring_test.go`: the catalog
returns a table with labels, and after `Refresh` the labels are set. Some other
claims in the description also have no test:
- `CreateTable` forwarding `ret.Labels`
- `UpdateView` forwarding labels from `viewResponse`
- `Table.Equals`/`View.Equals` ignoring labels
The REST ones are a few lines each with the existing fixtures.
##########
table/table.go:
##########
@@ -134,6 +137,10 @@ func (t Table) Spec() iceberg.PartitionSpec
{ return t.metadata
func (t Table) SortOrder() SortOrder { return
t.metadata.SortOrder() }
func (t Table) Properties() iceberg.Properties { return
t.metadata.Properties() }
+// Labels returns the catalog-provided labels from the load response, or nil if
+// the catalog returned none. Labels are transient enrichment, not table state.
+func (t Table) Labels() *iceberg.Labels { return t.labels }
Review Comment:
nit: this returns the internal pointer, and so does `View.Labels()`
(view/view.go:62), so a caller can change labels on a shared table or view.
`Properties()` already returns its map the same way, so I'm fine with the
shape. Please say in the doc comment that the returned value must be treated as
read-only.
##########
catalog/rest/rest_test.go:
##########
@@ -2953,6 +3092,62 @@ func (r *RestCatalogSuite) TestRegisterView200() {
r.Equal(exampleViewSQL,
v.Metadata().CurrentVersion().Representations[0].Sql)
}
+func (r *RestCatalogSuite) TestRegisterViewLabels() {
+ const (
+ ns = "fokko"
+ viewName = "myview"
+ metadataLoc =
"s3://bucket/warehouse/fokko.db/myview/metadata/00001.metadata.json"
+ )
+
+ r.mux.HandleFunc("/v1/namespaces/"+ns+"/register-view", func(w
http.ResponseWriter, req *http.Request) {
+ r.Require().Equal(http.MethodPost, req.Method)
+ w.Header().Set("Content-Type", "application/json")
+ fmt.Fprintf(w, `{"metadata-location": %q, "metadata": %s,
"config": {}, "labels": {"object-labels": {"owner": "analytics"}}}`,
+ metadataLoc, exampleViewMetadataJSON)
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL,
rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ v, err := cat.RegisterView(context.Background(), table.Identifier{ns,
viewName}, metadataLoc)
+ r.Require().NoError(err)
+
+ labels := v.Labels()
+ r.Require().NotNil(labels)
+ r.Equal(iceberg.Properties{"owner": "analytics"}, labels.Object())
+}
+
+func (r *RestCatalogSuite) TestCreateViewLabels() {
+ ns := "ns"
+ viewName := "view"
+ identifier := table.Identifier{ns, viewName}
+ schema := iceberg.NewSchemaWithIdentifiers(0, []int{1},
iceberg.NestedField{
+ ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int32,
Required: true,
+ })
+ reprs := []view.Representation{view.NewRepresentation(exampleViewSQL,
"default")}
+ version, err := view.NewVersion(1, 0, reprs, table.Identifier{ns},
+ view.WithDefaultViewCatalog("default-catalog"),
view.WithTimestampMS(0))
+ r.Require().NoError(err)
+
+ r.mux.HandleFunc("/v1/namespaces/"+ns+"/views", func(w
http.ResponseWriter, req *http.Request) {
+ r.Equal(http.MethodPost, req.Method)
+ w.Header().Set("Content-Type", "application/json")
+ fmt.Fprintf(w, `{"metadata-location": %q, "metadata": %s,
"config": {}, "labels": {"object-labels": {"owner": "analytics"}, "fields":
[{"field-id": 1, "labels": {"classification": "internal"}}]}}`,
+ "metadata-location", exampleViewMetadataJSON)
Review Comment:
nit: this formats the literal string `"metadata-location"` in as the
`metadata-location` value, which looks like a copy-paste slip. It doesn't
affect the assertions, but please use a realistic path such as the
`metadataLoc` constant in `TestRegisterViewLabels`.
--
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]
