badalprasadsingh commented on code in PR #2521:
URL: https://github.com/apache/iceberg-go/pull/2521#discussion_r4577755208
##########
table/row_delta.go:
##########
@@ -465,6 +469,77 @@ func (rd *RowDelta) validateAddedDeletionVectors() error {
return nil
}
+// validateDeletesMatchDataFiles rejects an added position delete file
+// that names its data file in referenced_data_file, a deletion vector or a
+// v2 file-scoped position delete, when its partition spec or partition
+// differs from that data file's. The spec requires them to match. Scans
+// match deletion vectors by path alone, so a mismatched DV would otherwise
+// commit and be applied, and a mismatched v2 position delete would commit
+// a table whose scans fail. A reference to a data file that is neither
+// added here nor live in the current snapshot is left to the conflict
+// checks.
+func (rd *RowDelta) validateDeletesMatchDataFiles(ctx context.Context, fs
iceio.IO, meta *MetadataBuilder) error {
+ byRef := make(map[string][]iceberg.DataFile)
+ for _, f := range rd.delFiles {
+ if f.ContentType() != iceberg.EntryContentPosDeletes {
+ continue
+ }
+ if ref := explicitReferencedDataFile(f); ref != "" {
+ byRef[ref] = append(byRef[ref], f)
+ }
+ }
+ if len(byRef) == 0 {
+ return nil
+ }
+
+ check := func(data iceberg.DataFile) error {
+ dels, ok := byRef[data.FilePath()]
+ if !ok {
+ return nil
+ }
+ delete(byRef, data.FilePath())
+ for _, del := range dels {
+ if del.SpecID() != data.SpecID() {
+ return fmt.Errorf("delete file %s uses
partition spec %d, but its referenced data file %s uses partition spec %d;
write the delete file with the data file's spec",
+ del.FilePath(), del.SpecID(),
data.FilePath(), data.SpecID())
+ }
+ if !partitionsMatch(dataFilePartition(data),
dataFilePartition(del)) {
+ return fmt.Errorf("delete file %s partition
does not match its referenced data file %s",
+ del.FilePath(), data.FilePath())
+ }
Review Comment:
P1: "partitionsMatch" treats the same value in two go types as different.
"DayTransform.Transformer": givesan int32 day, but manifests decode
"iceberg.Date", so a valid DV is rejected; identity dates, timestamps, times
and UUIDs too.
better: can we compare "canonicalPartitionKey", as
"validateAddedDeletionVectorTargets" does? It handles decimals, so the
"comparablePartitionKey" change can go.
##########
table/row_delta.go:
##########
@@ -465,6 +469,77 @@ func (rd *RowDelta) validateAddedDeletionVectors() error {
return nil
}
+// validateDeletesMatchDataFiles rejects an added position delete file
+// that names its data file in referenced_data_file, a deletion vector or a
+// v2 file-scoped position delete, when its partition spec or partition
+// differs from that data file's. The spec requires them to match. Scans
+// match deletion vectors by path alone, so a mismatched DV would otherwise
+// commit and be applied, and a mismatched v2 position delete would commit
+// a table whose scans fail. A reference to a data file that is neither
+// added here nor live in the current snapshot is left to the conflict
+// checks.
+func (rd *RowDelta) validateDeletesMatchDataFiles(ctx context.Context, fs
iceio.IO, meta *MetadataBuilder) error {
+ byRef := make(map[string][]iceberg.DataFile)
+ for _, f := range rd.delFiles {
+ if f.ContentType() != iceberg.EntryContentPosDeletes {
+ continue
+ }
+ if ref := explicitReferencedDataFile(f); ref != "" {
+ byRef[ref] = append(byRef[ref], f)
+ }
Review Comment:
p1: this reads only the explicit "referenced_data_file", a v2 pos-delete
without it, whose "file_path" bounds name one data file, is path-scoped for
scans too, so a mismatched one still commits and scans of its data file fail
better: can we use "referencedDataFilePath(f)", iceberg-ava's
"ContentFileUtil" rule, and update the doc?
##########
table/dynamic_partition_overwrite_test.go:
##########
@@ -595,10 +595,10 @@ func
TestDynamicPartitionOverwriteKeepsDeleteForSurvivingOldSpecData(t *testing.
require.NotZero(t, fieldID)
nullPartition := map[int]any{fieldID: nil}
- // A v2 position delete can target an old-spec data file even if its
- // manifest stores it under a new partition tuple with a null value.
- posDelete := newTestPosDeleteFileForSpec(t, currentSpec,
"file://old-spec-delete.parquet",
- nullPartition, oldData.FilePath())
+ // A v2 position delete for an old-spec data file carries that file's
+ // spec and partition; overwriting the new spec's partitions must keep
it.
+ posDelete := newTestPosDeleteFileForSpec(t, oldSpec,
"file://old-spec-delete.parquet",
+ map[int]any{1000: int32(1)}, oldData.FilePath())
Review Comment:
the old fixture is what v2 mor Delete writes after spec evolution (current
spec, null new field). On the data file's spec, the delete is kept as another
spec's, so this test and lin773 no longer need the
"survivingDataPaths"/"removedDataPaths" checks
better: could you keep an old-layout case via the producer?
--
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]
