github-actions[bot] commented on code in PR #66012:
URL: https://github.com/apache/doris/pull/66012#discussion_r3701142382
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUtils.java:
##########
@@ -781,6 +781,33 @@ public static List<String>
getIdentityPartitionColumns(Table table) {
return new ArrayList<>(partitionColumns);
}
+ /**
+ * Get identity partition columns that exist in all partition specs.
+ * The legacy file scanner uses partition columns in the first scan range
for all ranges,
+ * so only common identity partition columns can be used for partition
pruning.
+ */
+ public static List<String> getCommonIdentityPartitionColumns(Table table) {
+ LinkedHashSet<Integer> commonSourceIds = new LinkedHashSet<>();
+ for (PartitionField field : table.spec().fields()) {
+ NestedField sourceField =
table.schema().findField(field.sourceId());
+ if (field.transform().isIdentity() && sourceField != null
Review Comment:
[P1] Exclude UUID when it maps to VARBINARY
This supported-type check is unaware of the catalog mapping. With
`enable_mapping_varbinary=true`, Iceberg UUID maps to `VARBINARY(16)`, but
`serializePartitionValue()` emits the 36-byte textual UUID. The legacy
partition filler stores those text bytes unchanged, so projected constants are
wrong; FileScannerV2 instead calls `DataTypeVarbinarySerDe::from_string`, which
is unsupported, and fails split preparation. The existing position-delete path
already rejects UUID under this mapping for the same lack of binary-safe
transport. Please keep it file-backed/omit this metadata in this mode, or add
an explicit UUID-to-16-byte carrier and end-to-end coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/FileGroupInfo.java:
##########
@@ -312,15 +313,16 @@ public void
createScanRangeLocationsSplittable(FileLoadScanNode.ParamCreateConte
Util.getOrInferCompressType(context.fileGroup.getFileFormatProperties().getCompressionType(),
fileStatus.path);
context.params.setCompressType(compressType);
- List<String> columnsFromPath =
BrokerUtil.parseColumnsFromPath(fileStatus.path,
- context.fileGroup.getColumnNamesFromPath());
+ BrokerUtil.ParsedColumnsFromPath columnsFromPath =
+
BrokerUtil.parseColumnsFromPathWithNullInfo(fileStatus.path,
Review Comment:
[P1] Preserve the bulk-load \N NULL contract
This generic parser intentionally returns `isNull=false` for `p=\N`, but
`COLUMNS FROM PATH` load ranges previously omitted the bitmap and the BE
partition filler explicitly treats `\N` as the legacy path NULL sentinel.
Supplying false now bypasses nullable serde: a string target loads literal
`\N`, while numeric/date targets can reject the range. Please use a
load-specific normalization policy (in both classic and Nereids planners) that
maps the legacy sentinel to an explicit true flag, and cover split and unsplit
bulk loads.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUtils.java:
##########
@@ -815,6 +841,10 @@ public static Map<String, String>
getIdentityPartitionInfoMap(PartitionData part
return partitionInfoMap;
}
+ private static boolean isSupportedPartitionValueType(TypeID typeId) {
Review Comment:
[P1] Do not transport TIMESTAMPTZ as zone-less text
This eligibility includes Iceberg `timestamptz`. When timestamp-TZ mapping
is enabled, the serializer converts the instant to the session zone and then
calls `toLocalDateTime()`, dropping the offset. Both BE partition parsers use
default `FormatOptions` with `timezone=null`, and `CastToTimestampTz` rejects a
datetime string that has neither an offset nor a supplied local zone.
Identity-partition scans therefore fail while preparing/filling the split.
Please preserve an explicit offset/UTC representation, pass a query timezone
under a compatible contract, or keep this type out of metadata constants, with
v1/v2 tests in a non-UTC session.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -376,21 +378,56 @@ private void setIcebergParams(TFileRangeDesc rangeDesc,
IcebergSplit icebergSpli
deleteFilesDescByReferencedDataFile.put(icebergSplit.getOriginalPath(),
nonEqualityDeleteFileDesc);
}
tableFormatFileDesc.setIcebergParams(fileDesc);
- Map<String, String> partitionValues =
icebergSplit.getIcebergPartitionValues();
- if (partitionValues != null) {
- List<String> fromPathKeys = new ArrayList<>();
- List<String> fromPathValues = new ArrayList<>();
- List<Boolean> fromPathIsNull = new ArrayList<>();
- for (Map.Entry<String, String> entry : partitionValues.entrySet())
{
- fromPathKeys.add(entry.getKey());
- fromPathValues.add(entry.getValue() != null ? entry.getValue()
: "");
- fromPathIsNull.add(entry.getValue() == null);
+ setPartitionValues(rangeDesc,
icebergSplit.getIcebergPartitionValues());
+ rangeDesc.setTableFormatParams(tableFormatFileDesc);
+ }
+
+ private List<String> getOrderedPathPartitionKeys() {
+ if (isSystemTable || icebergTable == null) {
+ return Collections.emptyList();
+ }
+ return IcebergUtils.getCommonIdentityPartitionColumns(icebergTable);
+ }
+
+ private List<String> getOrderedPartitionMetadataKeys() {
+ if (isSystemTable || icebergTable == null) {
+ return Collections.emptyList();
+ }
+ if (sessionVariable.enableFileScannerV2) {
+ return IcebergUtils.getIdentityPartitionColumns(icebergTable);
+ }
+ return getOrderedPathPartitionKeys();
+ }
+
+ @VisibleForTesting
+ void setPartitionValues(TFileRangeDesc rangeDesc, Map<String, String>
partitionValues) {
+ rangeDesc.unsetColumnsFromPathKeys();
+ rangeDesc.unsetColumnsFromPath();
+ rangeDesc.unsetColumnsFromPathIsNull();
+
+ List<String> orderedPartitionKeys = getOrderedPartitionMetadataKeys();
Review Comment:
[P2] Cache the ordered metadata keys once per scan
`setPartitionValues()` runs for every file split, but this helper walks
every historical partition spec and allocates new sets/lists each time. That
makes planning O(files x specs) and adds per-file allocation on large evolved
tables, although the result depends only on the statement-pinned table and
scanner mode. Please compute the ordered common/v2 key list once during
initialization and only filter that cached list against each split's value map.
--
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]
