Gabriel39 commented on code in PR #68481:
URL: https://github.com/apache/doris/pull/68481#discussion_r4128952209
##########
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:
Fixed in 3c5652d8c4. Ordinary scans now omit unavailable row-ID partition
JSON when a historical partition type is unsupported, while scans requesting
the hidden row ID fail with an explicit UserException before generating delete
metadata. The strict serializer still never replaces unsupported values with
NULL. Added real metadata-evolution tests that remove the partition field, drop
its source column, reload the table, and exercise NULL/non-NULL values with
both partitioned and unpartitioned current specs. The read case reproduced the
UNKNOWN exception before the fix; both new tests now pass, including
preservation of the remaining identity partition metadata. Added Parquet/ORC
SQL regression cases for reads, DELETE/UPDATE rejection, unchanged results, and
no delete files. All 215 tests across the four affected Iceberg classes and FE
Checkstyle pass. The external SQL suite passes syntax validation; end-to-end
execution remains pending CI.
##########
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:
Addressed by rebasing onto branch-4.1 and retaining the UTC-offset transport
from #68532. TIMESTAMPTZ serialization now emits an explicit UTC offset instead
of a session-local timestamp without an offset. The inherited round-trip tests
exercise both instants of the New York DST overlap under UTC, Asia/Shanghai,
and America/New_York sessions, plus explicit-offset commit parsing. Those tests
passed in the fresh 215-test Iceberg run. The rebase also preserves
floorDiv/floorMod for negative fractional timestamps.
##########
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:
Rebase update: the target branch now provides a lossless 0x-prefixed
hexadecimal carrier for BINARY/FIXED through #68532. This PR retains that
serializer and its matching decoder instead of reintroducing the earlier Base64
format. The buffer-bound and evolved-spec round-trip tests were adjusted to the
target representation and pass. UUID support from the target branch and TIME
decoding from this PR are both retained.
--
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]
