github-actions[bot] commented on code in PR #65446:
URL: https://github.com/apache/doris/pull/65446#discussion_r3568045499
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/FileQueryScanNode.java:
##########
@@ -677,6 +683,23 @@ protected boolean getEnableMappingTimestampTz() {
return false;
}
+ protected String getHiveParquetTimeZone() throws UserException {
+ TableIf table = getTargetTable();
+ if (table instanceof HMSExternalTable
+ && ((HMSExternalTable) table).getDlaType() ==
HMSExternalTable.DLAType.HIVE) {
+ CatalogIf<?> catalog = ((HMSExternalTable) table).getCatalog();
+ if (catalog instanceof HMSExternalCatalog) {
+ return ((HMSExternalCatalog) catalog).getHiveParquetTimeZone();
+ }
+ }
+ if (table instanceof FunctionGenTable) {
+ FunctionGenTable functionGenTable = (FunctionGenTable) table;
+ ExternalFileTableValuedFunction tvf =
(ExternalFileTableValuedFunction) functionGenTable.getTvf();
+ return tvf.getHiveParquetTimeZone();
Review Comment:
`file()` TVFs still lose this property here. For `file()`,
`FunctionGenTable` stores the `FileTableValuedFunction` wrapper, while the
constructor skips `parseCommonProperties()` and creates a storage-specific
`delegateTvf` that actually parses `hive.parquet.time-zone`. `getScanNode()`
delegates to that storage TVF, but the descriptor still points at the wrapper,
so this line reads the wrapper's empty inherited `hiveParquetTimeZone` and
never sets the Thrift field. A query such as `file(..., "format"="parquet",
"hive.parquet.time-zone"="Asia/Shanghai")` will therefore use the new default
raw INT96 wall-clock path instead of the requested Hive-compatible conversion.
Please delegate this getter from `FileTableValuedFunction` to `delegateTvf` and
add a `file()` FE test for the scan param.
##########
be/src/format_v2/orc/orc_reader.cpp:
##########
@@ -2021,6 +2029,19 @@ Status OrcReader::get_aggregate_result(const
format::FileAggregateRequest& reque
RETURN_IF_ERROR(find_projected_minmax_leaf(*_state->root_type,
request_column.projection,
&leaf_type));
DORIS_CHECK(leaf_type != nullptr);
+ const bool is_datetime_timestamp =
+ leaf_type->getKind() == ::orc::TypeKind::TIMESTAMP ||
+ (leaf_type->getKind() == ::orc::TypeKind::TIMESTAMP_INSTANT &&
+ !_enable_mapping_timestamp_tz);
+ if (is_datetime_timestamp) {
Review Comment:
This guard only rejects named time zones, but the same PR now treats
negative ORC timestamp statistics as unsafe for SARG pruning.
`make_comparison_literal_for_sarg()` rejects `timestamp.second < 0` because ORC
can reconstruct negative timestamp stats with a non-canonical nanos component
and then leaves those predicates to row filtering. Aggregate pushdown does not
have that fallback: after this fixed-offset check, `set_timestamp_zone_map()`
returns the statistic directly, and `datetime_v2_from_orc_millis()` even clamps
a negative `nanos_tail` to zero before materializing the value. A fixed-offset
`MIN/MAX(timestamp_col)` over a pre-epoch stripe can therefore return a value
that does not match row decoding. Please also disable or canonicalize timestamp
aggregate pushdown for negative/epoch stats, and add a unit test using the new
negative timestamp fixture.
--
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]
