github-actions[bot] commented on code in PR #65253:
URL: https://github.com/apache/doris/pull/65253#discussion_r3592114789
##########
be/src/format/transformer/vorc_transformer.cpp:
##########
@@ -451,19 +511,26 @@ bool VOrcTransformer::_collect_column_bounds(const
orc::ColumnStatistics* col_st
} else if (const auto* int_stats =
dynamic_cast<const
orc::IntegerColumnStatistics*>(col_stats)) {
if (int_stats->hasMinimum() && int_stats->hasMaximum()) {
- has_bounds = true;
- int64_t min_val = int_stats->getMinimum();
- int64_t max_val = int_stats->getMaximum();
- (*lower_bounds)[field_id] =
- std::string(reinterpret_cast<const char*>(&min_val),
sizeof(int64_t));
- (*upper_bounds)[field_id] =
- std::string(reinterpret_cast<const char*>(&max_val),
sizeof(int64_t));
+ if (primitive_type == iceberg::TypeID::LONG) {
+ int64_t min_val = int_stats->getMinimum();
+ int64_t max_val = int_stats->getMaximum();
+ (*lower_bounds)[field_id] =
+ std::string(reinterpret_cast<const char*>(&min_val),
sizeof(int64_t));
+ (*upper_bounds)[field_id] =
+ std::string(reinterpret_cast<const char*>(&max_val),
sizeof(int64_t));
+ } else {
+ int32_t min_val =
static_cast<int32_t>(int_stats->getMinimum());
+ int32_t max_val =
static_cast<int32_t>(int_stats->getMaximum());
+ (*lower_bounds)[field_id] =
+ std::string(reinterpret_cast<const char*>(&min_val),
sizeof(int32_t));
+ (*upper_bounds)[field_id] =
+ std::string(reinterpret_cast<const char*>(&max_val),
sizeof(int32_t));
+ }
}
} else if (const auto* double_stats =
dynamic_cast<const
orc::DoubleColumnStatistics*>(col_stats)) {
if (double_stats->hasMinimum() && double_stats->hasMaximum()) {
Review Comment:
Raw ORC floating extrema are not sufficient for Iceberg bounds. ORC can
report NaN, which Iceberg forbids in `lower_bounds` / `upper_bounds`;
additionally Doris's pinned ORC 1.9.0 updates min/max with ordinary `<`, so
`-0.0` and `+0.0` compare equal and whichever arrives first can remain both
endpoints, contrary to Iceberg's required `-0.0 < +0.0` order. Because this PR
now visits struct descendants, these invalid/unsafe bounds are newly published
for nested FLOAT/DOUBLE fields too. Please track Iceberg-compliant non-NaN
extrema while writing or conservatively normalize both cases, and cover NaN
plus both signed-zero input orders for top-level and struct-child fields.
##########
be/src/format/transformer/vorc_transformer.cpp:
##########
@@ -499,7 +564,6 @@ bool VOrcTransformer::_collect_column_bounds(const
orc::ColumnStatistics* col_st
} else if (const auto* ts_stats =
dynamic_cast<const
orc::TimestampColumnStatistics*>(col_stats)) {
if (ts_stats->hasMinimum() && ts_stats->hasMaximum()) {
- has_bounds = true;
int64_t min_val = ts_stats->getMinimum() * 1000;
int64_t max_val = ts_stats->getMaximum() * 1000;
Review Comment:
ORC exposes the sub-millisecond part separately through `getMinimumNanos()`
/ `getMaximumNanos()`; `getMinimum()` / `getMaximum()` alone are only
milliseconds. Doris's reader already combines both components in
`set_timestamp_zone_map`, but this writes only `getMaximum() * 1000`. For a
newly supported `struct<ts: datetime(6)>` whose maximum ends in `.123456`, the
committed upper bound ends in `.123000`, so Iceberg may prune the file for a
predicate above `.123000` even though the row is present. Please reconstruct
the full microsecond endpoints (or use an equally conservative ORC bound) and
cover nonzero microseconds, including a struct 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]
