laskoviymishka commented on code in PR #2175:
URL: https://github.com/apache/iceberg-go/pull/2175#discussion_r4242912269
##########
table/locations.go:
##########
@@ -50,8 +52,28 @@ type simpleLocationProvider struct {
metadataPath *url.URL
}
+// Native paths must not be parsed as URLs: percent signs, fragments, and
Review Comment:
Worth making this a proper godoc that starts with the function name and
spells out the contract the rest of the file leans on: an empty scheme means
"native path carried in `URL.Path`", anything else is a real URI.
`joinLocationPath` has no comment at all, and that empty-scheme convention is
load-bearing for every caller, so it shouldn't be implicit.
##########
table/locations.go:
##########
@@ -50,8 +52,28 @@ type simpleLocationProvider struct {
metadataPath *url.URL
}
+// Native paths must not be parsed as URLs: percent signs, fragments, and
+// query delimiters are literal filename characters.
+func parseLocationPath(location string) (*url.URL, error) {
+ schemeEnd := strings.IndexByte(location, ':')
+ separator := strings.IndexAny(location, `/\`)
+ if filepath.VolumeName(location) != "" || schemeEnd < 0 || (separator
>= 0 && separator < schemeEnd) {
+ return &url.URL{Path: location}, nil
+ }
+
+ return url.Parse(location)
+}
+
+func joinLocationPath(location *url.URL, elem ...string) string {
+ if location.Scheme == "" {
+ return filepath.Join(append([]string{location.Path},
elem...)...)
Review Comment:
`filepath.Join` emits OS-native separators and runs `Clean`, and both land
in the strings we persist to manifests and metadata. On Windows that writes
backslash paths other clients can't resolve, and `Clean` quietly collapses
`.`/`..`/trailing slashes in names we should be treating as opaque. I'd join
with `path.Join` (forward slashes) and convert to native separators only at the
I/O boundary.
##########
table/locations_test.go:
##########
@@ -18,16 +18,87 @@
package table_test
import (
+ "net/url"
+ "os"
+ "path/filepath"
"strings"
"testing"
"github.com/apache/iceberg-go"
+ "github.com/apache/iceberg-go/io"
"github.com/apache/iceberg-go/table"
"github.com/google/uuid"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
+func TestLocationProviderLocalPaths(t *testing.T) {
+ for _, objectStore := range []string{"false", "true"} {
+ for _, partitionPaths := range []string{"false", "true"} {
+ for _, name := range []string{"my table",
"literal%20table", "100% complete", "table#fragment", "table?query"} {
+ t.Run(objectStore+"/"+partitionPaths+"/"+name,
func(t *testing.T) {
+ if strings.Contains(name, "?") &&
os.PathSeparator == '\\' {
+ t.Skip("question marks are not
valid in Windows filenames")
+ }
+ root := filepath.Join(t.TempDir(), name)
+ for _, custom := range []bool{false,
true} {
+ props := iceberg.Properties{
+
table.ObjectStoreEnabledKey: objectStore,
+
table.WriteObjectStorePartitionedPathsKey: partitionPaths,
+ }
+ dataDir, metadataDir :=
filepath.Join(root, "data"), filepath.Join(root, "metadata")
+ if custom {
+ dataDir, metadataDir =
filepath.Join(root, "custom data"), filepath.Join(root, "custom metadata")
+
props[table.WriteDataPathKey] = dataDir
+
props[table.WriteMetadataPathKey] = metadataDir
+ }
+ provider, err :=
table.LoadLocationProvider(root, props)
+ require.NoError(t, err)
+ metadataFile, err :=
provider.NewTableMetadataFileLocation(1)
+ require.NoError(t, err)
+ for _, file := range []struct{
location, dir string }{
+
{provider.NewDataLocation("data.parquet"), dataDir},
+
{provider.NewDataLocation("partition=value/data.parquet"), dataDir},
+
{provider.NewMetadataLocation("manifest.avro"), metadataDir},
+ {metadataFile,
metadataDir},
+ } {
+ require.True(t,
strings.HasPrefix(file.location, file.dir+string(os.PathSeparator)), "%q is
outside %q", file.location, file.dir)
+ writer, err :=
(io.LocalFS{}).Create(file.location)
+ require.NoError(t, err)
+ require.NoError(t,
writer.Close())
+ _, err =
os.Stat(file.location)
+ require.NoError(t, err)
+ }
+ }
+ })
+ }
+ }
+ }
+}
+
+func TestLocationProviderEscapedURIs(t *testing.T) {
+ for _, scheme := range []string{"file", "s3", "gs", "az"} {
+ t.Run(scheme, func(t *testing.T) {
+ location := (&url.URL{Scheme: scheme, Host: "bucket",
Path: "/my table/literal%20#?"}).String()
+ provider, err := table.LoadLocationProvider(location,
nil)
+ require.NoError(t, err)
+ assert.Equal(t, location+"/data/data.parquet",
provider.NewDataLocation("data.parquet"))
+ assert.Equal(t, location+"/metadata/manifest.avro",
provider.NewMetadataLocation("manifest.avro"))
+ })
+ }
+}
+
+func TestLocationProviderRelativePaths(t *testing.T) {
+ for _, location := range []string{"my table", "100% complete",
"literal%20table", "table#fragment", "table?query", "dir/table:part%20"} {
+ t.Run(location, func(t *testing.T) {
+ provider, err := table.LoadLocationProvider(location,
nil)
+ require.NoError(t, err)
+ assert.Equal(t, filepath.Join(location, "data",
"data.parquet"), provider.NewDataLocation("data.parquet"))
Review Comment:
These expectations are computed with the same `filepath.Join` the
implementation uses, so the test can't catch the separator change: it'll pass
on Windows even though the stored path flipped to backslashes. Hardcode the
expected forward-slash strings, and add a case that would fail if a backslash
crept in.
##########
table/locations_test.go:
##########
@@ -18,16 +18,87 @@
package table_test
import (
+ "net/url"
+ "os"
+ "path/filepath"
"strings"
"testing"
"github.com/apache/iceberg-go"
+ "github.com/apache/iceberg-go/io"
"github.com/apache/iceberg-go/table"
"github.com/google/uuid"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
+func TestLocationProviderLocalPaths(t *testing.T) {
+ for _, objectStore := range []string{"false", "true"} {
+ for _, partitionPaths := range []string{"false", "true"} {
+ for _, name := range []string{"my table",
"literal%20table", "100% complete", "table#fragment", "table?query"} {
+ t.Run(objectStore+"/"+partitionPaths+"/"+name,
func(t *testing.T) {
+ if strings.Contains(name, "?") &&
os.PathSeparator == '\\' {
+ t.Skip("question marks are not
valid in Windows filenames")
+ }
+ root := filepath.Join(t.TempDir(), name)
+ for _, custom := range []bool{false,
true} {
+ props := iceberg.Properties{
+
table.ObjectStoreEnabledKey: objectStore,
+
table.WriteObjectStorePartitionedPathsKey: partitionPaths,
+ }
+ dataDir, metadataDir :=
filepath.Join(root, "data"), filepath.Join(root, "metadata")
+ if custom {
+ dataDir, metadataDir =
filepath.Join(root, "custom data"), filepath.Join(root, "custom metadata")
+
props[table.WriteDataPathKey] = dataDir
+
props[table.WriteMetadataPathKey] = metadataDir
+ }
+ provider, err :=
table.LoadLocationProvider(root, props)
+ require.NoError(t, err)
+ metadataFile, err :=
provider.NewTableMetadataFileLocation(1)
+ require.NoError(t, err)
+ for _, file := range []struct{
location, dir string }{
+
{provider.NewDataLocation("data.parquet"), dataDir},
+
{provider.NewDataLocation("partition=value/data.parquet"), dataDir},
+
{provider.NewMetadataLocation("manifest.avro"), metadataDir},
+ {metadataFile,
metadataDir},
+ } {
+ require.True(t,
strings.HasPrefix(file.location, file.dir+string(os.PathSeparator)), "%q is
outside %q", file.location, file.dir)
+ writer, err :=
(io.LocalFS{}).Create(file.location)
+ require.NoError(t, err)
+ require.NoError(t,
writer.Close())
+ _, err =
os.Stat(file.location)
+ require.NoError(t, err)
+ }
+ }
+ })
+ }
+ }
+ }
+}
+
+func TestLocationProviderEscapedURIs(t *testing.T) {
+ for _, scheme := range []string{"file", "s3", "gs", "az"} {
+ t.Run(scheme, func(t *testing.T) {
+ location := (&url.URL{Scheme: scheme, Host: "bucket",
Path: "/my table/literal%20#?"}).String()
Review Comment:
The input here is built with `url.URL{...}.String()`, so it's already
percent-encoded before it reaches the provider; parse-then-String just
round-trips it and the assertion passes while the actual bug stands. Feed the
raw, unencoded location instead (`s3://bucket/my table`) and assert the literal
key comes back unchanged; that's the string Java and REST catalogs actually
produce.
##########
table/locations.go:
##########
@@ -50,8 +52,28 @@ type simpleLocationProvider struct {
metadataPath *url.URL
}
+// Native paths must not be parsed as URLs: percent signs, fragments, and
+// query delimiters are literal filename characters.
+func parseLocationPath(location string) (*url.URL, error) {
+ schemeEnd := strings.IndexByte(location, ':')
+ separator := strings.IndexAny(location, `/\`)
+ if filepath.VolumeName(location) != "" || schemeEnd < 0 || (separator
>= 0 && separator < schemeEnd) {
Review Comment:
A scheme-less name with a colon before the first separator still gets sent
to `url.Parse` here: `db:2024` parses as an opaque URL, and `my table:v2`
errors out with "first path segment in URL cannot contain colon". That
contradicts the helper's own premise that native paths never hit the parser.
I'd match PyIceberg and gate on a real scheme (`^[A-Za-z][A-Za-z0-9+.-]*://`),
falling back to a native path otherwise, with a test row for `db:2024`.
##########
table/locations.go:
##########
@@ -50,8 +52,28 @@ type simpleLocationProvider struct {
metadataPath *url.URL
}
+// Native paths must not be parsed as URLs: percent signs, fragments, and
+// query delimiters are literal filename characters.
+func parseLocationPath(location string) (*url.URL, error) {
+ schemeEnd := strings.IndexByte(location, ':')
+ separator := strings.IndexAny(location, `/\`)
+ if filepath.VolumeName(location) != "" || schemeEnd < 0 || (separator
>= 0 && separator < schemeEnd) {
+ return &url.URL{Path: location}, nil
+ }
+
+ return url.Parse(location)
+}
+
+func joinLocationPath(location *url.URL, elem ...string) string {
+ if location.Scheme == "" {
+ return filepath.Join(append([]string{location.Path},
elem...)...)
+ }
+
+ return location.JoinPath(elem...).String()
Review Comment:
This branch still round-trips scheme URIs through `url.Parse` +
`JoinPath().String()`, so #2161 is only half-fixed: a raw `s3://bucket/my
table` comes back as `s3://bucket/my%20table/data/...`, and a literal `%`
double-encodes. Java and PyIceberg write and read the unencoded key, so we'd be
pointing at a different object (or splitting one table across two prefixes).
I'd drop the URL round-trip and join at the string level for every location
(trim the trailing `/`, append `/elem`) so URIs and local paths both stay
byte-identical to what the catalog gave us.
--
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]
