github-actions[bot] commented on code in PR #65784:
URL: https://github.com/apache/doris/pull/65784#discussion_r3610150295
##########
be/src/format_v2/table_reader.cpp:
##########
@@ -488,6 +490,7 @@ Status TableReader::annotate_projected_column(const
TFileScanSlotInfo& slot_info
context->schema_column =
build_schema_column_from_external_field(*schema_field, column->type);
column->identifier = context->schema_column->identifier;
column->name_mapping = context->schema_column->name_mapping;
+ column->has_name_mapping = context->schema_column->has_name_mapping;
Review Comment:
[P1] Preserve binary initial-default metadata on projected roots
`build_schema_column_from_external_field` has the payload and Base64 marker,
but this annotation copies only ID/name metadata. The projected column already
has a generic FE `default_expr` built from the Base64 carrier; when the
physical field is absent, `_create_mapping_for_column` takes that expression
before the new decoding branch and returns text such as `AAEC/w==` instead of
the binary bytes. V1's projected fill has the same generic-expression behavior;
the new format helpers only cover nested children. This is separate from the
existing thread at this anchor, which covered the mapping-presence bit rather
than default metadata. Please propagate the initial-default metadata, give it
precedence for Iceberg missing fields, and add top-level BINARY/FIXED/UUID
result tests.
##########
be/src/format_v2/column_mapper.cpp:
##########
@@ -52,11 +52,59 @@
#include "format_v2/schema_projection.h"
#include "format_v2/table_reader.h"
#include "gen_cpp/Exprs_types.h"
+#include "util/url_coding.h"
namespace doris::format {
namespace {
+Status parse_initial_default(const ColumnDefinition& column,
std::optional<Field>* value) {
+ DORIS_CHECK(value != nullptr);
+ value->reset();
+ if (!column.initial_default_value.has_value()) {
+ return Status::OK();
+ }
+ const auto nested_type = remove_nullable(column.type);
+ Field parsed;
+ if (column.initial_default_value_is_base64 ||
+ nested_type->get_primitive_type() == TYPE_VARBINARY) {
+ std::string decoded;
+ if (!base64_decode(*column.initial_default_value, &decoded)) {
+ return Status::InvalidArgument("Invalid Base64 Iceberg initial
default for field {}",
+ column.name);
+ }
+ parsed = nested_type->get_primitive_type() == TYPE_VARBINARY
+ ?
Field::create_field<TYPE_VARBINARY>(StringView(decoded))
Review Comment:
[P1] Keep decoded VARBINARY defaults alive until materialization
`Field::create_field<TYPE_VARBINARY>(StringView(decoded))` does not own
payloads longer than 12 bytes: `StringView` stores only a pointer. `decoded` is
destroyed at the end of this block, before V2 builds the `VLiteral`; the
parallel Parquet and ORC helpers also destroy it before `create_column_const`.
A UUID default is always 16 bytes, so this can read freed storage and return
corrupted bytes or trip ASan. The new tests use `DataTypeString` and four
bytes, so they miss the branch. The V1/V2 equality-delete default helpers
contain the same borrowed-view lifetime. Please materialize into owned
VARBINARY storage at every one of these boundaries and add UUID/long FIXED
coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalUtil.java:
##########
@@ -199,7 +213,8 @@ private static TField getExternalSchema(Type columnType,
Column dorisColumn,
TFieldPtr fieldPtr = new TFieldPtr();
Column subColumn = subNameToSubColumn.get(subField.getName());
fieldPtr.setFieldPtr(getExternalSchema(
Review Comment:
[P1] Serialize non-binary defaults for nested fields
This recursive call cannot carry an `initialDefault` for an INT/STRING/DATE
child: `IcebergUtils.parseSchema` assigns the serialized default only to
top-level `Column`s, `createChildrenColumn` creates children without defaults,
and `updateIcebergColumnUniqueId` copies only IDs. The separate map repairs
UUID/BINARY/FIXED only. Consequently, when an older file lacks `s.added_b` with
initial default `7`, both new missing-child paths receive no value and
materialize NULL/type-default instead of `7`. This is separate from
r3609650017's binary Base64 transport. Please propagate every nested Iceberg
initial default and add FE-to-V1/V2 result coverage for a non-binary child.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -532,13 +534,14 @@ private List<String> getOrderedPathPartitionKeys() {
public void createScanRangeLocations() throws UserException {
super.createScanRangeLocations();
// Extract name mapping from Iceberg table properties
- Map<Integer, List<String>> nameMapping = extractNameMapping();
+ Optional<Map<Integer, List<String>>> nameMapping =
extractNameMapping();
// 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());
+ nameMapping.orElse(Collections.emptyMap()),
nameMapping.isPresent(),
+ getBase64EncodedInitialDefaultsForScan());
Review Comment:
[P1] Use the current schema for ordinary-scan default metadata
For an ordinary scan, `createTableScan().snapshot()` is the current data
snapshot, whose schema ID can remain old after a schema-only update, while
`source.getTargetTable().getColumns()` is deliberately built from
`table.schema()`. A nested BINARY/UUID/FIXED field added after that snapshot is
therefore absent from `getBase64EncodedInitialDefaultsForScan()`—the child's
only default transport—and both new missing-child paths emit NULL/type-default.
This is a distinct residual from r3609650017: that fix cannot preserve a field
omitted before the map is built. Use the selected snapshot schema only for
explicit time travel/ref scans; otherwise use the current schema that produced
the target Columns.
##########
be/src/format_v2/column_mapper.cpp:
##########
@@ -2084,7 +2145,19 @@ const ColumnDefinition*
TableColumnMapper::_find_file_field(
});
return field_it == file_schema.end() ? nullptr : &*field_it;
}
- return matcher_for_mode(_options.mode).find(table_column, file_schema);
+ const auto* matched = matcher_for_mode(_options.mode).find(table_column,
file_schema);
+ if (matched != nullptr || _options.mode !=
TableColumnMappingMode::BY_FIELD_ID ||
+ !_options.allow_idless_complex_wrapper_projection ||
table_column.children.empty()) {
+ return matched;
+ }
+ const auto* wrapper = find_column_by_name(table_column, file_schema);
+ if (wrapper == nullptr || wrapper->has_identifier_field_id() ||
wrapper->children.empty() ||
+ !has_shared_descendant_field_id(table_column, *wrapper)) {
+ return nullptr;
+ }
+ // Iceberg Parquet's PruneColumns retains an ID-less complex wrapper when
a nested field ID is
Review Comment:
[P1] Apply the Parquet wrapper fallback recursively
This fallback only runs for a root passed to `_find_file_field`. Once
`outer#10` matches, `_create_direct_mapping` still resolves its `inner#20`
child through the strict field-ID matcher, so a physical `outer#10 -> inner(no
id) -> leaf#30` is marked missing without ever reaching `leaf#30`. Iceberg's
Parquet projection retains that ID-less wrapper because the selected descendant
survives. This is distinct from r3609650016's root-wrapper case: here the root
already matched by ID and the failure is in the recursive child matcher. Please
thread the Parquet wrapper capability through recursive struct matching and add
a two-level reader-result test.
--
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]
