laskoviymishka commented on code in PR #1292:
URL: https://github.com/apache/iceberg-go/pull/1292#discussion_r3466848330
##########
catalog/hadoop/hadoop.go:
##########
@@ -439,7 +488,12 @@ func (c *Catalog) CheckTableExists(_ context.Context,
ident table.Identifier) (b
return false, nil
}
- return isTableDir(c.filesystem, c.tableToPath(ident)), nil
+ _, _, err := c.findMetadataLocation(ident)
Review Comment:
This swaps the old `isTableDir` short-circuit (first match → `fs.SkipAll`)
for a full `scanMetadataFiles` walk on every existence check. Java's
`HadoopCatalog` uses a filtered `listStatus` that bails on the first hit; a
table with a few hundred versions now pays an O(metadata-file-count) walk per
`CheckTableExists`, and this is called before `CreateTable` and to guard
commits.
`isTableDir` now correctly excludes hint-only dirs (same
`metadataFileFromName`), so it's still a valid fast path. I'd keep
`CheckTableExists` on `isTableDir` and reserve the full scan for callers that
actually need the location. Thoughts?
##########
catalog/hadoop/hadoop.go:
##########
@@ -318,28 +332,65 @@ 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",
+ return "", 0, fmt.Errorf("hadoop catalog: cannot read metadata
directory for %s: %w",
Review Comment:
I think there's a real bug here: any error out of `scanMetadataFiles`
(permission denied, transient FS failure) gets wrapped as
`catalog.ErrNoSuchTable`, and the original error is dropped — it's not threaded
through `%w`.
Downstream, `CheckTableExists` does `errors.Is(err, catalog.ErrNoSuchTable)`
and returns `(false, nil)`, so a metadata dir we simply couldn't read looks
exactly like a table that doesn't exist. That's the kind of thing that leads to
a `CreateTable` stomping live data.
I'd only map to `ErrNoSuchTable` when `errors.Is(err, fs.ErrNotExist)`, and
otherwise return the original error wrapped. wdyt?
##########
catalog/hadoop/hadoop.go:
##########
@@ -439,7 +488,12 @@ func (c *Catalog) CheckTableExists(_ context.Context,
ident table.Identifier) (b
return false, nil
}
- return isTableDir(c.filesystem, c.tableToPath(ident)), nil
+ _, _, err := c.findMetadataLocation(ident)
+ if errors.Is(err, catalog.ErrNoSuchTable) {
+ return false, nil
+ }
+
+ return err == nil, err
Review Comment:
`return err == nil, err` is doing two jobs in one line and obscures the
contract. Combined with the error-swallowing in `findMetadataLocation` above, a
non-`ErrNoSuchTable` failure flows through here as `(false, <err>)` — which is
the right shape only once that function stops masking real errors.
I'd make it explicit: `if err != nil { return false, err }` then `return
true, nil`. Easier to see that "not found" and "couldn't tell" are different
outcomes.
##########
catalog/hadoop/hadoop.go:
##########
@@ -59,9 +59,59 @@ var versionPattern =
regexp.MustCompile(`^v([0-9]+)(?:\.gz)?\.metadata\.json$`)
// 00000-<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])
+
+ return metadataFile{
+ location: path,
+ version: version,
+ compressed: strings.Contains(name, ".gz.metadata.json"),
+ }, true
+ }
+
+ return metadataFile{}, false
+}
+
+func (m metadataFile) betterThan(current metadataFile) bool {
Review Comment:
This tie-breaker is the most novel piece of the PR and it's the resolution
policy the whole discovery rewrite leans on, but nothing tests it. I'd add a
focused unit test on `betterThan`: empty current loses; higher version wins;
same version → hadoop name beats uuid; same version+style → uncompressed beats
gz; and the final lexicographic fallback.
The hadoop-over-uuid preference at equal version is also an undocumented
policy with no Java precedent (Java never mixes the two naming schemes). A
one-line comment on *why* we prefer the hadoop name would save the next reader
a real head-scratch. Once there's a test pinning these, I'm comfortable with
the logic.
##########
catalog/hadoop/hadoop.go:
##########
@@ -495,7 +549,7 @@ func (c *Catalog) CommitTable(ctx context.Context, ident
table.Identifier, reqs
var currentVersion int
if current != nil {
- currentVersion, err = c.findVersion(ident)
+ _, currentVersion, err = c.findMetadataLocation(ident)
Review Comment:
`CommitTable` already called `LoadTable` (→ `findMetadataLocation` → full
walk) just above, and now walks the same directory a second time here just to
recover the version. Two independent full scans of the same metadata dir per
commit.
Since `findMetadataLocation` already returns the version alongside the path,
I'd have `LoadTable` hand both back (or read the version off the already-loaded
metadata) and reuse it here. wdyt?
##########
catalog/hadoop/hadoop_test.go:
##########
@@ -230,6 +232,35 @@ func (s *HadoopCatalogTestSuite) createMetadataFile(ident
table.Identifier, vers
s.Require().NoError(os.WriteFile(path, nil, 0o644))
}
+func (s *HadoopCatalogTestSuite) replaceMetadataWithGzip(ident
table.Identifier, version int) string {
+ plainPath := s.cat.metadataFilePath(ident, version)
+ data, err := os.ReadFile(plainPath)
+ s.Require().NoError(err)
+
+ var buf bytes.Buffer
+ zw := gzip.NewWriter(&buf)
+ _, err = zw.Write(data)
+ s.Require().NoError(err)
+ s.Require().NoError(zw.Close())
+
+ gzPath := filepath.Join(s.cat.metadataDir(ident),
fmt.Sprintf("v%d.gz.metadata.json", version))
+ s.Require().NoError(os.WriteFile(gzPath, buf.Bytes(), 0o644))
+ s.Require().NoError(os.Remove(plainPath))
+
+ return gzPath
+}
+
+func (s *HadoopCatalogTestSuite) replaceMetadataWithUUIDName(ident
table.Identifier, version int) string {
+ plainPath := s.cat.metadataFilePath(ident, version)
+ uuidPath := filepath.Join(
+ s.cat.metadataDir(ident),
+ "00000-a1b2c3d4-e5f6-7890-abcd-ef1234567890.metadata.json",
Review Comment:
`replaceMetadataWithUUIDName` hardcodes the `00000-` prefix, which parses to
version 0. That means the UUID file lands at `files[0]`, so the hint
forward-scan in `findMetadataLocation` (`files[hint]`, hint=1) never finds it —
the test passes via the `latest` fallback, not the path it looks like it's
exercising.
I'd switch the fixture to `00001-<uuid>` so it actually drives the hint
lookup, and it matches what real Java/PyIceberg clients write. Pairs with
adding the `version <= 0` guard on the UUID branch.
##########
catalog/hadoop/hadoop.go:
##########
@@ -59,9 +59,59 @@ var versionPattern =
regexp.MustCompile(`^v([0-9]+)(?:\.gz)?\.metadata\.json$`)
// 00000-<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])
Review Comment:
The hadoop branch rejects `version <= 0`, but this UUID branch doesn't, so a
`00000-<uuid>.metadata.json` parses to `version: 0` and is accepted. In
Java/PyIceberg the first commit writes `00001-` (`%05d` on `newVersion`, which
starts at 1), and version 0 is the "table doesn't exist" sentinel — so a real
client never produces `00000-`.
I'd mirror the guard in this branch:
```go
if version <= 0 {
return metadataFile{}, false
}
```
That also lets the UUID test fixture move onto `00001-`, which it should be
anyway.
##########
catalog/hadoop/hadoop.go:
##########
@@ -318,28 +332,65 @@ 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",
+ return "", 0, fmt.Errorf("hadoop catalog: cannot read metadata
directory for %s: %w",
strings.Join(ident, "."), catalog.ErrNoSuchTable)
}
- 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)
+ }
+ }
+
+ return latest.location, latest.version, nil
+}
+
+func (c *Catalog) findVersion(ident table.Identifier) (int, error) {
Review Comment:
`findVersion` is now just a thin wrapper that throws away the location, and
the only callers left are tests — `LoadTable` and `CommitTable` both go
straight to `findMetadataLocation`. So `TestFindVersionScanForward` and friends
are testing an intermediary that production no longer uses.
I'd point those tests at `findMetadataLocation` directly and drop
`findVersion`, so the discovery tests cover the real call path. Not blocking,
but it's dead production code today.
--
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]
