github-actions[bot] commented on code in PR #66359:
URL: https://github.com/apache/doris/pull/66359#discussion_r3698564517
##########
be/src/format_v2/parquet/parquet_statistics.cpp:
##########
@@ -452,11 +452,11 @@ std::optional<format::LocalColumnId>
file_column_id_by_block_position(
return std::nullopt;
}
-bool has_expr_zonemap_filter(const format::FileScanRequest& request,
- const RuntimeState* runtime_state) {
- if (!expr_zonemap::is_expr_zonemap_filter_enabled(runtime_state)) {
- return false;
- }
+bool has_expr_zonemap_filter(const format::FileScanRequest& request, const
RuntimeState*) {
+ // FileScannerV2 metadata pruning is a fixed part of its scan pipeline and
must not inherit
+ // the legacy scanner's expression ZoneMap session gate.
+ // TODO: Fence metadata pruning at the first unsafe/error-preserving
conjunct so a later
Review Comment:
**[P1] Preserve the unsafe-expression barrier before making metadata pruning
unconditional**
`request.conjuncts` can contain an earlier error-preserving expression such
as `assert_true(c0 > 0, 'boom')`, followed by a ZoneMap-capable `c1 > 100`. For
a row that fails the assertion and a Row Group/Page whose `c1` bounds are
`[1,2]`, `VExprContext::evaluate_zonemap_filter()` skips the unsupported first
conjunct and prunes on the second. With this gate removed, FileScannerV2 now
returns successful EOF instead of the row-level error; the native Page Index
path has the same bypass. The row scheduler and partition pruner already treat
`is_safe_to_execute_on_selected_rows() == false` as an ordering barrier, and
the TODO here acknowledges that this metadata path does not. Please build the
same safe prefix and use it consistently for footer and Page evaluation, with
negative tests for an earlier unsafe conjunct (and the reverse order where an
earlier safe no-match may still prune).
##########
be/src/exec/scan/file_scanner_v2.cpp:
##########
@@ -642,7 +642,10 @@ Status FileScannerV2::_prepare_table_reader_split(const
TFileRangeDesc& range,
VExprContextSPtrs conjuncts;
RETURN_IF_ERROR(_build_table_conjuncts(&conjuncts));
VExprContextSPtrs partition_prune_conjuncts;
- if (_state->query_options().enable_runtime_filter_partition_prune) {
+ if (!partition_values.empty()) {
Review Comment:
**[P2] Avoid the second full clone when no partition-only safe-prefix
predicate is eligible**
A nonempty `partition_values` map only says the split is partitioned; it
does not mean the query has a predicate that can run on those constants. This
method already builds `conjuncts`, then this branch deep-clones, rewrites, and
runtime-filter-adapts every expression again. For a common partitioned scan
whose predicates reference only file columns (or whose first conjunct is an
unsafe ordering barrier), `TableReader::_evaluate_partition_prune_conjuncts()`
selects nothing and immediately discards that second tree. Because this runs
per split, many small files amplify allocation and rewrite cost even though
pruning is impossible. Please identify the safe partition-only prefix before
deep-cloning (and clone only it), with a test covering nonempty partition
metadata plus ineligible file-column/unsafe-first predicates.
##########
fe/be-java-extensions/paimon-connector/src/main/java/org/apache/doris/paimon/PaimonColumnValue.java:
##########
@@ -152,15 +179,18 @@ public LocalDateTime getDateTime() {
@Override
public LocalDateTime getTimeStampTz() {
Timestamp ts = record.getTimestamp(idx, dorisType.getPrecision());
- LocalDateTime v = ts.toInstant()
- .atZone(ZoneId.of("UTC"))
- .toLocalDateTime();
- return v;
+ // Timestamp's local representation is identical to converting its
epoch instant in UTC.
+ return ts.toLocalDateTime();
}
@Override
public boolean isNull() {
Review Comment:
**[P2] Keep these wrapper caches scoped to the projected column that warmed
them**
`PaimonJniScanner` owns one root `PaimonColumnValue` and rebinds it with
`setIdx()` for every projected field. For stable rows shaped as `a
ARRAY<INT>=[1..N], n INT=NULL`, appending `a` fills `arrayValues`, then
appending `n` reaches this `isNull()` and clears all four caches; the next row
recreates the list and all N wrappers. A later empty ARRAY or MAP similarly
trims the shared family cache to zero. The synchronous `VectorColumn`
consumption keeps values correct, but this ordinary multi-column path restores
O(rows * elements) wrapper allocation and can add cache-list churn beyond the
baseline. Please keep a persistent root/cache graph per projected field (or key
caches by stable field identity) and cover
large-complex-plus-null/empty-later-column cases across multiple rows.
--
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]
