laskoviymishka commented on code in PR #1326:
URL: https://github.com/apache/iceberg-go/pull/1326#discussion_r3491574443
##########
catalog/hadoop/hadoop.go:
##########
@@ -509,10 +550,13 @@ func (c *Catalog) CommitTable(ctx context.Context, ident
table.Identifier, reqs
return nil, "", fmt.Errorf("hadoop catalog: failed to create
metadata directory: %w", err)
}
- newMetaPath := c.metadataFilePath(ident, newVersion)
- tempPath := filepath.Join(metaDir, uuid.New().String()+".metadata.json")
-
compression := updated.Properties().Get(table.MetadataCompressionKey,
table.MetadataCompressionDefault)
+ newMetaPath, err := c.metadataFilePathForCompression(ident, newVersion,
compression)
Review Comment:
The conflict guard below only stats the codec-specific path for
`newVersion`, so cross-codec races slip through. If writer A plans
`v2.gz.metadata.json` and writer B plans `v2.metadata.json` concurrently,
neither `Stat` sees the other and both commit — two different files both
claiming version 2, breaking the monotonic chain.
Java's `getMetadataFile` iterates every codec variant, so its conflict
detection is codec-agnostic by construction.
I'd make the guard probe all variants: call `metadataVersionLocation(ident,
newVersion)` and fail the commit if it finds any file for that version,
regardless of suffix. That also covers the migration case where someone
switches codecs between commits.
##########
catalog/hadoop/hadoop.go:
##########
@@ -184,8 +186,20 @@ func (c *Catalog) metadataDir(ident table.Identifier)
string {
return filepath.Join(c.tableToPath(ident), "metadata")
}
-func (c *Catalog) metadataFilePath(ident table.Identifier, version int) string
{
- return filepath.Join(c.metadataDir(ident),
fmt.Sprintf("v%d.metadata.json", version))
+func (c *Catalog) metadataFilePathForCompression(ident table.Identifier,
version int, compression string) (string, error) {
+ var suffix string
+ switch compression {
+ case table.MetadataCompressionCodecNone:
+ suffix = ".metadata.json"
+ case table.MetadataCompressionCodecGzip:
+ suffix = ".gz.metadata.json"
+ case table.MetadataCompressionCodecZstd:
+ suffix = ".zstd.metadata.json"
Review Comment:
I think the zstd branch here is a cross-client trap. `zstd` isn't a value
Java's `TableMetadataParser.Codec` understands — its enum is only `NONE("")`
and `GZIP(".gz")`, and `HadoopTableOperations.getMetadataFile` only ever probes
`Codec.values()`, so a `vN.zstd.metadata.json` is never even looked for.
PyIceberg falls back to `NOOP_COMPRESSOR` for any non-`.gz` suffix and reads
the compressed bytes as garbage, and iceberg-rust rejects zstd outright in
`parse_metadata_file_compression`.
So a Hadoop-catalog table we write with zstd is invisible-or-corrupt to
every other Iceberg client, and the Hadoop catalog is precisely the
shared-filesystem case where that interop matters most.
I'd gate zstd at this layer for the Hadoop catalog — return the same
"unsupported codec" error you already produce for `brotli`, so the failure is
loud at write time rather than a silent unreadable file. If the intent is
genuinely a go-to-go-only extension, that needs a prominent warning in the PR
description and CHANGES.md and a clear note in the code that this is
non-portable. wdyt?
##########
catalog/hadoop/hadoop.go:
##########
@@ -330,16 +355,30 @@ func (c *Catalog) findVersion(ident table.Identifier)
(int, error) {
return nil
})
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",
+ return "", 0, fmt.Errorf("hadoop catalog: no metadata files
found for table %s: %w",
+ strings.Join(ident, "."), catalog.ErrNoSuchTable)
+ }
+
+ metaPath, ok := c.metadataVersionLocation(ident, maxVer)
Review Comment:
The `WalkDir` above already saw the exact filename for `maxVer` (it matched
`versionPattern` to compute the version), but we throw the name away and only
keep the integer — then re-probe here with the fixed plain→gz→zstd order.
That re-probe is where stale data wins: if both `v5.metadata.json` (old) and
`v5.gz.metadata.json` (new) exist, plain is probed first and silently shadows
the compressed current version, so the catalog serves stale uncompressed
metadata even though the authoritative version is the gzip one. A compression
migration would be quietly defeated.
I'd capture the matched filename in the `WalkDir` callback (it's the `path`
param) when `v > maxVer`, and thread that straight into `scanForward` so we
never re-probe a file we just saw. That removes the shadowing and drops the
extra `Stat`s per lookup on the slow path.
##########
catalog/hadoop/hadoop_test.go:
##########
@@ -392,6 +416,33 @@ func (s *HadoopCatalogTestSuite)
TestFindVersionMixedGzipAndPlain() {
s.Equal(3, ver)
}
+func (s *HadoopCatalogTestSuite) TestFindVersionZstdOnlyWithHint() {
+ ident := []string{"ns", "tbl"}
+ dir := s.cat.metadataDir(ident)
+ s.Require().NoError(os.MkdirAll(dir, 0o755))
+ s.Require().NoError(os.WriteFile(filepath.Join(dir,
"v1.zstd.metadata.json"), nil, 0o644))
+ s.Require().NoError(os.WriteFile(filepath.Join(dir,
"v2.zstd.metadata.json"), nil, 0o644))
+ s.cat.writeVersionHint(ident, 1)
+
+ ver, err := s.cat.findVersion(ident)
+ s.Require().NoError(err)
+ s.Equal(2, ver)
Review Comment:
`TestFindVersionZstdOnlyWithHint` and `TestFindVersionMixedZstdAndPlain`
only assert the version number, never the returned path — so the part of the
fix that actually matters (that `findMetadataLocation` returns the
codec-suffixed path, not a reconstructed plain one) isn't directly exercised
here. The empty `nil` files are fine since these call only `Stat`, but the path
is the thing the LoadTable fix hinges on.
I'd add a direct `findMetadataLocation` assertion that the returned path
ends in `.zstd.metadata.json` for the zstd-only case and `.gz.metadata.json`
for a gzip-only one. That's also the natural place to add the shadowing case
from the comment on `findMetadataLocation` — write both `v3.metadata.json` and
`v3.gz.metadata.json` and assert which path wins, so the resolution is pinned
by a test.
##########
catalog/hadoop/hadoop_test.go:
##########
@@ -129,12 +129,24 @@ func (s *HadoopCatalogTestSuite) TestMetadataDir() {
s.Equal(filepath.Join(s.warehouse, "ns", "tbl", "metadata"), path)
}
-func (s *HadoopCatalogTestSuite) TestMetadataFilePath() {
- path := s.cat.metadataFilePath([]string{"ns", "tbl"}, 1)
+func (s *HadoopCatalogTestSuite) TestMetadataFilePathForCompression() {
+ ident := []string{"ns", "tbl"}
+
+ path, err := s.cat.metadataFilePathForCompression(ident, 1,
table.MetadataCompressionCodecNone)
+ s.Require().NoError(err)
s.Equal(filepath.Join(s.warehouse, "ns", "tbl", "metadata",
"v1.metadata.json"), path)
- path = s.cat.metadataFilePath([]string{"ns", "tbl"}, 42)
- s.Equal(filepath.Join(s.warehouse, "ns", "tbl", "metadata",
"v42.metadata.json"), path)
+ path, err = s.cat.metadataFilePathForCompression(ident, 1,
table.MetadataCompressionCodecGzip)
+ s.Require().NoError(err)
+ s.Equal(filepath.Join(s.warehouse, "ns", "tbl", "metadata",
"v1.gz.metadata.json"), path)
+
+ path, err = s.cat.metadataFilePathForCompression(ident, 1,
table.MetadataCompressionCodecZstd)
+ s.Require().NoError(err)
+ s.Equal(filepath.Join(s.warehouse, "ns", "tbl", "metadata",
"v1.zstd.metadata.json"), path)
Review Comment:
The old `TestMetadataFilePath` covered version 1 and version 42; the rewrite
only keeps version 1 across the three codecs. The multi-digit case is the
normal path for any table past v9, and `fmt.Sprintf("v%d%s", ...)` is exactly
where a width/format slip would hide.
I'd carry a `version=42` assertion over for at least one codec while we're
here.
##########
catalog/hadoop/hadoop.go:
##########
@@ -270,36 +284,47 @@ 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 {
+// metadataVersionLocation returns the path for the given metadata version in
+// plain, gzip-compressed, or zstd-compressed form.
+func (c *Catalog) metadataVersionLocation(ident table.Identifier, version int)
(string, bool) {
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
+ for _, path := range []string{
+ filepath.Join(dir, fmt.Sprintf("v%d.metadata.json", version)),
+ filepath.Join(dir, fmt.Sprintf("v%d.gz.metadata.json",
version)),
+ filepath.Join(dir, fmt.Sprintf("v%d.zstd.metadata.json",
version)),
+ } {
Review Comment:
Worth a one-line comment that any non-`ErrNotExist` `Stat` error (permission
denied, transient FS error) is treated as "not found" here. It's the same
behavior as the old `metadataVersionExists`, so not a regression, but on a slow
filesystem a transient error could make a real version look absent — calling it
out keeps the next reader from assuming it's only an existence check.
##########
catalog/hadoop/hadoop.go:
##########
@@ -51,15 +51,17 @@ func init() {
var _ catalog.Catalog = (*Catalog)(nil)
// versionPattern matches Hadoop catalog metadata filenames:
-// v1.metadata.json, v42.metadata.json, v1.gz.metadata.json, etc.
-var versionPattern = regexp.MustCompile(`^v([0-9]+)(?:\.gz)?\.metadata\.json$`)
+// v1.metadata.json, v42.metadata.json, v1.gz.metadata.json,
+// v1.zstd.metadata.json, etc.
+var versionPattern =
regexp.MustCompile(`^v([0-9]+)(?:\.(?:gz|zstd))?\.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: 00000-<uuid>.metadata.json,
+// 00000-<uuid>.gz.metadata.json, or 00000-<uuid>.zstd.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|zstd))?\.metadata\.json$`,
Review Comment:
Two small things here. The `// Java/PyIceberg catalogs` comment above is now
inaccurate for the zstd alternative — neither Java nor PyIceberg ever emits
`00000-<uuid>.zstd.metadata.json`, so I'd note the zstd part is an
iceberg-go-only extension rather than implying parity.
And recognizing UUID-style zstd here (used by `isTableDir`/`ListTables`)
while `findMetadataLocation`'s `WalkDir` still keys off `versionPattern` only
creates a confusing split: a user could create a table in Python, see it in
`ListTables`, then get `ErrNoSuchTable` on `LoadTable`. That load gap is
pre-existing, but I'd at least not widen the recognition surface without a `//
recognized but not loadable` note. The new zstd UUID branch also has no test
backing it.
##########
catalog/hadoop/hadoop_test.go:
##########
@@ -990,6 +1041,50 @@ func (s *HadoopCatalogTestSuite) TestCreateTableAndLoad()
{
s.Equal(len(created.Schema().Fields()), len(loaded.Schema().Fields()))
}
+func (s *HadoopCatalogTestSuite) TestCreateTableGzipMetadata() {
Review Comment:
These four new integration tests (`TestCreateTableGzipMetadata`,
`TestCreateTableZstdMetadata`, `TestCommitTableGzipMetadata`,
`TestCommitTableZstdMetadata`) repeat the same `os.Mkdir` + props map +
`CreateTable` + `FileExists` boilerplate, where the existing suite leans on a
`createTestTable` helper.
I'd fold the four into a `createTestTableWithCompression` helper or
table-driven subtests to match the rest of the suite, and while you're adding
zstd coverage, a `TestIsTableDirTrueUUIDMetadataZstd` to mirror the existing
UUID+gzip case would catch a `ztsd`-style typo in `uuidMetadataPattern`. Low
priority next to the blockers above.
--
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]
