github-actions[bot] commented on code in PR #68481:
URL: https://github.com/apache/doris/pull/68481#discussion_r4093102490
##########
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] Preserve TIMESTAMPTZ instants across DST overlaps
This partition JSON is not round-trip safe for TIMESTAMPTZ in a non-UTC
session. The serializer turns the UTC instant into a session-local
LocalDateTime and drops its offset, while parseTimestampToMicros later
reattaches the zone. For example, in America/Los_Angeles, 1762075800000000
(2025-11-02T09:30Z, the later 01:30 occurrence) serializes as 2025-11-02T01:30
and Java parses that using the earlier offset, yielding 1762072200000000, one
hour earlier. With this widened call, DELETE/UPDATE over a retained historical
TIMESTAMPTZ partition can therefore commit delete metadata for the wrong
partition. This is distinct from the negative-fraction thread; the new tests
use UTC and do not exercise an overlap. Please use a
timezone-independent/lossless transport and add the later overlap occurrence.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUtils.java:
##########
@@ -1096,13 +1096,8 @@ public static List<String>
getPartitionValues(PartitionData partitionData, Parti
for (int i = 0; i < fields.size(); i++) {
NestedField field = fields.get(i);
Object value = partitionData.get(i);
- try {
- partitionValues.add(serializePartitionValue(field.type(),
value, timeZone));
- } catch (UnsupportedOperationException e) {
- LOG.warn("Failed to serialize Iceberg partition value for
field {}: {}", field.name(),
- e.getMessage());
- partitionValues.add(null);
- }
+ // These values also identify delete-file partitions; an
unsupported value must never become NULL.
+ partitionValues.add(serializePartitionValue(field.type(), value,
timeZone));
Review Comment:
[P1] Keep scans working after the partition source column is dropped
This now propagates every unsupported-type exception, but a valid historical
spec can intentionally expose an UNKNOWN partition field: Iceberg 1.11 uses
UnknownType when the old partition field's source column is no longer present
in the current schema (remove the partition field, then drop the column). The
runtime partition is declared with that UNKNOWN type even if its old primitive
value is still decoded, and serializePartitionValue has no UNKNOWN case, so it
throws regardless of the value. Because createIcebergSplit now calls this for
every data split, an ordinary SELECT over retained old files fails during
planning; previously the catch kept partitioned scans readable, and
current-unpartitioned scans skipped this serialization. This is separate from
the concrete BINARY/FIXED thread because the source type itself is gone. Please
keep read planning tolerant and reject only DML that cannot reconstruct the old
partition (or add an UNKNOWN-safe representation), with an evolution t
est that also drops the source column.
--
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]
