github-actions[bot] commented on code in PR #66820:
URL: https://github.com/apache/doris/pull/66820#discussion_r3859395421
##########
be/src/format/parquet/vparquet_reader.cpp:
##########
@@ -1565,6 +1567,7 @@ Status ParquetReader::_process_column_stat_filter(
auto cache_iter = bloom_filter_cache.find(parquet_col_id);
if (cache_iter != bloom_filter_cache.end()) {
// Bloom filter already loaded for this column, reuse
it
Review Comment:
[P1] Preserve Doris floating equality before Bloom pruning
V1 also enables FLOAT/DOUBLE here but probes only the literal's raw IEEE
bytes. A file Bloom built from `-0.0` can miss `+0.0` (and vice versa), even
though Doris equality treats them as equal and the min/max path retains that
row group; the Bloom result can therefore falsely prune a matching row. The v2
evaluator already covers both signed-zero encodings and conservatively retains
NaN equality classes. Please give V1 equality/IN the same treatment, or disable
floating Bloom pruning where a raw miss is not conclusive, with both sign
directions covered by tests.
##########
be/src/format/parquet/vparquet_reader.cpp:
##########
@@ -1565,6 +1567,7 @@ Status ParquetReader::_process_column_stat_filter(
auto cache_iter = bloom_filter_cache.find(parquet_col_id);
Review Comment:
[P1] Gate V1 Bloom probes by the Parquet physical representation
The eligibility check just above this cache lookup sees only the widened
Doris primitive. A valid physical INT32/logical UINT32 column is exposed as
BIGINT, so it passes the check but equality/IN hashes the 8-byte logical value
while the writer hashed the 4-byte INT32 carrier; FLOAT16 similarly maps a
2-byte FIXED_LEN_BYTE_ARRAY value to a 4-byte FLOAT probe, and V1's Boolean
special case uses four bytes rather than the one-byte carrier used by
schema-aware v2. A Bloom miss on those different bytes can falsely prune a
matching row group. Please make V1 probing schema-aware and use the exact
physical carrier as v2 does, or conservatively disable converted
representations, with UINT32/FLOAT16/Boolean equality and IN regressions.
##########
be/src/format/parquet/vparquet_reader.cpp:
##########
@@ -1565,6 +1567,7 @@ Status ParquetReader::_process_column_stat_filter(
auto cache_iter = bloom_filter_cache.find(parquet_col_id);
if (cache_iter != bloom_filter_cache.end()) {
// Bloom filter already loaded for this column, reuse
it
+ cached_bloom_filter_bytes -=
cache_iter->second->size();
Review Comment:
[P1] Keep the active Bloom filter keyed to its column
`_collect_predicate_columns_from_conjuncts()` puts predicates for all slots
in one `AndBlockColumnPredicate`, and that AND reuses this single `ColumnStat`
across its children. For `a = 1 AND b = 2`, after A's Bloom accepts 1, B misses
this cache but `stat->bloom_filter` still owns A, so the `if
(!stat->bloom_filter)` below skips reading B and B's literal is probed against
A's filter. A matching row group where A contains 1 but not 2 and B contains 2
is then falsely pruned. Please track the active Bloom's column ID
(return/discard it when `cid` changes), value-initialize/set the stat schema in
Bloom-only mode, and add a two-column AND regression proving each literal uses
its own filter.
--
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]
