laskoviymishka commented on code in PR #1292:
URL: https://github.com/apache/iceberg-go/pull/1292#discussion_r3481858731
##########
catalog/hadoop/hadoop.go:
##########
@@ -55,13 +55,68 @@ var _ catalog.Catalog = (*Catalog)(nil)
var versionPattern = regexp.MustCompile(`^v([0-9]+)(?:\.gz)?\.metadata\.json$`)
// uuidMetadataPattern matches UUID-style metadata filenames produced by
-// Java/PyIceberg catalogs: 00000-<uuid>.metadata.json or
-// 00000-<uuid>.gz.metadata.json. The sequence is a 5-digit zero-padded
+// Java/PyIceberg catalogs: 00001-<uuid>.metadata.json or
+// 00001-<uuid>.gz.metadata.json. The sequence is a 5-digit zero-padded
// number and the UUID is in canonical 8-4-4-4-12 hex format.
var uuidMetadataPattern = regexp.MustCompile(
-
`^[0-9]{5}-[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}(?:\.gz)?\.metadata\.json$`,
+
`^([0-9]{5})-[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}(?:\.gz)?\.metadata\.json$`,
)
+type metadataFile struct {
+ location string
+ version int
+ hadoopName bool
+ compressed bool
+}
+
+func metadataFileFromName(path, name string) (metadataFile, bool) {
+ if matches := versionPattern.FindStringSubmatch(name); len(matches) ==
2 {
+ version, _ := strconv.Atoi(matches[1])
+ if version <= 0 {
+ return metadataFile{}, false
+ }
+
+ return metadataFile{
+ location: path,
+ version: version,
+ hadoopName: true,
+ compressed: strings.Contains(name, ".gz.metadata.json"),
+ }, true
+ }
+
+ if matches := uuidMetadataPattern.FindStringSubmatch(name);
len(matches) == 2 {
+ version, _ := strconv.Atoi(matches[1])
+ if version <= 0 {
Review Comment:
Really nice that this PR teaches the catalog to read UUID-named metadata —
that's exactly the interop gap worth closing. One thing I think we need to
handle before merge: this guard rejects `00000-<uuid>.metadata.json`, which is
the first metadata file other engines write for a brand-new table.
Both reference implementations start the sequence at zero: Java's
`BaseMetastoreTableOperations` starts `version` at -1, so the first commit is
`(-1)+1 = 0` -> `String.format("%05d-%s%s", 0, uuid, ext)` =
`00000-<uuid>.metadata.json`; PyIceberg's
`new_table_metadata_file_location(new_version=0)` produces the same; and Java's
own `HadoopCatalog.isTableDir` filters on `endsWith(".metadata.json")`, so it's
happy with `00000-*`. With `version <= 0` here, a table whose only metadata
file is `00000-<uuid>.metadata.json` ends up invisible to this catalog —
`LoadTable` returns `ErrNoSuchTable`, `ListTables` skips it, `CheckTableExists`
returns false — which is the cross-engine round-trip this PR is trying to
enable.
I think the two naming schemes just want different guards: keep `<= 0` on
the Hadoop `vN` branch (Java starts `vN` at 1, so `v0` really is invalid
there), and use `< 0` on this UUID branch. A small fixture with
`00000-<uuid>.metadata.json` asserting `isTableDir`/`LoadTable` find it would
lock it in.
(Tiny related thing: the doc-comment edit just above, from `00000` to
`00001`, points the wrong way for the same reason — `00000` was correct — so
I'd revert that hunk alongside this.)
##########
catalog/hadoop/hadoop.go:
##########
@@ -318,28 +341,70 @@ func (c *Catalog) findVersion(ident table.Identifier)
(int, error) {
return fs.SkipDir
}
- name := d.Name()
- matches := versionPattern.FindStringSubmatch(name)
- if len(matches) == 2 {
- v, _ := strconv.Atoi(matches[1])
- if v > maxVer {
- maxVer = v
- }
+ file, ok := metadataFileFromName(path, d.Name())
+ if !ok {
+ return nil
+ }
+
+ if file.betterThan(byVersion[file.version]) {
+ byVersion[file.version] = file
+ }
+ if file.betterThan(latest) {
+ latest = file
}
return nil
})
+
+ return byVersion, latest, err
+}
+
+func scanForwardMetadata(files map[int]metadataFile, start metadataFile)
metadataFile {
+ current := start
+ for {
+ next, ok := files[current.version+1]
+ if !ok {
+ return current
+ }
+
+ current = next
+ }
+}
+
+func (c *Catalog) findMetadataLocation(ident table.Identifier) (string, int,
error) {
+ files, latest, err := c.scanMetadataFiles(ident)
if err != nil {
- return 0, fmt.Errorf("hadoop catalog: cannot read metadata
directory for %s: %w",
- strings.Join(ident, "."), catalog.ErrNoSuchTable)
+ if errors.Is(err, fs.ErrNotExist) {
+ return "", 0, fmt.Errorf("%w: cannot read metadata
directory for %s: %w",
+ catalog.ErrNoSuchTable, strings.Join(ident,
"."), err)
+ }
+
+ return "", 0, fmt.Errorf("hadoop catalog: cannot read metadata
directory for %s: %w",
+ strings.Join(ident, "."), err)
}
- if maxVer == 0 {
- return 0, fmt.Errorf("hadoop catalog: no metadata files found
for table %s: %w",
+ if latest.location == "" {
+ return "", 0, fmt.Errorf("hadoop catalog: no metadata files
found for table %s: %w",
strings.Join(ident, "."), catalog.ErrNoSuchTable)
}
- return c.scanForward(ident, maxVer), nil
+ hint := c.readVersionHint(ident)
+ if hint > 0 {
+ if hinted, ok := files[hint]; ok {
+ latest = scanForwardMetadata(files, hinted)
Review Comment:
I think there's a subtle case here that can quietly re-open the very bug
this PR is closing — flagging it because three of us landed on it independently.
`latest` already holds the global best across every discovered file, and
then when the hint resolves we overwrite it with `scanForwardMetadata`, which
walks contiguous versions and stops at the first gap. So with files `{v1, v2,
v5}` and `hint=1`, the walk goes 1 -> 2 -> (no v3) -> stops at v2, and we load
v2 while v5 is sitting right there.
The old code only trusted the hint when that exact version existed and
otherwise fell back to the max, so it didn't trip on gaps. A one-line guard
keeps the fast path while staying safe:
```go
if hint > 0 {
if hinted, ok := files[hint]; ok {
if walked := scanForwardMetadata(files, hinted);
walked.betterThan(latest) {
latest = walked
}
}
}
```
A `{v1,v2,v5}+hint=1` test would be a nice regression guard here — it's
currently uncovered.
##########
catalog/hadoop/hadoop.go:
##########
@@ -580,7 +657,11 @@ func (c *Catalog) ListTables(_ context.Context, ns
table.Identifier) iter.Seq2[t
}
// Skip anything that is not a table directory.
- if !isTableDir(c.filesystem, path) {
+ isTable, err := isTableDir(c.filesystem, path)
Review Comment:
Returning the `isTableDir` error instead of swallowing it is a good
instinct. The one thing I'd weigh: returning it straight out of the `WalkDir`
callback means a single unreadable metadata subdir now aborts the whole
`ListTables` (and `ListNamespaces` does the same), where the old code skipped
the bad entry and kept going.
Java's `HadoopCatalog.isTableDir` has a `shouldSuppressPermissionError`
guard for exactly this — it log-warns and treats the dir as not-a-table so
listing survives mixed-permission warehouses. I'd lean toward matching that
(suppress permission errors, skip the dir), but I'm happy either way as long as
it's a conscious choice — which behavior do you want here?
##########
catalog/hadoop/hadoop.go:
##########
@@ -55,13 +55,68 @@ var _ catalog.Catalog = (*Catalog)(nil)
var versionPattern = regexp.MustCompile(`^v([0-9]+)(?:\.gz)?\.metadata\.json$`)
// uuidMetadataPattern matches UUID-style metadata filenames produced by
-// Java/PyIceberg catalogs: 00000-<uuid>.metadata.json or
-// 00000-<uuid>.gz.metadata.json. The sequence is a 5-digit zero-padded
+// Java/PyIceberg catalogs: 00001-<uuid>.metadata.json or
+// 00001-<uuid>.gz.metadata.json. The sequence is a 5-digit zero-padded
// number and the UUID is in canonical 8-4-4-4-12 hex format.
var uuidMetadataPattern = regexp.MustCompile(
-
`^[0-9]{5}-[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}(?:\.gz)?\.metadata\.json$`,
+
`^([0-9]{5})-[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}(?:\.gz)?\.metadata\.json$`,
)
+type metadataFile struct {
+ location string
+ version int
+ hadoopName bool
+ compressed bool
+}
+
+func metadataFileFromName(path, name string) (metadataFile, bool) {
+ if matches := versionPattern.FindStringSubmatch(name); len(matches) ==
2 {
+ version, _ := strconv.Atoi(matches[1])
+ if version <= 0 {
+ return metadataFile{}, false
+ }
+
+ return metadataFile{
+ location: path,
+ version: version,
+ hadoopName: true,
+ compressed: strings.Contains(name, ".gz.metadata.json"),
+ }, true
+ }
+
+ if matches := uuidMetadataPattern.FindStringSubmatch(name);
len(matches) == 2 {
+ version, _ := strconv.Atoi(matches[1])
+ if version <= 0 {
+ return metadataFile{}, false
+ }
+
+ return metadataFile{
+ location: path,
+ version: version,
+ compressed: strings.Contains(name, ".gz.metadata.json"),
+ }, true
+ }
+
+ return metadataFile{}, false
+}
+
+func (m metadataFile) betterThan(current metadataFile) bool {
+ switch {
+ case current.location == "":
+ return true
+ case m.version != current.version:
+ return m.version > current.version
+ case m.hadoopName != current.hadoopName:
+ // If both naming styles exist for one version, prefer the
Hadoop vN
+ // file because this catalog writes and conflict-checks that
sequence.
+ return m.hadoopName
Review Comment:
This tie-break reads fine for the common case; I just want to flag the
assumption baked in. Preferring `vN` over `00001-<uuid>` for the same version
treats the UUID `<seq>` and the Hadoop version as the same namespace — but Java
derives its version purely from `v<N>.metadata.json` + version-hint and never
parses UUID names; those come from object-store/REST writers where the sequence
is just a monotonic counter. So for a single writer this is great, but for a
genuinely mixed-writer table the two files at "version 1" aren't guaranteed to
be the same snapshot. I don't think this needs to change here — a comment
noting it's a best-effort heuristic rather than spec would help the next
reader. wdyt?
##########
catalog/hadoop/hadoop.go:
##########
@@ -318,28 +341,70 @@ func (c *Catalog) findVersion(ident table.Identifier)
(int, error) {
return fs.SkipDir
}
- name := d.Name()
- matches := versionPattern.FindStringSubmatch(name)
- if len(matches) == 2 {
- v, _ := strconv.Atoi(matches[1])
- if v > maxVer {
- maxVer = v
- }
+ file, ok := metadataFileFromName(path, d.Name())
+ if !ok {
+ return nil
+ }
+
+ if file.betterThan(byVersion[file.version]) {
+ byVersion[file.version] = file
+ }
+ if file.betterThan(latest) {
+ latest = file
}
return nil
})
+
+ return byVersion, latest, err
+}
+
+func scanForwardMetadata(files map[int]metadataFile, start metadataFile)
metadataFile {
Review Comment:
Nice clean helper. One behavior worth a comment: because it walks `files`
purely by version, it'll happily chain from a UUID file at vN to a Hadoop
`vN+1` (and back) — reasonable for a table that switched writers, but novel
relative to Java (which only walks the `vN` sequence) and currently untested. A
one-line note that the walk is naming-agnostic, plus a small mixed-style test,
would pin the intent.
##########
catalog/hadoop/hadoop.go:
##########
@@ -197,11 +252,10 @@ func (c *Catalog) defaultTableLocation(ident
table.Identifier) string {
}
// isTableDir reports whether path is a table directory by checking for
-// metadata files in its metadata/ subdirectory. It recognizes:
+// loadable metadata files in its metadata/ subdirectory. It recognizes:
// - v*.metadata.json (Hadoop catalog format)
// - <seq>-<uuid>.metadata.json (Java/PyIceberg format)
-// - version-hint.text
-func isTableDir(filesystem HadoopCatalogFS, path string) bool {
+func isTableDir(filesystem HadoopCatalogFS, path string) (bool, error) {
Review Comment:
Good change — folding detection into `metadataFileFromName` is cleaner.
Worth calling out that a directory with only `version-hint.text` and no
metadata file is no longer treated as a table. I checked, and this matches Java
(its `isTableDir` never counted `version-hint.text`), so it's correct parity
rather than a regression — but since it's a user-visible change to
`CheckTableExists`/`ListTables`, a line in the PR description noting it's
intentional would help.
##########
catalog/hadoop/hadoop.go:
##########
@@ -55,13 +55,68 @@ var _ catalog.Catalog = (*Catalog)(nil)
var versionPattern = regexp.MustCompile(`^v([0-9]+)(?:\.gz)?\.metadata\.json$`)
// uuidMetadataPattern matches UUID-style metadata filenames produced by
-// Java/PyIceberg catalogs: 00000-<uuid>.metadata.json or
-// 00000-<uuid>.gz.metadata.json. The sequence is a 5-digit zero-padded
+// Java/PyIceberg catalogs: 00001-<uuid>.metadata.json or
+// 00001-<uuid>.gz.metadata.json. The sequence is a 5-digit zero-padded
// number and the UUID is in canonical 8-4-4-4-12 hex format.
var uuidMetadataPattern = regexp.MustCompile(
-
`^[0-9]{5}-[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}(?:\.gz)?\.metadata\.json$`,
+
`^([0-9]{5})-[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}(?:\.gz)?\.metadata\.json$`,
)
+type metadataFile struct {
+ location string
+ version int
+ hadoopName bool
+ compressed bool
+}
+
+func metadataFileFromName(path, name string) (metadataFile, bool) {
+ if matches := versionPattern.FindStringSubmatch(name); len(matches) ==
2 {
+ version, _ := strconv.Atoi(matches[1])
+ if version <= 0 {
+ return metadataFile{}, false
+ }
+
+ return metadataFile{
+ location: path,
+ version: version,
+ hadoopName: true,
+ compressed: strings.Contains(name, ".gz.metadata.json"),
+ }, true
+ }
+
+ if matches := uuidMetadataPattern.FindStringSubmatch(name);
len(matches) == 2 {
+ version, _ := strconv.Atoi(matches[1])
+ if version <= 0 {
+ return metadataFile{}, false
+ }
+
+ return metadataFile{
+ location: path,
+ version: version,
+ compressed: strings.Contains(name, ".gz.metadata.json"),
+ }, true
+ }
+
+ return metadataFile{}, false
+}
+
+func (m metadataFile) betterThan(current metadataFile) bool {
+ switch {
+ case current.location == "":
+ return true
+ case m.version != current.version:
+ return m.version > current.version
+ case m.hadoopName != current.hadoopName:
+ // If both naming styles exist for one version, prefer the
Hadoop vN
+ // file because this catalog writes and conflict-checks that
sequence.
+ return m.hadoopName
+ case m.compressed != current.compressed:
+ return !m.compressed
+ default:
+ return m.location > current.location
Review Comment:
Just a readability nit: the lexicographic fallback is perfectly fine for
determinism, but "higher path wins" isn't a meaningful Iceberg ordering, so a
future reader may pause on it. A one-line comment that it's purely a stable
tie-breaker under map-iteration order (and that within a single scan all
locations share a parent dir, so it reduces to filename order) would explain
itself.
##########
catalog/hadoop/hadoop.go:
##########
@@ -318,28 +341,70 @@ func (c *Catalog) findVersion(ident table.Identifier)
(int, error) {
return fs.SkipDir
}
- name := d.Name()
- matches := versionPattern.FindStringSubmatch(name)
- if len(matches) == 2 {
- v, _ := strconv.Atoi(matches[1])
- if v > maxVer {
- maxVer = v
- }
+ file, ok := metadataFileFromName(path, d.Name())
+ if !ok {
+ return nil
+ }
+
+ if file.betterThan(byVersion[file.version]) {
+ byVersion[file.version] = file
+ }
+ if file.betterThan(latest) {
+ latest = file
}
return nil
})
+
+ return byVersion, latest, err
+}
+
+func scanForwardMetadata(files map[int]metadataFile, start metadataFile)
metadataFile {
+ current := start
+ for {
+ next, ok := files[current.version+1]
+ if !ok {
+ return current
+ }
+
+ current = next
+ }
+}
+
+func (c *Catalog) findMetadataLocation(ident table.Identifier) (string, int,
error) {
+ files, latest, err := c.scanMetadataFiles(ident)
if err != nil {
- return 0, fmt.Errorf("hadoop catalog: cannot read metadata
directory for %s: %w",
- strings.Join(ident, "."), catalog.ErrNoSuchTable)
+ if errors.Is(err, fs.ErrNotExist) {
+ return "", 0, fmt.Errorf("%w: cannot read metadata
directory for %s: %w",
Review Comment:
Two small things on this branch. The message leads with the sentinel
(`"table does not exist: cannot read metadata directory for ns.tbl: file does
not exist"`), while everything else in this file — including the sibling branch
right below — leads with the `hadoop catalog:` prefix. Keeping the prefix and
moving the sentinel into the wrap chain reads more consistently:
```go
return "", 0, fmt.Errorf("hadoop catalog: cannot read metadata directory for
%s: %w: %w",
strings.Join(ident, "."), catalog.ErrNoSuchTable, err)
```
The slightly bigger one: this maps `fs.ErrNotExist` on the metadata dir to
`ErrNoSuchTable`, while `isTableDir` maps the same condition to `(false, nil)`.
Same physical state, two interpretations across the two callpaths — might be
worth picking one as canonical so they agree.
##########
catalog/hadoop/hadoop.go:
##########
@@ -55,13 +55,68 @@ var _ catalog.Catalog = (*Catalog)(nil)
var versionPattern = regexp.MustCompile(`^v([0-9]+)(?:\.gz)?\.metadata\.json$`)
// uuidMetadataPattern matches UUID-style metadata filenames produced by
-// Java/PyIceberg catalogs: 00000-<uuid>.metadata.json or
-// 00000-<uuid>.gz.metadata.json. The sequence is a 5-digit zero-padded
+// Java/PyIceberg catalogs: 00001-<uuid>.metadata.json or
+// 00001-<uuid>.gz.metadata.json. The sequence is a 5-digit zero-padded
// number and the UUID is in canonical 8-4-4-4-12 hex format.
var uuidMetadataPattern = regexp.MustCompile(
-
`^[0-9]{5}-[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}(?:\.gz)?\.metadata\.json$`,
+
`^([0-9]{5})-[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}(?:\.gz)?\.metadata\.json$`,
)
+type metadataFile struct {
+ location string
+ version int
+ hadoopName bool
+ compressed bool
+}
+
+func metadataFileFromName(path, name string) (metadataFile, bool) {
+ if matches := versionPattern.FindStringSubmatch(name); len(matches) ==
2 {
+ version, _ := strconv.Atoi(matches[1])
Review Comment:
Small one: the version capture is unbounded (`[0-9]+`), so something like
`v99999999999999999999.metadata.json` overflows `Atoi` to `(0, ErrRange)`. We
drop the error and then `version <= 0` rejects it — so the file does get
skipped, but for the wrong reason, and a legitimately huge version would vanish
the same way. I'd check the error explicitly here (and on the UUID branch, with
`< 0` per the note above):
```go
version, err := strconv.Atoi(matches[1])
if err != nil || version <= 0 {
return metadataFile{}, false
}
```
##########
catalog/hadoop/hadoop_test.go:
##########
@@ -1240,6 +1411,67 @@ func (s *HadoopCatalogTestSuite)
TestLoadTableStaleHint() {
s.NotNil(tbl)
}
+func (s *HadoopCatalogTestSuite) TestLoadTableGzipMetadata() {
+ ctx := context.Background()
+ ident := []string{"ns", "tbl"}
+ s.Require().NoError(os.Mkdir(filepath.Join(s.warehouse, "ns"), 0o755))
+
+ created, err := s.cat.CreateTable(ctx, ident, s.testSchema())
+ s.Require().NoError(err)
+ gzPath := s.replaceMetadataWithGzip(ident, 1)
+
+ loaded, err := s.cat.LoadTable(ctx, ident)
+ s.Require().NoError(err)
+ s.Equal(created.Metadata().TableUUID(), loaded.Metadata().TableUUID())
+ s.Equal(gzPath, loaded.MetadataLocation())
+
+ exists, err := s.cat.CheckTableExists(ctx, ident)
+ s.Require().NoError(err)
+ s.True(exists)
+}
+
+func (s *HadoopCatalogTestSuite) TestLoadTableUUIDMetadata() {
+ ctx := context.Background()
+ ident := []string{"ns", "tbl"}
+ s.Require().NoError(os.Mkdir(filepath.Join(s.warehouse, "ns"), 0o755))
+
+ created, err := s.cat.CreateTable(ctx, ident, s.testSchema())
+ s.Require().NoError(err)
+ uuidPath := s.replaceMetadataWithUUIDName(ident, 1)
+
+ loaded, err := s.cat.LoadTable(ctx, ident)
+ s.Require().NoError(err)
+ s.Equal(created.Metadata().TableUUID(), loaded.Metadata().TableUUID())
+ s.Equal(uuidPath, loaded.MetadataLocation())
+
+ exists, err := s.cat.CheckTableExists(ctx, ident)
+ s.Require().NoError(err)
+ s.True(exists)
+}
+
+func (s *HadoopCatalogTestSuite)
TestVersionHintWithoutMetadataIsNotLoadableTable() {
+ ctx := context.Background()
+ ident := []string{"ns", "tbl"}
+ metaDir := s.cat.metadataDir(ident)
+ s.Require().NoError(os.MkdirAll(metaDir, 0o755))
+ s.cat.writeVersionHint(ident, 1)
+
+ exists, err := s.cat.CheckTableExists(ctx, ident)
+ s.Require().NoError(err)
+ s.False(exists)
+
+ var listed []table.Identifier
+ for ident, err := range s.cat.ListTables(ctx, []string{"ns"}) {
Review Comment:
Tiny nit: the range variable `ident` shadows the outer `ident` for this
test. No bug under Go 1.22+ scoping, but vet/staticcheck flags it and it reads
a little confusingly — renaming the range var to `tblIdent` would clear it up.
##########
catalog/hadoop/hadoop_test.go:
##########
@@ -54,6 +58,125 @@ func TestHadoopCatalogTestSuite(t *testing.T) {
suite.Run(t, new(HadoopCatalogTestSuite))
}
+func (s *HadoopCatalogTestSuite) requireIsTableDir(path string) bool {
+ isTable, err := isTableDir(s.cat.filesystem, path)
+ s.Require().NoError(err)
+
+ return isTable
+}
+
+func TestMetadataFileBetterThan(t *testing.T) {
Review Comment:
Solid table test. Two small things: it uses bare `t.Fatalf` while the rest
of the suite is on testify — `require.Equal(t, tt.want, got)` would match the
house style and give a nicer diff on failure. And since `scanMetadataFiles`
relies on `betterThan` being a strict weak ordering under nondeterministic map
iteration, a couple of property rows (`x.betterThan(x) == false`, plus the case
where both compression and naming differ at the same version) would lock that
contract down.
##########
catalog/hadoop/hadoop_test.go:
##########
@@ -1222,6 +1382,17 @@ func (s *HadoopCatalogTestSuite)
TestLoadTableNotExists() {
s.ErrorIs(err, catalog.ErrNoSuchTable)
}
+func (s *HadoopCatalogTestSuite) TestLoadTablePropagatesMetadataReadError() {
+ ctx := context.Background()
+ walkErr := errors.New("metadata walk failed")
+ s.cat.filesystem = failingWalkFS{HadoopCatalogFS: s.cat.filesystem,
err: walkErr}
Review Comment:
This and `TestCheckTableExistsPropagatesMetadataReadError` swap
`s.cat.filesystem` in place and don't restore it. If `SetupTest` rebuilds
`s.cat` fresh per test, we're totally fine — I just couldn't see it in the
diff. If it reuses the instance, a later test could inherit `failingWalkFS` and
flake depending on order; a deferred restore would make it bulletproof. Could
you confirm `SetupTest` reconstructs `s.cat`?
--
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]
