github-actions[bot] commented on code in PR #65253:
URL: https://github.com/apache/doris/pull/65253#discussion_r3610310601
##########
be/src/format/transformer/vorc_transformer.cpp:
##########
@@ -428,18 +432,71 @@ Status
VOrcTransformer::collect_file_statistics_after_close(TIcebergColumnStats*
}
}
-bool VOrcTransformer::_collect_column_bounds(const orc::ColumnStatistics*
col_stats,
- int32_t field_id, const
DataTypePtr& data_type,
- std::map<int32_t, std::string>*
lower_bounds,
- std::map<int32_t, std::string>*
upper_bounds) {
- bool has_bounds = false;
- auto primitive_type = remove_nullable(data_type)->get_primitive_type();
+Status VOrcTransformer::_collect_iceberg_metrics(
+ const orc::Reader* reader, const orc::Statistics* file_stats,
+ std::map<int32_t, int64_t>* column_sizes, std::map<int32_t, int64_t>*
value_counts,
+ std::map<int32_t, int64_t>* null_value_counts, std::map<int32_t,
std::string>* lower_bounds,
+ std::map<int32_t, std::string>* upper_bounds) const {
+ std::map<uint64_t, int32_t> orc_to_iceberg_field_ids;
+ for (uint32_t orc_col_id = 1; orc_col_id <
file_stats->getNumberOfColumns(); ++orc_col_id) {
+ const orc::Type* orc_type = _schema->getTypeByColumnId(orc_col_id);
+ if (!orc_type->hasAttributeKey(ORC_ICEBERG_ID_KEY)) {
+ continue;
+ }
+
+ int32_t field_id =
std::stoi(orc_type->getAttributeValue(ORC_ICEBERG_ID_KEY));
+ iceberg::Type* field_type = _iceberg_schema->find_type(field_id);
+ if (field_type == nullptr) {
+ return Status::InternalError("Can not find Iceberg field for ORC
metrics: {}",
+ field_id);
+ }
+ if (_iceberg_schema->is_nested_in_list_or_map(field_id)) {
+ continue;
+ }
+
+ orc_to_iceberg_field_ids.emplace(orc_col_id, field_id);
+ (*column_sizes)[field_id] = 0;
+ const orc::ColumnStatistics* col_stats =
file_stats->getColumnStatistics(orc_col_id);
+ if (col_stats != nullptr) {
+ int64_t non_null_count =
cast_set<int64_t>(col_stats->getNumberOfValues());
+ DORIS_CHECK_GE(_cur_written_rows, non_null_count);
+ int64_t null_count = col_stats->hasNull() ? _cur_written_rows -
non_null_count : 0;
+ (*null_value_counts)[field_id] = null_count;
+ (*value_counts)[field_id] = non_null_count + null_count;
+ if (field_type->is_primitive_type()) {
+ _collect_primitive_column_bounds(col_stats, field_id,
field_type, lower_bounds,
+ upper_bounds);
+ }
+ }
+ }
+
+ for (uint64_t stripe_id = 0; stripe_id < reader->getNumberOfStripes();
++stripe_id) {
+ std::unique_ptr<orc::StripeInformation> stripe =
reader->getStripe(stripe_id);
+ for (uint64_t stream_id = 0; stream_id < stripe->getNumberOfStreams();
++stream_id) {
+ std::unique_ptr<orc::StreamInformation> stream =
+ stripe->getStreamInformation(stream_id);
Review Comment:
[P2] Avoid quadratic stream traversal on close. In the pinned ORC 1.9.0,
each `getStreamInformation(i)` recomputes its offset by summing all earlier
stream lengths. Calling it for every stream makes this new collector
O(streams^2) per stripe (plus one allocation per call), which is costly for
wide Iceberg schemas on the synchronous commit path. Please aggregate directly
from the stripe footer or provide a single-pass/bulk accessor.
##########
be/src/format/transformer/vorc_transformer.cpp:
##########
@@ -482,13 +546,11 @@ bool VOrcTransformer::_collect_column_bounds(const
orc::ColumnStatistics* col_st
} else if (const auto* string_stats =
dynamic_cast<const
orc::StringColumnStatistics*>(col_stats)) {
if (string_stats->hasMinimum() && string_stats->hasMaximum()) {
- has_bounds = true;
(*lower_bounds)[field_id] = string_stats->getMinimum();
Review Comment:
[P1] Encode nested UUID bounds as 16 bytes. With
`enable.mapping.varbinary=false`, UUID is written to ORC as its 36-byte STRING
form, and this newly recursive path copies that text directly into Iceberg
bounds. Iceberg UUID bounds are 16-byte values (the Parquet path explicitly
enforces width 16), so readers decode different metadata and may prune a file
incorrectly. Please convert canonical UUID text to the Iceberg binary
representation, or omit UUID bounds, and cover a UUID struct child with
varbinary mapping disabled.
##########
be/src/format/transformer/vorc_transformer.cpp:
##########
@@ -482,13 +546,11 @@ bool VOrcTransformer::_collect_column_bounds(const
orc::ColumnStatistics* col_st
} else if (const auto* string_stats =
dynamic_cast<const
orc::StringColumnStatistics*>(col_stats)) {
if (string_stats->hasMinimum() && string_stats->hasMaximum()) {
- has_bounds = true;
(*lower_bounds)[field_id] = string_stats->getMinimum();
(*upper_bounds)[field_id] = string_stats->getMaximum();
Review Comment:
[P1] Do not publish ORC's embedded-NUL string extrema. Doris preserves
embedded NUL bytes when writing, but pinned ORC 1.9 computes STRING min/max
with `strncmp`. If a nested child writes `\x00a` before `\x00z`, ORC leaves
`\x00a` as the maximum; this newly recursive copy therefore commits an upper
bound below an actual value and Iceberg can prune the file incorrectly. Please
compute length-aware extrema or conservatively omit these bounds, and add a
struct-string regression containing U+0000.
##########
be/src/format/transformer/vorc_transformer.cpp:
##########
@@ -165,6 +185,18 @@ Status VOrcTransformer::open() {
return Status::OK();
}
+const iceberg::NestedField* VOrcTransformer::_iceberg_field_for_column(
+ const std::string& column_name) const {
+ if (_iceberg_schema == nullptr) {
+ return nullptr;
+ }
+ const auto& fields = _iceberg_schema->root_struct().fields();
+ auto it = std::find_if(fields.begin(), fields.end(), [&](const
iceberg::NestedField& field) {
Review Comment:
[P2] Index root fields instead of rescanning them per column. `open()`
invokes this helper for every output name, and each call linearly scans all
root fields, so the normal N-column writer setup performs O(N^2) string
comparisons for every partition/file rollover. Please add a non-inserting
root-name index or pass the already resolved fields/IDs into the transformer.
--
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]
