zeroshade commented on code in PR #1292:
URL: https://github.com/apache/iceberg-go/pull/1292#discussion_r3493439820
##########
catalog/hadoop/hadoop.go:
##########
@@ -318,28 +392,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 || file.version != version {
+ return nil
}
- return nil
+ found = true
+
+ return fs.SkipAll
})
if err != nil {
- return 0, fmt.Errorf("hadoop catalog: cannot read metadata
directory for %s: %w",
- strings.Join(ident, "."), catalog.ErrNoSuchTable)
+ return false, err
}
- if maxVer == 0 {
- return 0, fmt.Errorf("hadoop catalog: no metadata files found
for table %s: %w",
+ return found, nil
+}
+
+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 {
+ if errors.Is(err, fs.ErrNotExist) {
+ return "", 0, fmt.Errorf("hadoop catalog: cannot read
metadata directory for %s: %w: %w",
+ strings.Join(ident, "."),
catalog.ErrNoSuchTable, err)
+ }
+
+ return "", 0, fmt.Errorf("hadoop catalog: cannot read metadata
directory for %s: %w",
+ strings.Join(ident, "."), err)
+ }
+
+ 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 {
+ if walked := scanForwardMetadata(files, hinted);
walked.betterThan(latest) {
Review Comment:
This hint-forward walk reads as dead code: `latest` is already the global
best via `betterThan`, so `walked.betterThan(latest)` can never be true.
Consider removing `scanForwardMetadata` + this check, or documenting why it
stays.
##########
catalog/hadoop/hadoop.go:
##########
@@ -270,43 +343,44 @@ func (c *Catalog) writeVersionHint(ident
table.Identifier, version int) {
}
}
-// metadataVersionExists checks whether a metadata file for the given version
-// exists in either plain or gzip-compressed form.
-func (c *Catalog) metadataVersionExists(ident table.Identifier, version int)
bool {
+func (c *Catalog) scanMetadataFiles(ident table.Identifier)
(map[int]metadataFile, metadataFile, error) {
dir := c.metadataDir(ident)
- plain := filepath.Join(dir, fmt.Sprintf("v%d.metadata.json", version))
-
- if _, err := c.filesystem.Stat(plain); err == nil {
- return true
- }
+ byVersion := map[int]metadataFile{}
+ var latest metadataFile
- gz := filepath.Join(dir, fmt.Sprintf("v%d.gz.metadata.json", version))
+ err := c.filesystem.WalkDir(dir, func(path string, d fs.DirEntry, err
error) error {
+ if err != nil {
+ return err
+ }
+ if path == dir {
+ return nil
+ }
+ if d.IsDir() {
+ return fs.SkipDir
+ }
- _, err := c.filesystem.Stat(gz)
+ file, ok := metadataFileFromName(path, d.Name())
+ if !ok {
+ return nil
+ }
- return err == nil
-}
+ if file.betterThan(byVersion[file.version]) {
+ byVersion[file.version] = file
+ }
+ if file.betterThan(latest) {
+ latest = file
+ }
-func (c *Catalog) scanForward(ident table.Identifier, start int) int {
- ver := start
- for c.metadataVersionExists(ident, ver+1) {
- ver++
- }
+ return nil
+ })
- return ver
+ return byVersion, latest, err
}
-func (c *Catalog) findVersion(ident table.Identifier) (int, error) {
- hint := c.readVersionHint(ident)
- if hint > 0 && c.metadataVersionExists(ident, hint) {
- return c.scanForward(ident, hint), nil
- }
-
+func (c *Catalog) metadataVersionExists(ident table.Identifier, version int)
(bool, error) {
Review Comment:
`metadataVersionExists` now does a full directory walk on every
`CommitTable` (hot path); with Hadoop naming `v10` sorts before `v2`, so it
usually scans most entries before short-circuiting. Consider targeted `Stat`s
for the plain/gz/UUID names, or document the tradeoff if the walk is
intentional (UUID-race coverage).
##########
catalog/hadoop/hadoop.go:
##########
@@ -419,27 +539,44 @@ func (c *Catalog) CreateTable(ctx context.Context, ident
table.Identifier, sc *i
return tbl, nil
}
-func (c *Catalog) LoadTable(ctx context.Context, ident table.Identifier)
(*table.Table, error) {
+func (c *Catalog) loadTable(ctx context.Context, ident table.Identifier)
(*table.Table, int, error) {
if err := validateTableIdentifier(ident); err != nil {
- return nil, err
+ return nil, 0, err
}
- ver, err := c.findVersion(ident)
+ metaPath, version, err := c.findMetadataLocation(ident)
if err != nil {
- return nil, err
+ return nil, 0, err
+ }
+
+ tbl, err := table.NewFromLocation(ctx, ident, metaPath,
icebergio.LoadFSFunc(c.props, metaPath), c)
+ if err != nil {
+ return nil, 0, err
}
- metaPath := c.metadataFilePath(ident, ver)
+ return tbl, version, nil
+}
+
+func (c *Catalog) LoadTable(ctx context.Context, ident table.Identifier)
(*table.Table, error) {
+ tbl, _, err := c.loadTable(ctx, ident)
- return table.NewFromLocation(ctx, ident, metaPath,
icebergio.LoadFSFunc(c.props, metaPath), c)
+ return tbl, err
}
func (c *Catalog) CheckTableExists(_ context.Context, ident table.Identifier)
(bool, error) {
if err := validateTableIdentifier(ident); err != nil {
return false, nil
}
- return isTableDir(c.filesystem, c.tableToPath(ident)), nil
+ exists, err := isTableDir(c.filesystem, c.tableToPath(ident))
+ if err != nil {
+ return false, err
+ }
+ if !exists {
Review Comment:
Two small things: (1) `CheckTableExists` now propagates `isTableDir` errors
where it previously returned `(false, nil)` — asymmetric vs. the tolerant
listing path; confirm intended or route via `isTableDirForListing`. (2) `if
!exists { return false, nil }; return true, nil` collapses to `return exists,
nil`.
--
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]
