laskoviymishka commented on code in PR #1389:
URL: https://github.com/apache/iceberg-go/pull/1389#discussion_r3535328668
##########
catalog/hadoop/hadoop_test.go:
##########
@@ -1900,7 +1901,8 @@ func (s *HadoopCatalogTestSuite)
TestCheckTableExistsInvalidIdentifierFalse() {
for _, tt := range tests {
s.Run(tt.name, func() {
exists, err :=
s.cat.CheckTableExists(context.Background(), tt.ident)
- s.Require().NoError(err)
+ s.Require().Error(err)
+ s.ErrorIs(err, catalog.ErrNoSuchTable)
s.False(exists)
Review Comment:
This asserts the exact sentinel I flagged in `CheckTableExists`, so it
encodes the defect into the suite — if we switch to `ErrInvalidIdentifier`,
both this and the `ShortIdentifier` case have to be rewritten. I'd hold the
test change until the sentinel is settled, then assert on the new one. It'd
also be worth one case proving an invalid identifier is distinguishable from a
genuine not-found, which isn't possible while both wrap the same error.
##########
catalog/hadoop/hadoop_test.go:
##########
@@ -1876,7 +1876,8 @@ func (s *HadoopCatalogTestSuite)
TestCheckTableExistsPropagatesMetadataReadError
func (s *HadoopCatalogTestSuite) TestCheckTableExistsShortIdentifier() {
exists, err := s.cat.CheckTableExists(context.Background(),
[]string{"tbl"})
- s.Require().NoError(err)
+ s.Require().Error(err)
+ s.ErrorIs(err, catalog.ErrNoSuchTable)
Review Comment:
Minor: `s.ErrorIs` at assert level won't halt the test, so a
non-nil-but-wrong-type error gives you two failure lines.
`s.Require().ErrorIs(err, ...)` folds the `Require().Error` and the type check
into one and stops cleanly on mismatch.
##########
catalog/hadoop/hadoop.go:
##########
@@ -584,7 +584,7 @@ func (c *Catalog) LoadTable(ctx context.Context, ident
table.Identifier) (*table
func (c *Catalog) CheckTableExists(_ context.Context, ident table.Identifier)
(bool, error) {
if err := validateTableIdentifier(ident); err != nil {
- return false, nil
+ return false, err
Review Comment:
Returning the error instead of a silent `false` is the right move, but
`validateTableIdentifier` wraps `catalog.ErrNoSuchTable`, so `errors.Is(err,
ErrNoSuchTable)` now returns true for a malformed identifier like `["tbl"]`.
That sentinel means "catalog queried, table absent" — a caller doing `if
errors.Is(err, ErrNoSuchTable) { createTable(...) }` would treat garbage input
as "not found, go ahead and create".
I'd introduce a distinct `catalog.ErrInvalidIdentifier` and wrap that from
the validation helpers instead — which also lines Hadoop up with
Glue/Hive/REST, none of which wrap `ErrNoSuchTable` for invalid identifiers.
Worth checking `CheckNamespaceExists`/`validateIdentifierParts` too, since they
have the symmetric issue with `ErrNoSuchNamespace`. wdyt?
--
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]
