github-actions[bot] commented on code in PR #65446:
URL: https://github.com/apache/doris/pull/65446#discussion_r3567544898
##########
be/src/format_v2/table_reader.cpp:
##########
@@ -674,10 +674,14 @@ Status
TableReader::create_file_reader(std::unique_ptr<FileReader>* reader) {
const bool enable_mapping_timestamp_tz = _scan_params != nullptr &&
_scan_params->__isset.enable_mapping_timestamp_tz &&
_scan_params->enable_mapping_timestamp_tz;
+ const std::string hive_parquet_time_zone =
+ _scan_params != nullptr &&
_scan_params->__isset.hive_parquet_time_zone
+ ? _scan_params->hive_parquet_time_zone
+ : "";
if (_format == FileFormat::PARQUET) {
*reader = std::make_unique<format::parquet::ParquetReader>(
_system_properties, _current_task->data_file, _io_ctx,
_scanner_profile,
- _global_rowid_context, enable_mapping_timestamp_tz);
Review Comment:
The new scan parameter is still not applied to Iceberg equality-delete
Parquet files. Normal data-file reads go through this
`TableReader::create_file_reader()` path and pass `hive_parquet_time_zone` into
`ParquetReader`, but `IcebergTableReader::_read_equality_delete_file()` copies
the same `_scan_params` and then `_create_delete_file_reader()` constructs its
Parquet reader with only `system_properties, file_description, io_ctx,
_scanner_profile`, leaving the new timezone argument at the default empty
string. Since `FileQueryScanNode` now serializes this property for HMS-backed
Iceberg tables, an HMS Iceberg table with
`hive.parquet.time-zone=Asia/Shanghai` can decode INT96 data-file values with
the catalog timezone while an INT96 timestamp equality-delete key is decoded as
raw/UTC. The delete predicate is then built from a different DATETIMEV2 value
and can fail to remove matching rows. Please pass the same
`hive_parquet_time_zone` into Parquet delete-file readers, and add cover
age for an Iceberg equality delete on an INT96 timestamp key.
##########
be/src/format_v2/orc/orc_reader.cpp:
##########
@@ -503,7 +503,15 @@ DateV2Value<DateTimeV2ValueType>
datetime_v2_from_orc_millis(int64_t millis, int
millis_remainder += 1000;
}
const auto extra_nanos = std::max<int32_t>(nanos_tail, 0);
- const auto microseconds = cast_set<uint64_t>(millis_remainder * 1000 +
extra_nanos / 1000);
+ constexpr int64_t NANOS_PER_MICROSECOND = 1000;
+ constexpr int64_t MICROS_PER_SECOND = 1000000;
+ // Stripe statistics split the timestamp into milliseconds and the
remaining nanoseconds. Use
+ // the same half-up rule as row decoding so zone-map pruning observes
identical values.
+ const auto rounded_extra_microseconds =
+ (extra_nanos + NANOS_PER_MICROSECOND / 2) / NANOS_PER_MICROSECOND;
Review Comment:
This stats conversion still feeds ORC `MIN/MAX` aggregate pushdown, so named
timezones with DST folds can return the wrong aggregate value.
`set_timestamp_zone_map()` stores only `datetime_v2_from_orc_millis(raw_min,
timezone)` and `datetime_v2_from_orc_millis(raw_max, timezone)`, and
`OrcReader::get_aggregate_result(MINMAX)` later materializes those two stats
values as the rows consumed by the upper aggregate. In a zone such as
`America/Los_Angeles`, a stripe spanning `2021-11-07 08:30:00Z..09:00:00Z`
decodes row-by-row as local `01:30..01:59` before the fall-back and then
`01:00` after it, but endpoint conversion records `min=01:30` and `max=01:00`;
the pushed-down rows make `MAX(timestamp_col)` return `01:30` even though
`01:59` exists in the stripe. The new SARG code already disables timestamp
SARGs for non-fixed-offset zones, but this aggregate stats path has no similar
guard. Please disable ORC timestamp MIN/MAX aggregate pushdown for
non-fixed-offset `DATETIMEV2` zones, or
compute transition-aware local min/max bounds before returning stats.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/FileQueryScanNode.java:
##########
@@ -677,6 +682,17 @@ protected boolean getEnableMappingTimestampTz() {
return false;
}
+ protected String getHiveParquetTimeZone() throws UserException {
+ TableIf table = getTargetTable();
+ if (table instanceof ExternalTable) {
+ CatalogIf<?> catalog = ((ExternalTable) table).getCatalog();
+ if (catalog instanceof HMSExternalCatalog) {
+ return ((HMSExternalCatalog) catalog).getHiveParquetTimeZone();
Review Comment:
This makes the new INT96 compatibility setting depend on the Doris catalog
wrapper rather than on the Iceberg scan itself. An HMS table whose `DLAType` is
`ICEBERG` is planned as `IcebergScanNode` and reaches this branch with an
`HMSExternalCatalog`, so it serializes `hive.parquet.time-zone`; the same
Iceberg table reached through `type='iceberg', iceberg.catalog.type='hms'` is
also planned as `IcebergScanNode`, but its catalog is
`IcebergHMSExternalCatalog`, so this method returns an empty string and BE
decodes INT96 values with the default/raw semantics. Since the BE Iceberg
data-file readers now consume this field when it is present, the same
HMS-backed Iceberg Parquet data can produce different timestamp values solely
based on which catalog type the user chose. Please either restrict this
property to Hive-only tables instead of all HMS-backed DLA formats, or
expose/canonicalize the same setting for Iceberg HMS catalogs and add coverage
that reads the same INT96 Iceberg table t
hrough both catalog types.
--
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]
