badalprasadsingh commented on code in PR #2435: URL: https://github.com/apache/iceberg-go/pull/2435#discussion_r4483248490
########## table/transaction_location.go: ########## @@ -0,0 +1,47 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package table + +import ( + "fmt" + "strings" + + "github.com/apache/iceberg-go" +) + +// SetLocation stages a change to the table's base location. Subsequent +// writes in the transaction use the new location; existing files are not moved. +// The location is stored as provided. Call Commit to persist the change. Review Comment: P0: a trailing slash stays in the location, so a later v3 mor delete writes its dv to `new//data/...`, `FileIO.Open` rejects that path, so on s3 the table can't be scanned after commti can you strip it here and update the tests pinning it? ########## table/transaction_location.go: ########## @@ -0,0 +1,47 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package table + +import ( + "fmt" + "strings" + + "github.com/apache/iceberg-go" +) + +// SetLocation stages a change to the table's base location. Subsequent +// writes in the transaction use the new location; existing files are not moved. Review Comment: P1: this is missed by `RewriteDataFiles`: passes `t.tbl` to `ExecuteCompactionGroup`, so compacted files still go under the old location please build `t.StagedTable()` once above the loop and pass the table instead ########## table/partition_statistics_write.go: ########## @@ -164,7 +164,7 @@ func (t *Transaction) ComputePartitionStatistics(ctx context.Context) (*Partitio return nil, fmt.Errorf("partition statistics: %w", err) } - locationProvider, err := t.tbl.LocationProvider() + locationProvider, err := t.stagedLocationProvider() Review Comment: this changes nothing, revert this line please `ComputePartitionStatistics` returns early on any staged change (`meta.HasChanges()`), so staged and base locactions are always equal here ########## table/transaction_location.go: ########## @@ -0,0 +1,47 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package table + +import ( + "fmt" + "strings" + + "github.com/apache/iceberg-go" +) + +// SetLocation stages a change to the table's base location. Subsequent +// writes in the transaction use the new location; existing files are not moved. +// The location is stored as provided. Call Commit to persist the change. +func (t *Transaction) SetLocation(location string) error { Review Comment: P1: the thing is: moving to another bucket commits fine, but after `LoadTable` the FileIO opens only the new bucket, so every scan and append fails on the old files. #1078 was supposed to do this, can you reject a different bucket with `ErrNotImplemented` until that PR or similar lands, or simply document the limit for now? upto you. ########## table/transaction_location_test.go: ########## @@ -0,0 +1,265 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package table_test + +import ( + "encoding/json" + "errors" + "path/filepath" + "strings" + "testing" + + "github.com/apache/arrow-go/v18/arrow/array" + "github.com/apache/arrow-go/v18/arrow/memory" + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/table" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestTransactionSetLocationStagesMetadataUpdate(t *testing.T) { + tbl := newCommitTestTable(t) + original := tbl.Location() + tx := tbl.NewTransaction() + + newLocation := "s3://bucket/new-location///" + require.NoError(t, tx.SetLocation(newLocation)) + + staged, err := tx.StagedTable() + require.NoError(t, err) + assert.Equal(t, newLocation, staged.Location()) + assert.Equal(t, original, tbl.Location(), "the original table stays unchanged until commit") + + commit, err := tx.TableCommit() + require.NoError(t, err) + require.Len(t, commit.Updates, 1) + assert.Equal(t, table.UpdateSetLocation, commit.Updates[0].Action()) + updateJSON, err := json.Marshal(commit.Updates[0]) + require.NoError(t, err) + assert.JSONEq(t, "{\"action\":\"set-location\",\"location\":\"s3://bucket/new-location///\"}", string(updateJSON)) Review Comment: nit: a backtick string avoids the escaped quotes here ########## table/transaction_location_test.go: ########## @@ -0,0 +1,265 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package table_test + +import ( + "encoding/json" + "errors" + "path/filepath" + "strings" + "testing" + + "github.com/apache/arrow-go/v18/arrow/array" + "github.com/apache/arrow-go/v18/arrow/memory" + "github.com/apache/iceberg-go" + "github.com/apache/iceberg-go/table" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestTransactionSetLocationStagesMetadataUpdate(t *testing.T) { + tbl := newCommitTestTable(t) + original := tbl.Location() + tx := tbl.NewTransaction() + + newLocation := "s3://bucket/new-location///" + require.NoError(t, tx.SetLocation(newLocation)) + + staged, err := tx.StagedTable() + require.NoError(t, err) + assert.Equal(t, newLocation, staged.Location()) + assert.Equal(t, original, tbl.Location(), "the original table stays unchanged until commit") + + commit, err := tx.TableCommit() + require.NoError(t, err) + require.Len(t, commit.Updates, 1) + assert.Equal(t, table.UpdateSetLocation, commit.Updates[0].Action()) + updateJSON, err := json.Marshal(commit.Updates[0]) + require.NoError(t, err) + assert.JSONEq(t, "{\"action\":\"set-location\",\"location\":\"s3://bucket/new-location///\"}", string(updateJSON)) + require.Len(t, commit.Requirements, 2) + assert.Equal(t, "assert-ref-snapshot-id", commit.Requirements[0].GetType()) + assert.Equal(t, "assert-table-uuid", commit.Requirements[1].GetType()) +} + +func TestTransactionSetLocationNoOp(t *testing.T) { + tbl := newCommitTestTable(t) + tx := tbl.NewTransaction() + require.NoError(t, tx.SetLocation(tbl.Location())) + + commit, err := tx.TableCommit() + require.NoError(t, err) + assert.Empty(t, commit.Updates) +} + +func TestTransactionSetLocationRepeatedSameLocation(t *testing.T) { + tx := newCommitTestTable(t).NewTransaction() + require.NoError(t, tx.SetLocation("s3://bucket/new")) + require.NoError(t, tx.SetLocation("s3://bucket/new")) + + commit, err := tx.TableCommit() + require.NoError(t, err) + require.Len(t, commit.Updates, 1) +} + +func TestTransactionSetLocationPreservesTrailingSlash(t *testing.T) { + tx := newCommitTestTable(t).NewTransaction() + require.NoError(t, tx.SetLocation("s3://bucket/new/")) + + staged, err := tx.StagedTable() + require.NoError(t, err) + assert.Equal(t, "s3://bucket/new/", staged.Location()) +} + +func TestTransactionSetLocationRejectsEmptyLocation(t *testing.T) { + for _, location := range []string{"", " ", "\t\n"} { + t.Run("location="+location, func(t *testing.T) { + tx := newCommitTestTable(t).NewTransaction() + err := tx.SetLocation(location) + require.Error(t, err) + assert.True(t, errors.Is(err, iceberg.ErrInvalidArgument)) Review Comment: replace both lines with: ```go require.ErrorIs(t, err, iceberg.ErrInvalidArgument) ``` ########## table/transaction.go: ########## @@ -2500,7 +2500,7 @@ func (t *Transaction) Overwrite(ctx context.Context, rdr array.RecordReader, sna return err } - itr := recordsToDataFiles(ctx, t.tbl.Location(), meta, recordWritingArgs{ + itr := recordsToDataFiles(ctx, meta.loc, meta, recordWritingArgs{ Review Comment: P2: #2381 adds `DynamicPartitionOverwrite` with `t.tbl.Location()`, so with both merged its files go to the old location after SetLocation. whichever lands second should use `meta.loc` and cover it with a test. -- 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]
