github-actions[bot] commented on code in PR #68934:
URL: https://github.com/apache/doris/pull/68934#discussion_r4254681379
##########
be/src/format_v2/orc/orc_reader.cpp:
##########
@@ -1646,24 +1715,91 @@ Status OrcReader::_select_stripe_ranges_by_statistics()
{
return Status::OK();
}
-void OrcReader::_apply_current_stripe_range() {
- if (!_state->stripe_pruning_applied ||
_state->selected_stripe_ranges.empty()) {
- return;
+Status OrcReader::_prepare_timestamp_stripe_ranges() {
+ if (!_state->match_writer_timezone) {
+ return Status::OK();
+ }
+ std::vector<uint64_t> stripes;
+ if (_state->stripe_ranges_applied) {
+ for (const auto& range : _state->selected_stripe_ranges) {
+ for (auto stripe = range.first_stripe; stripe < range.last_stripe;
++stripe) {
+ stripes.push_back(stripe);
+ }
+ }
+ } else {
+ RETURN_IF_ERROR(_collect_split_stripes(&stripes));
+ }
+ _state->selected_stripe_ranges.clear();
+ _state->stripe_ranges_applied = true;
+ _reader_statistics.read_row_groups = cast_set<int32_t>(stripes.size());
+ if (stripes.empty()) {
+ return Status::OK();
+ }
+ try {
+ uint64_t first_row = 0;
+ size_t selected = 0;
+ for (uint64_t index = 0; index <= stripes.back(); ++index) {
+ const auto stripe = _state->reader->getStripe(index);
+ const auto end_row = first_row + stripe->getNumberOfRows();
+ if (index == stripes[selected]) {
+ // A RowReader has one timezone, whereas writer timezones
belong to stripes.
+ // Read footer timezone metadata only when entering the
selected stripe.
+ _state->selected_stripe_ranges.push_back({
+ .first_stripe = index,
+ .last_stripe = index + 1,
+ .offset = stripe->getOffset(),
+ .length = stripe->getLength(),
+ .first_row = first_row,
+ .end_row = end_row,
+ });
+ ++selected;
+ }
+ first_row = end_row;
+ }
+ } catch (const std::exception& e) {
+ return Status::InternalError("Failed to prepare ORC timestamp stripe
ranges: {}", e.what());
+ }
+ return Status::OK();
+}
+
+Status OrcReader::_apply_current_stripe_range() {
+ if (!_state->stripe_ranges_applied ||
_state->selected_stripe_ranges.empty()) {
+ return Status::OK();
}
DORIS_CHECK(_state->current_stripe_range <
_state->selected_stripe_ranges.size());
const auto& stripe_range =
_state->selected_stripe_ranges[_state->current_stripe_range];
_state->row_reader_options.range(stripe_range.offset, stripe_range.length);
+ if (_state->match_writer_timezone) {
+ try {
+ const auto stripe =
_state->reader->getStripe(stripe_range.first_stripe);
+ const auto& name = stripe->getWriterTimezone();
+ const auto writer_timezone = name.empty() ? "GMT" : name;
+ if (!TimezoneUtils::find_cctz_time_zone(writer_timezone,
_state->writer_timezone_obj)) {
+ return Status::InvalidArgument("Unknown ORC stripe writer
timezone: {}",
+ writer_timezone);
+ }
+ // Matching zones preserves the SDK's UTC carrier until its
negative-fraction
+ // correction is complete. cctz then applies historical offsets to
that original second.
+ _state->row_reader_options.setTimezoneName(writer_timezone);
Review Comment:
[P2] Keep ORC MIN/MAX consistent with the repaired row timestamps. With a
C++ ORC New York file containing an ordinary timestamp at source second
`-5364644638` and fraction `123456789`, plus a hidden NANOS sibling, this
matching-zone path now correctly returns `1800-01-01 00:00:00.123457`. However,
[the pinned writer's
statistics](https://github.com/apache/doris-thirdparty/blob/5860f94768b94406f31698ac66f4cc3d2a781324/c%2B%2B/src/ColumnWriter.cc#L1713-L1722)
use ORC's ancient EST offset, and `get_aggregate_result` still interprets
those bounds in UTC: pushed `MIN(plain)`/`MAX(plain)` return `1799-12-31
23:56:02.123457`. The nonzero nanos tail passes the sentinel fallback. This is
a distinct metadata-aggregate path from the earlier row-carrier thread;
negative timestamp predicates already remain residual. Reconcile these ordinary
timestamp statistics with the selected producer/carrier semantics or fall back
to row aggregation, and add a pushdown-enabled/disabled differential test.
##########
be/src/core/data_type_serde/data_type_datetimev2_serde.cpp:
##########
@@ -1049,7 +1050,11 @@ Status
DataTypeDateTimeV2SerDe::read_column_from_orc(IColumn& column,
if (orc_serde_utils::orc_decode_row_count(view.rows, view.selected_rows)
== 0) {
return Status::OK();
}
- return decode_timestamp_orc_values(column, view, *view.timezone);
+ const auto* timezone = view.file_type->getKind() == ::orc::TIMESTAMP &&
+ view.plain_timestamp_timezone !=
nullptr
+ ? view.plain_timestamp_timezone
Review Comment:
[P2] Preserve ancient timestamps produced by the Java ORC writer. A
supported non-UTC Java ORC 1.9 writer in `America/New_York` stores
`Timestamp.valueOf("1800-01-01 00:00:00")` as second `-5364644400`: [Java's
historical timezone carrier uses raw
EST](https://github.com/openjdk/jdk17u/blob/master/src/java.base/share/classes/sun/util/calendar/ZoneInfo.java#L250-L261).
With a hidden NANOS-annotated sibling, the new matching-zone reader preserves
that second, and this branch applies cctz's UTC-04:56:02 LMT offset, returning
`1800-01-01 00:03:58`. The prior UTC-session reader returned midnight. The
[Java ORC
writer](https://github.com/apache/orc/blob/rel/release-1.9.0/java/core/src/java/org/apache/orc/impl/writer/TimestampTreeWriter.java#L128-L141)
and public Iceberg schema converter support this mixed file. The existing C++
ancient fixture uses a different carrier, so fixing its earlier thread does not
cover this regression. Preserve both producer representations and add a
Java-prod
uced fixture read in a UTC session.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergPartitionUtils.java:
##########
@@ -801,6 +828,11 @@ static boolean isValidRelatedTable(Table table) {
return false;
}
PartitionField partitionField = fields.get(0);
+ if (spec.schema().findType(partitionField.sourceId()).typeId() ==
Type.TypeID.TIMESTAMP_NANO) {
Review Comment:
[P2] Handle dropped partition sources before inspecting their type. A valid
v1 table can remove its sole `day(ts)` partition field, leaving `void(ts)`, and
then drop `ts`. [Iceberg rebuilds historical specs against the new
schema](https://github.com/apache/iceberg/blob/apache-iceberg-1.11.0/core/src/main/java/org/apache/iceberg/TableMetadata.java#L756-L768),
preserving the removed source ID, so `findType(sourceId)` is null in both the
old calendar spec and current void spec. This new dereference runs before
rejecting non-calendar transforms. `IcebergConnectorMetadata.latestSnapshotPin`
calls it unconditionally during `beginQuerySnapshot`, so even a normal SELECT
of a surviving column now fails with a wrapped null-pointer error; the previous
code returned an unpartitioned view. Treat a missing source type as ineligible
for RANGE partitioning, and cover a real v1 remove-partition/drop-source
lifecycle through snapshot planning.
##########
be/src/format_v2/orc/orc_reader.cpp:
##########
@@ -1646,24 +1715,91 @@ Status OrcReader::_select_stripe_ranges_by_statistics()
{
return Status::OK();
}
-void OrcReader::_apply_current_stripe_range() {
- if (!_state->stripe_pruning_applied ||
_state->selected_stripe_ranges.empty()) {
- return;
+Status OrcReader::_prepare_timestamp_stripe_ranges() {
+ if (!_state->match_writer_timezone) {
+ return Status::OK();
+ }
+ std::vector<uint64_t> stripes;
+ if (_state->stripe_ranges_applied) {
+ for (const auto& range : _state->selected_stripe_ranges) {
+ for (auto stripe = range.first_stripe; stripe < range.last_stripe;
++stripe) {
+ stripes.push_back(stripe);
+ }
+ }
+ } else {
+ RETURN_IF_ERROR(_collect_split_stripes(&stripes));
+ }
+ _state->selected_stripe_ranges.clear();
+ _state->stripe_ranges_applied = true;
+ _reader_statistics.read_row_groups = cast_set<int32_t>(stripes.size());
+ if (stripes.empty()) {
+ return Status::OK();
+ }
+ try {
+ uint64_t first_row = 0;
+ size_t selected = 0;
+ for (uint64_t index = 0; index <= stripes.back(); ++index) {
+ const auto stripe = _state->reader->getStripe(index);
+ const auto end_row = first_row + stripe->getNumberOfRows();
+ if (index == stripes[selected]) {
+ // A RowReader has one timezone, whereas writer timezones
belong to stripes.
+ // Read footer timezone metadata only when entering the
selected stripe.
+ _state->selected_stripe_ranges.push_back({
+ .first_stripe = index,
+ .last_stripe = index + 1,
Review Comment:
[P2] Reuse row readers for adjacent stripes with the same timezone. Every
selected stripe becomes its own range here, including unfiltered all-GMT nanos
files, and each range transition constructs a new `RowReader`. The [pinned
constructor](https://github.com/apache/doris-thirdparty/blob/5860f94768b94406f31698ac66f4cc3d2a781324/c%2B%2B/src/Reader.cc#L263-L307)
allocates `firstRowOfStripe` for the entire file and copies/scans every footer
stripe even for a one-stripe range. For N small flushed stripes in one split
this changes O(N) setup to O(N²), plus repeated schema and batch
initialization; projecting only a plain timestamp beside a hidden NANOS field
also triggers it. Coalesce adjacent selected stripes with identical writer
zones or retain reusable SDK setup, and cover many-stripe files while
preserving different-zone correctness.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorTransaction.java:
##########
@@ -929,14 +930,19 @@ private Expression buildPartitionFilter(Map<String,
String> staticPartitions, Pa
* both paths that build this predicate: DELETE/UPDATE/MERGE conflict
detection and
* {@code INSERT OVERWRITE ... PARTITION(d='nan')}.
*/
- private static Expression identityPartitionPredicate(String sourceColName,
Object value) {
+ private static Expression identityPartitionPredicate(Types.NestedField
sourceField, Object value) {
+ String sourceColName = sourceField.name();
if (value == null) {
return Expressions.isNull(sourceColName);
}
if ((value instanceof Double || value instanceof Float)
&& Double.isNaN(((Number) value).doubleValue())) {
return Expressions.isNaN(sourceColName);
}
+ if (sourceField.type().typeId() == TypeID.TIMESTAMP_NANO) {
+ // Partition values are already nanos; a raw Long would be rebound
as microseconds.
+ return Expressions.equal(sourceColName, Expressions.nanos((Long)
value));
Review Comment:
[P1] Guard generated overwrite predicates against retained DATE bounds. A v3
table can retain an unpartitioned DATE file containing `2024-01-02`, promote
that field to `timestamp_ns`, then add an identity partition. `INSERT OVERWRITE
... PARTITION(ts='2024-01-02')` reaches this direct nanos equality without
`forTable`'s history safeguard. Iceberg 1.11 interprets the retained four-byte
bounds as the unscaled long `19724`, so [its deletion
evaluator](https://github.com/apache/iceberg/blob/apache-iceberg-1.11.0/core/src/main/java/org/apache/iceberg/ManifestFilterManager.java#L580-L620)
excludes the old file from deletion while the replacement is committed; the
overwritten rows remain. Generated identity conflict filters use the same
helper. This is a separate commit-time instance from the earlier user-predicate
pruning thread. Reject unsafe static overwrites or safely promote old bounds;
conflict-only filters can conservatively omit the unsafe conjunct. Cover
retained files from the
previous unpartitioned spec.
--
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]
