github-actions[bot] commented on code in PR #65851:
URL: https://github.com/apache/doris/pull/65851#discussion_r3621307203
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -532,33 +537,89 @@ public void createScanRangeLocations() throws
UserException {
super.createScanRangeLocations();
// Extract name mapping from Iceberg table properties
Map<Integer, List<String>> nameMapping = extractNameMapping();
+ Schema scanSchema = getQuerySchema();
+ List<Column> scanColumns = getScanColumns(scanSchema);
Review Comment:
[P1] Preserve dropped equality-key defaults
This metadata contains only `getQuerySchema()`, which is insufficient for
equality-delete fields that have since been dropped. For example: write an old
file without `k`, add optional `k INT` with initial default `7`, commit an
equality delete for `k = 7`, then drop `k` in a schema-only update. The delete
remains applicable, but the current schema sent here has no field `k`; V1 fails
with missing schema metadata, while V2 falls back to a NULL literal, so the old
row survives. Iceberg requires dropped equality columns to keep participating
in delete matching. Please retain field-ID-bound type/default metadata from
schema history for every equality field referenced by the planned delete files,
and cover this add-default/delete/drop sequence in both scanners.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -532,33 +537,89 @@ public void createScanRangeLocations() throws
UserException {
super.createScanRangeLocations();
// Extract name mapping from Iceberg table properties
Map<Integer, List<String>> nameMapping = extractNameMapping();
+ Schema scanSchema = getQuerySchema();
+ List<Column> scanColumns = getScanColumns(scanSchema);
// Equality-delete keys are hidden scan dependencies and need not
appear in the query
// projection. Both scanners need the complete current schema to
resolve field ids,
// historical names, types, and initial defaults when an old data file
lacks such a key.
- ExternalUtil.initSchemaInfoForAllColumn(params, -1L,
source.getTargetTable().getColumns(),
- nameMapping, getBase64EncodedInitialDefaultsForScan());
+ ExternalUtil.initSchemaInfoForAllColumn(params, -1L, scanColumns,
nameMapping,
Review Comment:
[P1] Gate recursive defaults during smooth upgrade
These defaults depend on new BE readers consuming the recursive `TField`
metadata. A smooth-upgrade source BE remains query-eligible, but the prior
Parquet/ORC readers ignore this metadata and fill missing nested children with
NULL; complex top-level slots also have only the intentional FE NULL
placeholder. The same query can therefore return defaults on an upgraded BE and
NULL on a source BE. Please reject or exclude `isSmoothUpgradeSrc()` backends
when this scan needs nested/complex initial-default materialization (as the
position-deletes path already does), or provide an old-BE-compatible carrier.
##########
be/test/format/table/iceberg/iceberg_reader_test.cpp:
##########
@@ -1049,6 +1270,341 @@ TEST_F(IcebergReaderTest,
v1_position_delete_read_error_releases_cache_entry) {
EXPECT_NE(status.to_string().find(delete_file.path), std::string::npos);
}
+TEST_F(IcebergReaderTest,
v1_rejects_missing_required_field_without_initial_default) {
+ schema::external::TField field;
+ field.__set_name("required_added");
+ field.__set_is_optional(false);
+
+ ColumnPtr column;
+ const auto status = iceberg::create_initial_default_column(
+ field, std::make_shared<DataTypeInt32>(), &column);
+
+ ASSERT_FALSE(status.ok());
+ EXPECT_NE(status.to_string().find("required_added"), std::string::npos);
+ EXPECT_NE(status.to_string().find("no initial default"),
std::string::npos);
+}
+
+// GTest assertion macros inflate clang-tidy's cognitive-complexity score.
+// NOLINTNEXTLINE(readability-function-cognitive-complexity)
+TEST_F(IcebergReaderTest,
v1_reuses_prepared_complex_initial_default_across_block_types) {
+ schema::external::TField field;
+ field.__set_id(1);
+ field.__set_name("nested");
+ field.__set_is_optional(true);
+ field.__set_initial_default_value("{}");
+ auto child = std::make_shared<schema::external::TField>();
+ child->__set_id(2);
+ child->__set_name("added");
+ child->__set_is_optional(false);
+ child->__set_initial_default_value("7");
+ schema::external::TFieldPtr child_ptr;
+ child_ptr.__set_field_ptr(std::move(child));
+ schema::external::TStructField struct_field;
+ struct_field.__set_fields({child_ptr});
+ field.nestedField.__set_struct_field(struct_field);
+ field.__isset.nestedField = true;
+
+ auto make_type = [] {
+ return make_nullable(std::make_shared<DataTypeStruct>(
+ DataTypes {std::make_shared<DataTypeInt32>()}, Strings
{"added"}));
+ };
+ const auto first_type = make_type();
+ const auto second_type = make_type();
+ ASSERT_NE(first_type.get(), second_type.get());
+ ASSERT_TRUE(first_type->equals(*second_type));
+
+ std::unordered_map<int32_t, std::pair<DataTypePtr, ColumnPtr>>
prepared_values;
+ ColumnPtr first_batch = first_type->create_column();
+ ASSERT_TRUE(
+ iceberg::append_initial_default(field, first_type, 2,
&prepared_values, &first_batch)
+ .ok());
+ ASSERT_EQ(prepared_values.size(), 1);
+ const auto* prepared_value = prepared_values.at(field.id).second.get();
+
+ ColumnPtr second_batch = second_type->create_column();
+ ASSERT_TRUE(
+ iceberg::append_initial_default(field, second_type, 3,
&prepared_values, &second_batch)
+ .ok());
+ ASSERT_EQ(prepared_values.at(field.id).second.get(), prepared_value);
+ ASSERT_EQ(prepared_values.at(field.id).first.get(), first_type.get());
+ ASSERT_EQ(first_batch->size(), 2);
+ ASSERT_EQ(second_batch->size(), 3);
+ for (size_t row = 0; row < first_batch->size(); ++row) {
+ Field value;
+ first_batch->get(row, value);
+ ASSERT_EQ(value.get<TYPE_STRUCT>().size(), 1);
+ EXPECT_EQ(value.get<TYPE_STRUCT>()[0].get<TYPE_INT>(), 7);
+ }
+ for (size_t row = 0; row < second_batch->size(); ++row) {
+ Field value;
+ second_batch->get(row, value);
+ ASSERT_EQ(value.get<TYPE_STRUCT>().size(), 1);
+ EXPECT_EQ(value.get<TYPE_STRUCT>()[0].get<TYPE_INT>(), 7);
+ }
+}
+
+// GTest assertion macros inflate clang-tidy's cognitive-complexity score.
+// NOLINTNEXTLINE(readability-function-cognitive-complexity)
+TEST_F(IcebergReaderTest, v1_materializes_complex_initial_defaults) {
+ auto make_field = [](const std::string& name, int32_t id, bool optional,
+ std::optional<std::string> initial_default =
std::nullopt,
+ bool binary_like = false) {
+ auto field = std::make_shared<schema::external::TField>();
+ field->__set_name(name);
+ field->__set_id(id);
+ field->__set_is_optional(optional);
+ if (initial_default.has_value()) {
+ field->__set_initial_default_value(*initial_default);
+ }
+ if (binary_like) {
+ field->__set_initial_default_value_is_base64(true);
+ }
+ schema::external::TFieldPtr result;
+ result.__set_field_ptr(std::move(field));
+ return result;
+ };
+
+ const auto required_int_type = std::make_shared<DataTypeInt32>();
+ const auto optional_string_type =
make_nullable(std::make_shared<DataTypeString>());
+ const auto struct_type = make_nullable(
+ std::make_shared<DataTypeStruct>(DataTypes {required_int_type,
optional_string_type},
+ Strings {"required_added",
"optional_added"}));
+ auto required_added = make_field("required_added", 2, false, "7");
+ required_added.field_ptr->__set_name_mapping({"legacy_required_added"});
+ auto optional_added = make_field("optional_added", 3, true);
+ schema::external::TStructField struct_metadata;
+ struct_metadata.__set_fields({required_added, optional_added});
+ auto struct_field = make_field("struct_default", 1, true, "{}");
+ struct_field.field_ptr->nestedField.__set_struct_field(struct_metadata);
+ struct_field.field_ptr->__isset.nestedField = true;
+
+ ColumnPtr struct_column;
+
ASSERT_TRUE(iceberg::create_initial_default_column(*struct_field.field_ptr,
struct_type,
+ &struct_column)
+ .ok());
+ Field struct_value;
+ struct_column->get(0, struct_value);
+ const auto& struct_fields = struct_value.get<TYPE_STRUCT>();
+ ASSERT_EQ(struct_fields.size(), 2);
+ EXPECT_EQ(struct_fields[0].get<TYPE_INT>(), 7);
+ EXPECT_TRUE(struct_fields[1].is_null());
+
+ const auto pruned_struct_type =
make_nullable(std::make_shared<DataTypeStruct>(
+ DataTypes {required_int_type}, Strings {"legacy_required_added"}));
+ ColumnPtr pruned_struct_column;
+
ASSERT_TRUE(iceberg::create_initial_default_column(*struct_field.field_ptr,
pruned_struct_type,
+ &pruned_struct_column)
+ .ok());
+ Field pruned_struct_value;
+ pruned_struct_column->get(0, pruned_struct_value);
+ const auto& pruned_struct_fields = pruned_struct_value.get<TYPE_STRUCT>();
+ ASSERT_EQ(pruned_struct_fields.size(), 1);
+ EXPECT_EQ(pruned_struct_fields[0].get<TYPE_INT>(), 7);
+
+ const auto reordered_struct_type = make_nullable(
+ std::make_shared<DataTypeStruct>(DataTypes {optional_string_type,
required_int_type},
+ Strings {"optional_added",
"legacy_required_added"}));
+ ColumnPtr reordered_struct_column;
+ ASSERT_TRUE(iceberg::create_initial_default_column(
+ *struct_field.field_ptr, reordered_struct_type,
&reordered_struct_column)
+ .ok());
+ Field reordered_struct_value;
+ reordered_struct_column->get(0, reordered_struct_value);
+ const auto& reordered_struct_fields =
reordered_struct_value.get<TYPE_STRUCT>();
+ ASSERT_EQ(reordered_struct_fields.size(), 2);
+ EXPECT_TRUE(reordered_struct_fields[0].is_null());
+ EXPECT_EQ(reordered_struct_fields[1].get<TYPE_INT>(), 7);
+
+ const auto list_type =
make_nullable(std::make_shared<DataTypeArray>(optional_string_type));
+ auto list_element = make_field("element", 5, true);
+ schema::external::TArrayField list_metadata;
+ list_metadata.__set_item_field(list_element);
+ auto list_field = make_field("list_default", 4, true, "[\"alpha\",null]");
+ list_field.field_ptr->nestedField.__set_array_field(list_metadata);
+ list_field.field_ptr->__isset.nestedField = true;
+
+ ColumnPtr list_column;
+ ASSERT_TRUE(
+ iceberg::create_initial_default_column(*list_field.field_ptr,
list_type, &list_column)
+ .ok());
+ Field list_value;
+ list_column->get(0, list_value);
+ const auto& list_fields = list_value.get<TYPE_ARRAY>();
+ ASSERT_EQ(list_fields.size(), 2);
+ EXPECT_EQ(list_fields[0].get<TYPE_STRING>(), "alpha");
+ EXPECT_TRUE(list_fields[1].is_null());
+
+ const auto map_type = make_nullable(std::make_shared<DataTypeMap>(
+ std::make_shared<DataTypeString>(),
std::make_shared<DataTypeInt32>()));
+ const std::string binary_key_hex = "00112233445566778899aabbccddeeff0011";
Review Comment:
[P1] Keep the binary fixture from failing the secret scan
This added 36-character hex literal is currently detected by the required
**Check for secrets** workflow as a generic API key; the identical fixture in
the V2 test is reported separately, so run 29818896122 exits with `leaks found:
2`. These are synthetic values, but the required check remains red until both
are represented in a form the scanner does not classify as credentials (or
covered by the project's accepted test-fixture allowlist). Please adjust both
copies so the PR can pass its blocking checks.
--
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]
