laskoviymishka commented on code in PR #1269:
URL: https://github.com/apache/iceberg-go/pull/1269#discussion_r3466954903
##########
catalog/glue/glue_test.go:
##########
@@ -255,6 +255,39 @@ func TestGlueGetTableCaseInsensitive(t *testing.T) {
}
}
+func TestGlueConvertGlueToIcebergCaseInsensitive(t *testing.T) {
+ assert := require.New(t)
Review Comment:
`assert` is bound to the outer `t` but used inside the `t.Run` subtests, so
a failure calls `FailNow()` on the parent — it kills the remaining subtests and
attributes the failure to the parent rather than the named case. I'd move
`assert := require.New(t)` inside the closure so it binds to the subtest's `t`.
This is copied from `TestGlueGetTableCaseInsensitive`, which has the same
issue, so it's a pre-existing pattern rather than something this PR introduced
— but since we're touching the success-path assertions anyway, worth fixing
here rather than carrying it forward.
##########
catalog/glue/glue_test.go:
##########
@@ -255,6 +255,39 @@ func TestGlueGetTableCaseInsensitive(t *testing.T) {
}
}
+func TestGlueConvertGlueToIcebergCaseInsensitive(t *testing.T) {
+ assert := require.New(t)
+
+ testCases := []struct {
+ name string
+ tableType string
+ wantErr string
+ }{
+ {"uppercase", "ICEBERG", "missing metadata location"},
Review Comment:
The three iceberg cases here don't actually pin the type check — they assert
on "missing metadata location", which is a downstream error from the
`metadata_location` lookup, not from the type guard. If someone reordered those
two checks, or broke the type check in a way that still fell through to the
metadata lookup, this test keeps passing. The whole point of the test is to
prove lowercase `table_type` is now accepted, and right now it doesn't prove
that.
I'd follow the `TestGlueGetTableCaseInsensitive` pattern instead: add
`tableParamMetadataLocation` to the Parameters map for the iceberg cases,
switch to a `shouldErr bool`, and on the success path assert `NoError` plus
something concrete on the returned `*table.Table`. At minimum, assert the error
does *not* contain "is not an iceberg table". While we're here it'd be cheap to
add `""` and absent-key cases too, both expecting "is not an iceberg table".
wdyt?
##########
catalog/glue/glue.go:
##########
@@ -700,7 +700,7 @@ func (c *Catalog) convertGlueToIceberg(ctx context.Context,
glueTable *types.Tab
return nil, fmt.Errorf("missing parameters for table %s",
tableName)
}
- if glueTable.Parameters[tableParamTableType] != glueTypeIceberg {
+ if !strings.EqualFold(glueTable.Parameters[tableParamTableType],
glueTypeIceberg) {
Review Comment:
This is now a second gate — every caller funnels through `getTable`, which
already does the same `EqualFold` check, so the only path that relies on this
one directly is `CommitTable`. That's fine as defensive depth, but the
duplication is exactly what let #537 regress: a new caller site got the old
comparison and nobody caught it. A one-line comment noting this must stay
consistent with the `getTable` check would help the next person. Not 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]
