morningman commented on code in PR #69229:
URL: https://github.com/apache/doris/pull/69229#discussion_r4308089858
##########
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:
Fixed in `a755a5444f9`. The mechanism is right: a field whose source column
was dropped has the UNKNOWN type, and the check rejected the tuple even when
the value is null. Confirmed with unit tests on the previous revision: the
`void(p)` field a v1 table keeps after removing `p` was rejected with `...
unsupported partition types in spec 1`, for a mixed spec (`void(p),
identity(q)`) and for an all-void one, and so was the file of a historical NULL
partition of the dropped column.
A null needs no type, so `getPartitionValues` now emits null for a null
value of any type, and only a non-null value whose type cannot be serialized
still throws. Iceberg keeps the value of a historical field after its source
column is dropped (the existing tests read a `p=7` file back from the manifest
after the drop and are still rejected), so this does not reopen the NULL delete
partition this PR fixes.
End to end on a local cluster, the regression now deletes the row in the
`partition_key = NULL` file after the column is dropped (Parquet and ORC) and
compares the table with Spark; it fails on the previous revision and passes
now. The v1 scenario itself does not reach this scan check today: Doris write
planning rejects any write to a v1 table whose current spec holds a `void`
field, before scanning, with `pinned spec 1 is not available with the same
definition` when every field is void (even when no column was dropped) and with
`Iceberg partition field 1000 references source field 2 outside pinned schema
1` when the void field's source column was dropped. That is a separate defect,
and branch-4.x has the same write planning code; it will be fixed in its own
PR. Until then the void cases are covered by
`rowIdScanKeepsNullOfRetainedVoidFieldWhoseSourceColumnWasDropped` (mixed spec,
expects `[null,"5"]` under spec 1) and
`rowIdScanAcceptsAllVoidSpecWhoseSourceColumnWasDropped`, and
the serializer by
`partitionDataJsonKeepsNullOfFieldWhoseSourceColumnWasDropped`. Serializing a
null by its type turns them red.
branch-4.x #68481 handles nulls the same way (its `serializePartitionValue`
throws in the default branch before looking at the value); we will pass this on
to the branch maintainers.
##########
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:
Fixed in `a755a5444f9`. Agreed, this came from leaving `partitionDataJson`
null. Falling back to the engine count would not be conservative here:
`listPartitions` reports such a table as unpartitioned once a partition source
column is dropped, so the Nereids count is no upper bound.
A file whose tuple cannot be serialized now keeps its raw values:
`IcebergPartitionUtils.getRawPartitionKey` renders them as a JSON array in spec
order, binary as hex and null apart from the string `"null"`.
`IcebergScanRange.getScannedPartitionKey` returns `specId|raw<values>` for such
a file, so each distinct tuple counts once, as legacy `IcebergScanNode` keyed
the count by the `PartitionData` itself. The key is only used for the count; a
row-ID scan of these files is still rejected.
Tests: `scannedPartitionCountKeepsPartitionsWhoseSourceColumnWasDropped`
(two old files of `p=7`, one of `p=8` and one new file of `q=1` count as 3; it
was 1) and `rawPartitionKeyIdentifiesTupleThatCannotBeSerialized`. Ignoring the
raw key, rendering binary with `toString()`, or writing null as `"null"` each
turns them red.
--
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]
