laskoviymishka commented on code in PR #2368:
URL: https://github.com/apache/iceberg-go/pull/2368#discussion_r4431201303
##########
table/rolling_data_writer.go:
##########
@@ -309,16 +295,10 @@ func (w *writerFactory) openFileWriter(ctx
context.Context, partitionPath string
fileName := dataFileName(*w.writeUUID, cnt, partitionID, fileCount,
strings.ToLower(string(w.fileFormat)))
- var filePath string
if partitionPath != "" {
- partitionLoc, err := w.partitionLocProvider(partitionPath)
- if err != nil {
- return nil, err
- }
- filePath = partitionLoc.NewDataLocation(fileName)
- } else {
- filePath = w.locProvider.NewDataLocation(fileName)
+ fileName = partitionPath + "/" + fileName
Review Comment:
Could `NewDataLocation` take the partition as its own argument, like Java's
[`newDataLocation(PartitionSpec, StructLike,
String)`](https://github.com/apache/iceberg/blob/831b4ea108b9def6ae92258a7382aab096d87ed2/core/src/main/java/org/apache/iceberg/LocationProviders.java#L175-L182)
and PyIceberg's [`new_data_location(data_file_name,
partition_key)`](https://github.com/apache/iceberg-python/blob/79a05574b18e38c9298632437b6f20e93eca7f62/pyiceberg/table/locations.py#L62)?
Here the partition rides inside the name and the object-store provider [strips
it with
`path.Base`](https://github.com/apache/iceberg-go/blob/5ba52550fe00559e1dc886b1abbcf9ea07dd79d7/table/locations.go#L157-L165).
On
[`main`](https://github.com/apache/iceberg-go/blob/00fbb576ea4e8e9d87757189cc690f3c137dcbca/table/locations.go#L154-L165),
a name with `/` went to the simple provider unhashed, so direct
`LoadLocationProvider` callers now get a different path. It does change an
exported interface, so the title would need a `!` for apidiff.
##########
table/locations_test.go:
##########
@@ -125,7 +125,7 @@ func TestLocationProviderLiteralCustomPaths(t *testing.T) {
})
require.NoError(t, err)
assert.Equal(t, tt.data, provider.NewDataLocation("a"))
- assert.Equal(t, "s3://bucket/my data%20#?/partition=a
b%20#?/file.parquet",
+ assert.Equal(t, tt.partitioned,
Review Comment:
Could we add a multi-level `a=1/b=2/file.parquet` case here for both
partitioned-paths values? Every object-store case is single-level, so nothing
pins that
[`path.Base`](https://github.com/apache/iceberg-go/blob/5ba52550fe00559e1dc886b1abbcf9ea07dd79d7/table/locations.go#L162)
drops both directories when partitioned paths are off (that row should equal
the single-level `partitioned` value, since only `file.parquet` is hashed), or
that the hash covers the full name when they're on.
--
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]
