laskoviymishka commented on code in PR #2521:
URL: https://github.com/apache/iceberg-go/pull/2521#discussion_r4538026148
##########
table/row_delta.go:
##########
@@ -465,6 +469,72 @@ func (rd *RowDelta) validateAddedDeletionVectors() error {
return nil
}
+// validateDVsMatchDataFiles rejects an added deletion vector whose
+// partition spec or partition differs from the data file it references.
+// The spec requires them to match, and scans match deletion vectors to
+// data files by path alone, so a mismatched DV would otherwise commit and
+// be applied as if it were valid. 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) validateDVsMatchDataFiles(ctx context.Context, fs
iceio.IO, meta *MetadataBuilder) error {
+ byRef := make(map[string][]iceberg.DataFile)
+ for _, f := range rd.delFiles {
+ if ref := explicitReferencedDataFile(f); IsDeletionVector(f) &&
ref != "" {
Review Comment:
Could we cover v2 position deletes with `referenced_data_file` in this PR
too? That's our answer to the question in the description. The
`IsDeletionVector(f)` filter skips them today
([row_delta.go#L481-L483](https://github.com/apache/iceberg-go/blob/586a312e55f4872a13ae882921a0e3a1279140de/table/row_delta.go#L481-L483)),
and #2520 covers them as well. It means moving
`TestDynamicPartitionOverwriteKeepsDeleteForSurvivingOldSpecData` and its
pruned variant to the data file's spec, which also fits what you describe the
scan side doing since #2471.
##########
table/row_delta.go:
##########
@@ -465,6 +469,72 @@ func (rd *RowDelta) validateAddedDeletionVectors() error {
return nil
}
+// validateDVsMatchDataFiles rejects an added deletion vector whose
+// partition spec or partition differs from the data file it references.
+// The spec requires them to match, and scans match deletion vectors to
+// data files by path alone, so a mismatched DV would otherwise commit and
+// be applied as if it were valid. 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) validateDVsMatchDataFiles(ctx context.Context, fs
iceio.IO, meta *MetadataBuilder) error {
+ byRef := make(map[string][]iceberg.DataFile)
+ for _, f := range rd.delFiles {
+ if ref := explicitReferencedDataFile(f); IsDeletionVector(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("deletion vector %s uses
partition spec %d, but its referenced data file %s uses partition spec %d;
write the deletion vector with the data file's spec",
+ del.FilePath(), del.SpecID(),
data.FilePath(), data.SpecID())
+ }
+ if !partitionsMatch(dataFilePartition(data),
dataFilePartition(del)) {
Review Comment:
Could this compare partition values rather than Go types? A test that
commits a `decimal(9,2)` data file and then a DV with the same `{1000:
iceberg.Decimal{Val: 1234, Scale: 2}}` map fails on this head with "partition
does not match" and commits on `main`. The manifest decodes the data file's
value as `DecimalLiteral`
([manifest.go#L2754-L2767](https://github.com/apache/iceberg-go/blob/586a312e55f4872a13ae882921a0e3a1279140de/manifest.go#L2754-L2767)),
while the DV keeps the caller's `iceberg.Decimal`, which is also what our
partitioned writer uses
([partitioned_fanout_writer.go#L1262-L1267](https://github.com/apache/iceberg-go/blob/586a312e55f4872a13ae882921a0e3a1279140de/table/partitioned_fanout_writer.go#L1262-L1267)).
Normalizing both sides before `partitionsMatch`, plus a decimal acceptance
case, would cover it.
--
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]
