github-actions[bot] commented on code in PR #65610:
URL: https://github.com/apache/doris/pull/65610#discussion_r3585496753
##########
be/test/format/table/iceberg/iceberg_reader_create_column_ids_test.cpp:
##########
@@ -1166,4 +1167,75 @@ TEST_F(IcebergReaderCreateColumnIdsTest,
test_create_column_ids_6) {
}
}
+// Regression: a synthesized/metadata slot (e.g. the TopN global row-id
column) is projected but
+// is never serialized into the Iceberg schema tree, so it is absent from the
schema-mapping
+// StructNode. Before the get_children().contains() guard,
_create_column_ids() called
+// StructNode::children_column_exists() on that unregistered name, which hits
+// DCHECK(children.contains(name)) and aborts in debug/ASAN builds (and throws
std::out_of_range
+// from .at() in release builds). These tests core without the guard and pass
with it: the
+// synthesized slot is skipped and only the real column contributes a column
id.
+TEST_F(IcebergReaderCreateColumnIdsTest,
parquet_synthesized_slot_is_skipped_not_crash) {
+ // Physical Parquet schema: a single real column "id" (Iceberg field id 1).
+ FieldDescriptor field_desc;
+ FieldSchema id_field;
+ id_field.name = "id";
+ id_field.data_type =
+
DataTypeFactory::instance().create_data_type(PrimitiveType::TYPE_BIGINT, true);
+ id_field.field_id = 1;
+ field_desc._fields.emplace_back(id_field);
+
+ // Schema-mapping node registers only the real table column "id" (mapped
to file column "id").
+ // The synthesized global row-id column is intentionally NOT registered as
a child.
+ auto struct_node = std::make_shared<TableSchemaChangeHelper::StructNode>();
+ struct_node->add_children("id", "id",
TableSchemaChangeHelper::ConstNode::get_instance());
+ std::shared_ptr<TableSchemaChangeHelper::Node> table_info_node =
struct_node;
+
+ // Projected tuple: the real column plus a synthesized global-row-id slot
absent from the node.
+ SlotDescriptor id_slot;
+ id_slot._type =
DataTypeFactory::instance().create_data_type(PrimitiveType::TYPE_BIGINT, true);
+ id_slot._col_name = "id";
+
+ SlotDescriptor row_id_slot;
+ row_id_slot._type =
+
DataTypeFactory::instance().create_data_type(PrimitiveType::TYPE_BIGINT, true);
+ row_id_slot._col_name = BeConsts::GLOBAL_ROWID_COL;
+
+ TupleDescriptor tuple_desc;
+ tuple_desc.add_slot(&id_slot);
+ tuple_desc.add_slot(&row_id_slot);
+
+ const ColumnIdResult result =
+ IcebergParquetReader::_create_column_ids(&field_desc, &tuple_desc,
table_info_node);
+ EXPECT_EQ(result.column_ids, (std::set<uint64_t> {1}));
+ EXPECT_TRUE(result.filter_column_ids.empty());
+}
+
+TEST_F(IcebergReaderCreateColumnIdsTest,
orc_synthesized_slot_is_skipped_not_crash) {
+ // Physical ORC schema: a single real column "id" carrying Iceberg field
id 1.
+ std::unique_ptr<orc::Type>
orc_type(orc::Type::buildTypeFromString("struct<id:bigint>"));
+
orc_type->getSubtype(0)->setAttribute(IcebergOrcReader::ICEBERG_ORC_ATTRIBUTE,
"1");
+
+ auto struct_node = std::make_shared<TableSchemaChangeHelper::StructNode>();
+ struct_node->add_children("id", "id",
TableSchemaChangeHelper::ConstNode::get_instance());
+ std::shared_ptr<TableSchemaChangeHelper::Node> table_info_node =
struct_node;
+
+ SlotDescriptor id_slot;
+ id_slot._type =
DataTypeFactory::instance().create_data_type(PrimitiveType::TYPE_BIGINT, true);
+ id_slot._col_name = "id";
+
+ SlotDescriptor row_id_slot;
+ row_id_slot._type =
+
DataTypeFactory::instance().create_data_type(PrimitiveType::TYPE_BIGINT, true);
+ row_id_slot._col_name = BeConsts::GLOBAL_ROWID_COL;
+
+ TupleDescriptor tuple_desc;
+ tuple_desc.add_slot(&id_slot);
+ tuple_desc.add_slot(&row_id_slot);
+
+ const ColumnIdResult result =
+ IcebergOrcReader::_create_column_ids(orc_type.get(), &tuple_desc,
table_info_node);
+ EXPECT_EQ(result.column_ids, (std::set<uint64_t> {1}));
+ EXPECT_TRUE(result.filter_column_ids.empty());
+}
+
Review Comment:
This touched C++ file is missing its final newline (`tail -c 1` returns
`0x73`, and the diff shows `\ No newline at end of file`). Please add the
trailing newline so the BE style/format checks do not fail on the new test.
##########
be/test/format/table/iceberg/iceberg_reader_create_column_ids_test.cpp:
##########
@@ -1166,4 +1167,75 @@ TEST_F(IcebergReaderCreateColumnIdsTest,
test_create_column_ids_6) {
}
}
+// Regression: a synthesized/metadata slot (e.g. the TopN global row-id
column) is projected but
+// is never serialized into the Iceberg schema tree, so it is absent from the
schema-mapping
+// StructNode. Before the get_children().contains() guard,
_create_column_ids() called
+// StructNode::children_column_exists() on that unregistered name, which hits
+// DCHECK(children.contains(name)) and aborts in debug/ASAN builds (and throws
std::out_of_range
+// from .at() in release builds). These tests core without the guard and pass
with it: the
+// synthesized slot is skipped and only the real column contributes a column
id.
+TEST_F(IcebergReaderCreateColumnIdsTest,
parquet_synthesized_slot_is_skipped_not_crash) {
+ // Physical Parquet schema: a single real column "id" (Iceberg field id 1).
+ FieldDescriptor field_desc;
+ FieldSchema id_field;
+ id_field.name = "id";
+ id_field.data_type =
+
DataTypeFactory::instance().create_data_type(PrimitiveType::TYPE_BIGINT, true);
+ id_field.field_id = 1;
+ field_desc._fields.emplace_back(id_field);
+
+ // Schema-mapping node registers only the real table column "id" (mapped
to file column "id").
+ // The synthesized global row-id column is intentionally NOT registered as
a child.
+ auto struct_node = std::make_shared<TableSchemaChangeHelper::StructNode>();
Review Comment:
These new BE unit tests only cover the unregistered synthesized-slot crash
with an identity schema mapping (`id` -> `id`). The production branch being
protected also relies on `children_file_column_name()` to keep column-id
pruning aligned with BY_NAME/name-mapping decisions, and the existing review
threads already caught bugs in that non-identity partial-id boundary. Please
add Parquet and ORC cases where the logical slot maps through `StructNode` to a
differently named physical field, ideally with an unrelated/stale field id
present, so the test would fail if `_create_column_ids()` regressed back to
id-only or identity-name binding.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -3991,6 +3991,9 @@ private void setFuzzyForCatalog(Random random) {
if (!Config.fuzzy_test_type.equals("external")) {
return;
}
+ // file scanner
Review Comment:
This new randomization currently does not run in the checked-in external
fuzzy pipeline. `setFuzzyForCatalog()` returns unless `Config.fuzzy_test_type`
is exactly `external`, but `regression-test/pipeline/external/conf/fe.conf`
enables fuzzy sessions with `fuzzy_test_type=p1`. Marking the variable
`fuzzy=true` only makes it appear in `printFuzzyVariables()`; it does not
randomize it. Since `enableFileScannerV2` defaults to `true`, those external
fuzzy runs still keep the FileScannerV2 path on and never exercise the intended
random legacy-scanner path from this change. Please either align this gate with
the actual external pipeline mode or update the pipeline/config so this branch
is reachable, and add a small check so the coverage cannot silently disappear
again.
--
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]
