laskoviymishka commented on code in PR #1507:
URL: https://github.com/apache/iceberg-go/pull/1507#discussion_r3639553823
##########
catalog/hive/hive.go:
##########
@@ -776,6 +777,16 @@ func (c *Catalog) ListNamespaces(ctx context.Context,
parent table.Identifier) (
}
// CreateNamespace creates a new namespace in the catalog.
+// defaultNamespaceLocation derives <warehouse>/<db>.db, the metastore's
Review Comment:
There's no blank line between the two comment groups, so godoc attaches the
whole block to `defaultNamespaceLocation` — which means `CreateNamespace` below
loses its doc and the unexported helper ends up documented as "creates a new
namespace in the catalog."
I'd move the `// CreateNamespace ...` line back down to sit directly above
`func CreateNamespace`, and give the helper its own standalone comment.
##########
catalog/hive/hive_test.go:
##########
@@ -399,6 +399,28 @@ func TestHiveCreateNamespace(t *testing.T) {
mockClient.AssertExpectations(t)
}
+func TestDefaultNamespaceLocation(t *testing.T) {
Review Comment:
Could we make this table-driven with `t.Run`? Right now it's three bare
`require.Equal`s, so a failure only points at the function, not the case — and
it's the one test in the file not using the `require.New(t)` style.
While we're restructuring, a `file:///` case and a double-trailing-slash
input would be worth pinning down given the `TrimRight` thread above, so the
behavior is locked in whichever way we land it.
##########
catalog/hive/hive.go:
##########
@@ -776,6 +777,16 @@ func (c *Catalog) ListNamespaces(ctx context.Context,
parent table.Identifier) (
}
// CreateNamespace creates a new namespace in the catalog.
+// defaultNamespaceLocation derives <warehouse>/<db>.db, the metastore's
+// conventional database location, or "" when no warehouse is configured.
+func defaultNamespaceLocation(warehouse, database string) string {
+ if warehouse == "" {
+ return ""
+ }
+
+ return strings.TrimRight(warehouse, "/") + "/" + database + ".db"
Review Comment:
`TrimRight(warehouse, "/")` treats the arg as a cutset, so it strips every
trailing slash rather than one. Mostly harmless, but two edges leak through:
`warehouse == "/"` trims to `""` and we hand the metastore `/db.db`, and
`file:///` trims to `file:` so we'd derive `file:/db.db` where Java's
stripTrailingSlash stops at the scheme and keeps `file:///db.db`.
`strings.TrimSuffix(warehouse, "/")` sidesteps both and matches what
hive.go:340 already does for the table location. I know hadoop.go uses
`TrimRight` too, so this isn't a new convention break — just a sharper edge
than that path hits. wdyt?
##########
catalog/hive/hive_test.go:
##########
@@ -399,6 +399,28 @@ func TestHiveCreateNamespace(t *testing.T) {
mockClient.AssertExpectations(t)
}
+func TestDefaultNamespaceLocation(t *testing.T) {
+ require.Equal(t, "s3://warehouse/db.db",
defaultNamespaceLocation("s3://warehouse", "db"))
+ require.Equal(t, "s3://warehouse/db.db",
defaultNamespaceLocation("s3://warehouse/", "db"))
+ require.Empty(t, defaultNamespaceLocation("", "db"))
+}
+
+func TestHiveCreateNamespaceDerivesLocationFromWarehouse(t *testing.T) {
Review Comment:
This covers the happy path nicely. Two guard cases aren't exercised though:
no warehouse + no location prop (should preserve empty `LocationUri` — the
no-regression case), and warehouse set + explicit location prop (explicit
should still win). Both are easy to invert accidentally, so I'd add them
alongside this one.
##########
catalog/hive/hive.go:
##########
@@ -798,6 +809,12 @@ func (c *Catalog) CreateNamespace(ctx context.Context,
namespace table.Identifie
}
}
+ // Derive a location from the warehouse when none is given, so the
metastore
+ // is not handed an empty path (which it rejects).
+ if db.LocationUri == "" {
Review Comment:
Small one: if a caller passes `location: ""` explicitly, the props loop sets
it empty and this guard then overwrites it with the derived path, so "explicit
empty" and "absent" become indistinguishable. Probably fine for the metastore's
sake, but if we want to preserve intent we'd have to track whether the key was
present. Non-blocking.
--
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]
