github-actions[bot] commented on code in PR #68481:
URL: https://github.com/apache/doris/pull/68481#discussion_r4091130527
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -2577,16 +2577,18 @@ private Split createIcebergSplit(FileScanTask
fileScanTask) throws UserException
}
split.setTableFormatType(TableFormatType.ICEBERG);
split.setTargetSplitSize(selectFeSplitSize(fileScanTask,
targetSplitSize));
- if (isPartitionedTable) {
- int specId = fileScanTask.file().specId();
+ // REPLACE or partition evolution can leave an unpartitioned table
with historical specs.
+ // Row-level deletes must retain each file's spec instead of
defaulting to historical spec 0.
+ int specId = dataFile.specId();
+ split.setPartitionSpecId(specId);
+ PartitionData partitionData = (PartitionData) dataFile.partition();
+ if (partitionData != null) {
PartitionSpec partitionSpec = icebergTable.specs().get(specId);
Preconditions.checkNotNull(partitionSpec, "Partition spec with
specId %s not found for table %s",
specId, icebergTable.name());
- PartitionData partitionData = (PartitionData)
fileScanTask.file().partition();
- if (partitionData != null) {
- split.setPartitionSpecId(specId);
- split.setPartitionDataJson(IcebergUtils.getPartitionDataJson(
- partitionData, partitionSpec,
sessionVariable.getTimeZone()));
+ split.setPartitionDataJson(IcebergUtils.getPartitionDataJson(
Review Comment:
[P1] Keep historical partition values round-trip safe
This newly sends every historical partition through `getPartitionDataJson`,
but that format is not lossless for valid identity specs. `getPartitionValues`
turns BINARY/FIXED serialization failures into JSON `null`, while the inverse
used by `IcebergWriterHelper.convertToPartitionData` has no UUID or TIME cases.
After dropping the last partition field, DELETE/UPDATE of a live old file
therefore either writes delete metadata in the null partition or fails while
rebuilding it. The existing binary-partition test already treats silent NULL
materialization as invalid, but the new evolution cases cover only INTEGER.
Please use a type- and binary-safe round trip (or reject unsupported DML before
planning) and add an evolved-spec case.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -2577,16 +2577,18 @@ private Split createIcebergSplit(FileScanTask
fileScanTask) throws UserException
}
split.setTableFormatType(TableFormatType.ICEBERG);
split.setTargetSplitSize(selectFeSplitSize(fileScanTask,
targetSplitSize));
- if (isPartitionedTable) {
- int specId = fileScanTask.file().specId();
+ // REPLACE or partition evolution can leave an unpartitioned table
with historical specs.
+ // Row-level deletes must retain each file's spec instead of
defaulting to historical spec 0.
+ int specId = dataFile.specId();
+ split.setPartitionSpecId(specId);
+ PartitionData partitionData = (PartitionData) dataFile.partition();
+ if (partitionData != null) {
PartitionSpec partitionSpec = icebergTable.specs().get(specId);
Preconditions.checkNotNull(partitionSpec, "Partition spec with
specId %s not found for table %s",
specId, icebergTable.name());
- PartitionData partitionData = (PartitionData)
fileScanTask.file().partition();
- if (partitionData != null) {
- split.setPartitionSpecId(specId);
- split.setPartitionDataJson(IcebergUtils.getPartitionDataJson(
- partitionData, partitionSpec,
sessionVariable.getTimeZone()));
+ split.setPartitionDataJson(IcebergUtils.getPartitionDataJson(
+ partitionData, partitionSpec,
sessionVariable.getTimeZone()));
Review Comment:
[P1] Normalize negative fractional timestamp partitions
The widened call also exposes a pre-epoch timestamp bug during ordinary scan
planning. `serializePartitionValue` derives seconds/nanos with `/` and `%`; for
`timestampMicros == -1`, Java produces second `0` and nano adjustment `-1000`,
so `LocalDateTime.ofEpochSecond` throws. After dropping an identity TIMESTAMP
partition field, any retained file with a negative fractional value now reaches
this code even though the current spec is unpartitioned, so SELECT/DELETE
planning fails before the row-id fix can help. Please normalize with
`Math.floorDiv`/`Math.floorMod` (or equivalent) and add this evolution case.
--
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]
