github-actions[bot] commented on code in PR #65446:
URL: https://github.com/apache/doris/pull/65446#discussion_r3567688765
##########
be/src/format/parquet/vparquet_reader.cpp:
##########
@@ -1305,6 +1322,9 @@ Status ParquetReader::_process_page_index_filter(
const auto& page_locations = offset_index.page_locations;
sig_stat.col_schema = col_schema;
+ cached_page_index.ctz = col_schema->parquet_schema.type ==
tparquet::Type::INT96
Review Comment:
This wires INT96 page-index min/max decoding into the new Hive Parquet
timezone path, but the page-index path still accepts non-equal INT96 stats that
the row-group path rejects as unsafe. `read_column_stats()` rejects INT96
`min_value != max_value`, while this page-index branch stores raw
`ColumnIndex.min_values`/`max_values` and later calls `parse_min_max_value()`
directly; that helper's INT96 guard currently compares `min_field` to itself
instead of `max_field`, so non-equal INT96 page stats can build a zonemap and
remove matching pages before row filtering. Please disable INT96
page-index/expr-zonemap pruning unless the decoded min and max are equal, or
apply the same encoded-stat safety check before these page stats become
available.
##########
be/src/core/data_type_serde/data_type_serde.cpp:
##########
@@ -493,8 +511,10 @@ Status decode_timestamp_orc_values(IColumn& nested_column,
const OrcDecodedColum
}
auto& value =
reinterpret_cast<DateV2Value<DateTimeV2ValueType>&>(data[old_data_size + row]);
- value.from_unixtime(orc_batch->data[source_row], timezone);
-
value.set_microsecond(cast_set<uint64_t>(orc_batch->nanoseconds[source_row] /
1000));
+ const auto timestamp = round_orc_timestamp_to_microseconds(
Review Comment:
This fixes ORC timestamp rounding only in the scanner-v2 decode path, but
the same scan can still run through the legacy ORC reader when
`enable_file_scanner_v2=false`, in load mode, or for transactional Hive. That
path still materializes ORC `TIMESTAMP`/`TIMESTAMPTZ` with `nanoseconds /
1000`, so values such as `...999999500ns` continue to truncate instead of
carrying to the next second. It also keeps the old timestamp SearchArgument
construction in `format/orc/vorc_reader.cpp`, without the new named-timezone,
negative/epoch, and NOT/NE/NOT IN safety guards, so the earlier false-negative
pruning cases remain reachable through legacy scans. Please port the same
rounding and conservative timestamp SARG rules to
`be/src/format/orc/vorc_reader.*`, or disable legacy ORC timestamp
pushdown/force scanner v2 for these cases, and add coverage with
`enable_file_scanner_v2=false` or another legacy-reader path.
##########
fe/fe-core/src/test/java/org/apache/doris/datasource/hive/source/HiveScanNodeTest.java:
##########
@@ -97,6 +98,23 @@ public void
testSelectedPartitionsCarryPartitionPredicateFlag() {
Assert.assertTrue(selectedPartitions.hasPartitionPredicate);
}
+ @Test
+ public void testHiveParquetTimeZoneComesFromCatalogInsteadOfSession()
throws Exception {
+ SessionVariable sv = new SessionVariable();
+ TupleDescriptor desc = new TupleDescriptor(new TupleId(0));
+ HMSExternalTable table = Mockito.mock(HMSExternalTable.class);
+ HMSExternalCatalog catalog = Mockito.mock(HMSExternalCatalog.class);
+ Mockito.when(table.getCatalog()).thenReturn(catalog);
+ Mockito.when(catalog.bindBrokerName()).thenReturn("");
+
Mockito.when(catalog.getHiveParquetTimeZone()).thenReturn("Asia/Shanghai");
+ desc.setTable(table);
+ HiveScanNode node = new HiveScanNode(new PlanNodeId(0), desc, false,
sv, null, ScanContext.EMPTY);
+
+ Method method =
FileQueryScanNode.class.getDeclaredMethod("getHiveParquetTimeZone");
+ method.setAccessible(true);
+ Assert.assertEquals("Asia/Shanghai", method.invoke(node));
Review Comment:
This new test does not actually set up a Hive table for the helper it is
asserting. `getHiveParquetTimeZone()` now returns the catalog value only when
the target table is an `HMSExternalTable` with `DLAType.HIVE`, but this mock
only stubs `getCatalog()`. An unstubbed Mockito enum-returning method returns
`null`, so the helper falls through to `""` and this assertion fails instead of
proving the catalog property path. Please stub `table.getDlaType()` to
`HMSExternalTable.DLAType.HIVE` (or drive the assertion through the existing
scan-param fixture) before asserting the returned timezone.
--
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]
