Doris-Breakwater commented on issue #68118:
URL: https://github.com/apache/doris/issues/68118#issuecomment-5709813345
Breakwater-GitHub-Analysis-Slot: slot_d74038b87ed5
### Initial triage
**Assessment: confirmed v1 correctness issue, with one scope qualification
for INT96.** I reviewed the exact reported revision `dea1b99e528`. This can
produce false-negative pruning, so it should be treated as a correctness bug
even though `enable_file_scanner_v2` defaults to `true`. At triage time the
issue is open, has no labels or comments, and the reporter is willing to submit
a PR.
#### Verified from the code
1. **The DST rollback case is valid when an adjusted-to-UTC INT64 timestamp
is exposed as DATETIMEV2.**
- `ParquetPredicate::parse_min_max_value` passes the session
`cctz::time_zone` to `PhysicalToLogicalConverter` and converts the two physical
bounds independently.
- For an INT64 Parquet TIMESTAMP with `isAdjustedToUTC=true`,
`Int64ToTimestamp` applies the offset at each instant. There is no v1 check
equivalent to `timestamp_min_max_is_safe` / `utc_timestamp_range_is_monotonic`
before the converted fields become pruning bounds.
- Therefore, across the New York 2021 rollback, UTC `[1636263000,
1636266600]` (05:30-06:30 UTC) converts to civil `[01:30, 01:30]`, although an
interior value such as 05:59 UTC converts to 01:59. A `dt > '2021-11-07
01:45:00'` predicate can consequently reject the zone while a matching row
exists.
- The unsafe fields are consumed by both footer row-group pruning
(`_process_column_stat_filter` and `_process_expr_zonemap_filter`) and
page-index pruning (the comparison/in-list predicate path and
`_process_expr_zonemap_page_filter`). All of these paths already fall back
conservatively when parsing returns a non-OK status, so rejecting the
statistics centrally is the right shape of fix.
2. **The `max_field` typo and over-broad condition are both real.**
- Lines 344-345 read `min_field` twice, so the local `min_value !=
max_value` check cannot fail.
- The condition is `physical INT96 || logical DATETIMEV2`. Merely
changing the second read to `max_field` would incorrectly reject every
non-singleton INT64-backed DATETIMEV2 range.
3. **INT96 impact is narrower than the issue currently states.**
- Footer row-group statistics already pass through `read_column_stats`,
which rejects unequal INT96 `min_value`/`max_value` at lines 379-386; the
legacy `min`/`max` path also classifies INT96 order as unknown and rejects
unequal bounds. Thus I do not see a current footer row-group hole for unequal
INT96 statistics.
- Page-index statistics do bypass `read_column_stats`:
`_process_page_index_filter` copies `ColumnIndex.min_values` / `max_values`
directly and later calls `parse_min_max_value`. The dead check therefore leaves
the page-index paths exposed. Any claim that unequal INT96 *footer* statistics
prune a row group needs a concrete counterexample to the earlier guard.
The DST risk applies when UTC timestamps are mapped to local DATETIMEV2 (the
default when timestamp-TZ mapping is disabled). When
`enable_mapping_timestamp_tz` maps them to TIMESTAMPTZ, the converter preserves
UTC ordering and the rollback fence should not disable those bounds, matching
the v2 policy.
### Recommended change
- In the central v1 statistics parser, validate the raw INT64 timestamp
range before converting it: for `TIMESTAMP(isAdjustedToUTC=true)` mapped to
DATETIMEV2 with a session time zone, reject the range if the UTC-to-civil
mapping contains a backward transition. Apply the same instant-semantics
handling to legacy `TIMESTAMP_MILLIS` / `TIMESTAMP_MICROS` annotations. Reuse
or relocate the existing v2 helper rather than duplicating transition logic;
keep TIMESTAMPTZ exempt.
- Separate the INT96 rule from DATETIMEV2. Apply the singleton-only rule
only when the physical Parquet type is INT96, preferably by comparing the
encoded physical bounds before conversion. General INT64-backed DATETIMEV2
min/max must remain usable.
- Preserve the existing behavior in every caller: an unusable range means no
zone map / no pruning, not a scan error.
### Regression coverage requested
- A v1 end-to-end case with `enable_file_scanner_v2=false`,
`time_zone='America/New_York'`, an adjusted-to-UTC INT64 TIMESTAMP range
spanning the 2021 rollback, and `dt > '2021-11-07 01:45:00'`; compare results
with metadata pruning disabled. Cover both footer row-group statistics and page
indexes.
- Unit cases showing rollback ranges are rejected, a transition-free range
remains usable, and spring-forward does not get rejected solely for crossing
the gap.
- INT96 unequal bounds are unusable on the direct page-index path; equal
bounds remain usable; unequal INT64 DATETIMEV2 bounds remain usable.
- If timestamp-TZ mapping is supported on v1, pin that TIMESTAMPTZ ranges
keep UTC ordering and are not unnecessarily discarded.
### Missing runtime evidence
The code mechanism is sufficient to confirm the defect, but the report does
not include an end-to-end reproducer. For a runtime confirmation, please attach
either a minimal Parquet file or a deterministic generator plus the table DDL,
full SQL/session settings, and a metadata dump showing the writer, logical
timestamp annotation, `isAdjustedToUTC`, physical min/max, column order, and
whether page indexes are present. For the INT96 claim in particular, include a
file with an unequal `ColumnIndex` range so the page-index reachability is
explicit. Also include result counts with metadata pruning enabled and
disabled; profile counters such as `RowGroupsFilteredByMinMax`,
`ParquetExprZoneMapFilteredRowGroups`, `FilteredRowsByPage`, and
`PageIndexReadCalls` would identify the affected consumer.
### Temporary mitigation
Prefer the default v2 scanner. If the v1 scanner is required before a fix is
available, `set enable_parquet_filter_by_min_max = false` disables these
min/max pruning paths at the cost of additional I/O and decoding.
--
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]
