github-actions[bot] commented on code in PR #67207:
URL: https://github.com/apache/doris/pull/67207#discussion_r3870945459
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonScanPlanProvider.java:
##########
@@ -596,6 +617,11 @@ private List<ConnectorScanRange> planScanInternal(
boolean countPushdown) {
PaimonTableHandle paimonHandle = (PaimonTableHandle) handle;
+ boolean requiresMetadataColumns = columns.stream()
+ .filter(column -> column instanceof PaimonColumnHandle)
+ .map(column -> ((PaimonColumnHandle) column).getName())
+ .anyMatch(name -> PAIMON_FILE_PATH_COL.equalsIgnoreCase(name)
+ || PAIMON_ROW_POSITION_COL.equalsIgnoreCase(name));
Review Comment:
[P1] Exclude synthetic metadata handles from the @options schema-mismatch
check. Both new handles intentionally map to -1 because they are absent from
table.rowType(); this filter retains -1 for every @options scan and the guard
below then throws before native split planning. As a result, SELECT
__paimon_file_path (or __paimon_row_index) FROM t@options(...) always fails.
Keep the negative-index failure for missing physical columns, but remove
recognized synthetic handles from the physical projection/check and add
metadata-only plus mixed @options coverage.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadata.java:
##########
@@ -1051,13 +1071,16 @@ static void applyCreateTableComment(Map<String, String>
tableProperties, String
*/
private void rejectReservedRowLineageColumns(ConnectorCreateTableRequest
request) {
int formatVersion =
IcebergSchemaBuilder.getEffectiveFormatVersion(request.getProperties(),
properties);
- if (formatVersion < ICEBERG_ROW_LINEAGE_MIN_VERSION) {
- return;
- }
for (ConnectorColumn column : request.getColumns()) {
String name = column.getName();
- if (ICEBERG_ROW_ID_COL.equalsIgnoreCase(name)
- ||
ICEBERG_LAST_UPDATED_SEQUENCE_NUMBER_COL.equalsIgnoreCase(name)) {
+ if (ICEBERG_FILE_PATH_COL.equalsIgnoreCase(name)
Review Comment:
[P1] Apply this reservation to schema evolution too. ADD COLUMN, batch ADD
COLUMNS, and top-level RENAME currently bypass this CREATE-only helper and
Iceberg accepts either unused name. After refresh, buildTableSchema appends a
second `_file`/`_pos` while buildColumnHandles overwrites the physical field
handle with id -1, so stored data can bind as metadata instead. Reuse a
case-insensitive guard on every top-level add/rename entry point, including the
single-part nested delegates, and cover the mutation paths in DDL tests.
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonScanPlanProvider.java:
##########
@@ -596,6 +617,11 @@ private List<ConnectorScanRange> planScanInternal(
boolean countPushdown) {
PaimonTableHandle paimonHandle = (PaimonTableHandle) handle;
+ boolean requiresMetadataColumns = columns.stream()
+ .filter(column -> column instanceof PaimonColumnHandle)
Review Comment:
[P1] Also require current-backend semantics for this Paimon projection. A
smooth-upgrade source is the old BE process; it ignores the new
original_file_path carrier and has neither Paimon metadata materializer. Since
this connector never emits REQUIRED_CURRENT_BACKEND_SEMANTICS,
PluginDrivenScanNode can still schedule
`__paimon_file_path`/`__paimon_row_index` there. Reuse this predicate in
getScanNodeProperties and add the corresponding smooth-source scheduling test.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java:
##########
@@ -3000,6 +3013,30 @@ private static boolean sessionBool(ConnectorSession
session, String key, boolean
return Boolean.parseBoolean(raw.trim());
}
+ private static boolean requiresMetadataColumns(List<ConnectorColumnHandle>
columns) {
+ return columns.stream()
+ .filter(column -> column instanceof IcebergColumnHandle)
+ .map(column -> ((IcebergColumnHandle) column).getName())
+ .anyMatch(name -> ICEBERG_FILE_PATH_COL.equalsIgnoreCase(name)
Review Comment:
[P1] Fence this projection from a smooth-upgrade-source BE. These field-id
-1 handles do not make requiresCurrentScanSemantics emit
REQUIRED_CURRENT_BACKEND_SEMANTICS, yet the source flag explicitly denotes the
old process and that BE lacks the new virtual mapper/materializers.
PluginDrivenScanNode has no other per-feature fence, so `_file`/`_pos` can be
scheduled where they cannot be produced. Use requiresMetadataColumns(columns)
when building scan-node properties and add a smooth-source compatibility test.
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonScanPlanProvider.java:
##########
@@ -596,6 +617,11 @@ private List<ConnectorScanRange> planScanInternal(
boolean countPushdown) {
PaimonTableHandle paimonHandle = (PaimonTableHandle) handle;
+ boolean requiresMetadataColumns = columns.stream()
Review Comment:
[P1] Exclude these synthetic handles from the physical schema-evolution
dictionary. getScanNodeProperties runs before planScanInternal and passes this
columns list to buildSchemaEvolutionParam; resolveCurrentSchemaFields then
treats both metadata names as physical fields and selectCurrentSchemaFields
throws because neither exists in the Paimon schema. Consequently even a default
non-@options metadata query fails before the new reader runs. Keep the handles
for validation/materialization, but filter them only from the dictionary input
and add default metadata-only and mixed property-construction tests.
##########
be/src/exec/scan/file_scanner_v2.cpp:
##########
@@ -196,7 +196,11 @@ bool is_wal_format(TFileFormatType::type format_type) {
bool is_partition_slot(const TFileScanSlotInfo& slot_info, const std::string&
column_name) {
if (column_name.starts_with(BeConsts::GLOBAL_ROWID_COL) ||
- column_name == BeConsts::ICEBERG_ROWID_COL) {
+ column_name == BeConsts::ICEBERG_ROWID_COL ||
+ column_name == BeConsts::ICEBERG_FILE_PATH_COL ||
Review Comment:
[P1] Do not override the explicit slot category by these connector-specific
raw names. This helper is shared by every FileScannerV2 scan: an ordinary
Hive/text column or partition key named `_file`, `_pos`, or either Paimon name
is marked REGULAR/PARTITION_KEY by FE, but this early return drops it before
the category is consulted. Text-family readers then omit the physical column,
and partition values are likewise lost. Actual new metadata slots already
arrive as SYNTHESIZED; trust that category (or scope any fallback to the
matching table format) and add unrelated data/partition-column coverage.
##########
regression-test/suites/external_table_p0/iceberg/test_iceberg_file_metadata_columns.groovy:
##########
@@ -0,0 +1,165 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_iceberg_file_metadata_columns",
"p0,external,iceberg,external_docker,external_docker_iceberg") {
+ String enabled = context.config.otherConfigs.get("enableIcebergTest")
+ if (enabled == null || !enabled.equalsIgnoreCase("true")) {
+ logger.info("Iceberg test is disabled")
+ return
+ }
+
+ String catalogName = "test_iceberg_file_metadata_columns"
+ String dbName = "test_iceberg_file_metadata_columns_db"
+ String restPort = context.config.otherConfigs.get("iceberg_rest_uri_port")
+ String minioPort = context.config.otherConfigs.get("iceberg_minio_port")
+ String externalEnvIp = context.config.otherConfigs.get("externalEnvIp")
+ String endpoint = "http://${externalEnvIp}:${minioPort}";
+
+ def verifyFileMetadata = { tableName, format ->
+ sql """refresh table ${dbName}.${tableName}"""
+
+ // Keep the golden result independent of UUID-based Iceberg data-file
names.
+ "order_qt_${format}_metadata_not_null" """
Review Comment:
[P1] Commit the generated expected output for this suite. The closure
defines three order_qt blocks and runs for both parquet and orc, but the
authoritative change has no matching
regression-test/data/external_table_p0/iceberg/test_iceberg_file_metadata_columns.out.
A clean verification run therefore has no six golden results to compare
against and cannot pass. Please record the suite with the prescribed regression
runner and include the generated .out file.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorMetadata.java:
##########
@@ -492,9 +494,20 @@ public ConnectorTableSchema getTableSchema(
* {@code iceberg.partition-spec} properties are table-level (not
schema-versioned). Factored out so the
* latest and at-snapshot paths share ONE assembly.
*/
- private ConnectorTableSchema buildTableSchema(String tableName, Table
table, Schema schema) {
+ private ConnectorTableSchema buildTableSchema(String tableName, Table
table, Schema schema,
+ boolean appendDataFileMetadataColumns) {
List<ConnectorColumn> columns = parseSchema(schema);
+ // Iceberg file metadata columns are always available for data tables,
but are hidden from
+ // SELECT * / DESCRIBE unless explicitly requested. They are
synthesized by the native BE
+ // reader and are not part of the Iceberg schema or physical file
projection.
+ if (appendDataFileMetadataColumns) {
Review Comment:
[P1] Handle pre-existing physical fields with either metadata name before
appending. Older Doris versions or another Iceberg client can already have
`_file`/`_pos`; this unconditional append creates a case-insensitive duplicate
that SchemaCacheValue rejects, so the table cannot load (and an exact-name
handle would also be overwritten with id -1). The new CREATE/ALTER guards only
prevent future mutations and cannot repair existing catalog state. Preserve the
physical column or fail with an explicit compatibility error before
constructing an invalid schema, and test a legacy/external table with both
names and case variants.
--
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]
