github-actions[bot] commented on code in PR #65253:
URL: https://github.com/apache/doris/pull/65253#discussion_r3601856418
##########
be/src/format/transformer/vorc_transformer.cpp:
##########
@@ -376,48 +408,20 @@ Status
VOrcTransformer::collect_file_statistics_after_close(TIcebergColumnStats*
return Status::OK();
}
+ 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;
- bool has_any_null_count = false;
- bool has_any_min_max = false;
-
- const iceberg::StructType& root_struct =
_iceberg_schema->root_struct();
- const auto& nested_fields = root_struct.fields();
- for (uint32_t i = 0; i < nested_fields.size(); i++) {
- uint32_t orc_col_id = i + 1; // skip root struct
- if (orc_col_id >= file_stats->getNumberOfColumns()) {
- continue;
- }
- const orc::ColumnStatistics* col_stats =
file_stats->getColumnStatistics(orc_col_id);
- if (col_stats == nullptr) {
- continue;
- }
-
- int32_t field_id = nested_fields[i].field_id();
- int64_t non_null_count = col_stats->getNumberOfValues();
- value_counts[field_id] = non_null_count;
- if (col_stats->hasNull()) {
- has_any_null_count = true;
- int64_t null_count = _cur_written_rows - non_null_count;
- null_value_counts[field_id] = null_count;
- value_counts[field_id] += null_count;
- }
-
- if (_collect_column_bounds(col_stats, field_id,
-
_output_vexpr_ctxs[i]->root()->data_type(), &lower_bounds,
- &upper_bounds)) {
- has_any_min_max = true;
- }
- }
+ RETURN_IF_ERROR(_collect_iceberg_metrics(reader.get(),
file_stats.get(), &column_sizes,
Review Comment:
This new traversal lazily reads every stripe footer, and ORC's
`getNumberOfStreams()` / `getStreamInformation()` can throw on I/O or a
malformed footer. The surrounding catch logs any `std::exception` and returns
OK, so `_build_iceberg_commit_data` commits the file with an empty stats
object; for a `ParseError`, that can commit a corrupt ORC file that only fails
on later scans. Please convert these exceptions to a non-OK `Status` so
close/commit aborts, and cover a stripe-footer read/parse failure.
##########
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;
Review Comment:
ORC child stats here can omit nulls inherited from a nullable parent struct.
For `IF(cond, NULL, NAMED_STRUCT('c', 42))`, the outer struct map marks the row
null while the child payload remains present; sink coercion does not merge that
ancestor map, `DataTypeStructSerDe` drops it, and ORC counts only the
child-local mask. This therefore emits `null_value_counts[c] = 0` even though
`c` is null through its parent. Iceberg can use that zero to prune `c IS NULL`,
skipping this file. Please fold ancestor nulls into eligible descendant
statistics (or omit those child metrics) and add an ORC regression with a null
optional parent and required primitive child.
--
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]
