github-actions[bot] commented on code in PR #69229:
URL: https://github.com/apache/doris/pull/69229#discussion_r4306277393
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergPartitionUtils.java:
##########
@@ -206,15 +207,7 @@ static List<String> getPartitionValues(PartitionData
partitionData, PartitionSpe
List<String> partitionValues = new ArrayList<>(fields.size());
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, zone));
- } catch (UnsupportedOperationException e) {
- LOG.warn("Failed to serialize Iceberg partition value for
field {}: {}", field.name(),
- e.getMessage());
- partitionValues.add(null);
- }
+ partitionValues.add(serializePartitionValue(fields.get(i).type(),
partitionData.get(i), zone));
Review Comment:
[P2] Preserve nulls for retained void partition fields
A v1 table can remove partition field `p` (Iceberg retains it as `void(p)`),
write files under that spec, drop source column `p`, and then upgrade to v2.
The retained void field now has UNKNOWN type but a guaranteed null value. This
call throws for UNKNOWN even when the value is null, so those safe files are
marked unavailable and DELETE/UPDATE/MERGE is rejected. This also happens when
only files from the new void spec remain, so rewriting historical files as the
error suggests cannot fix it. Previously the fallback emitted the correct null,
and `IcebergWriterHelper` safely handles it (an all-void spec needs no delete
partition at all). Please preserve guaranteed void/null fields while rejecting
unknown identity values, and cover this evolution in a test. The retained-field
behavior is defined by the pinned [Iceberg partition-spec
implementation](https://github.com/apache/iceberg/blob/apache-iceberg-1.12.0/core/src/main/java/org/apache/iceberg/BaseUpdatePartitionSpec.java#L278-L30
6).
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java:
##########
@@ -1701,13 +1708,20 @@ private PerFileScratch computePerFileInvariants(Table
table, DataFile dataFile,
perFileInvariantComputeCount++;
Integer partitionSpecId = null;
String partitionDataJson = null;
+ boolean partitionDataUnavailable = false;
Map<String, String> partitionValues = Collections.emptyMap();
if (partitioned && dataFile.partition() instanceof PartitionData) {
PartitionData partitionData = (PartitionData) dataFile.partition();
int specId = dataFile.specId();
PartitionSpec spec = table.specs().get(specId);
partitionSpecId = specId;
- partitionDataJson =
IcebergPartitionUtils.getPartitionDataJson(partitionData, spec, zone);
+ try {
+ partitionDataJson =
IcebergPartitionUtils.getPartitionDataJson(partitionData, spec, zone);
+ } catch (UnsupportedOperationException e) {
+ // A historical partition field whose source column was
dropped has no serializable type. Only
+ // row IDs need the tuple, so the file stays readable and a
row-ID scan is rejected per call.
+ partitionDataUnavailable = true;
Review Comment:
[P2] Keep partition accounting complete when JSON is unavailable
Leaving `partitionDataJson` null also makes
`IcebergScanRange.getScannedPartitionKey()` return null. For example, retain an
old file under `identity(p)`, evolve to `identity(q)`, drop source column `p`,
and append a new file under `q`. An ordinary full scan still reads both files,
but `scannedPartitionCount()` now counts only the new partition and returns a
present count of 1; before this change the old spec also contributed a key.
`PluginDrivenScanNode.resolveSelectedPartitionNum()` trusts that partial count,
so EXPLAIN underreports partitions and a `sql_block_rule` with
`partition_num=1` permits a scan it previously rejected. Preserve a separate
accounting identity or fall back conservatively whenever any scanned partition
cannot be represented, rather than reporting the supported subset as the
complete count.
--
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]
