csun5285 opened a new pull request, #67642:
URL: https://github.com/apache/doris/pull/67642
### What problem does this PR solve?
A string zone map bound is cut to 512 bytes. The min is then a prefix of the
smallest value, and the max is raised by one byte so it still stands above
every value sharing that prefix. Neither bound is a value the column holds, and
two places trusted them anyway.
**1. A cut max ending in `0xff` wraps and hides rows.**
Adding one to `0xff` wraps it to `0x00` and leaves a max *below* the real
value, so the zone map rules out rows it covers. Doris stores strings as UTF-8,
where `0xff` never appears, but a string column holds arbitrary bytes and a
regression inserts `unhex('FF')`. A single row holding `repeat('a', 511) ||
0xff` was invisible:
```sql
SELECT count(*) FROM t; -- 1
SELECT count(*) FROM t WHERE v = <that value>; -- 0, expected 1
SELECT count(*) FROM t WHERE v >= <that value>; -- 0, expected 1
SELECT count(*) FROM t WHERE v IN (<that value>); -- 0, expected 1
```
**2. MIN/MAX push-down read those bounds as if they were values.**
FE decided by the declared column length, which is wrong in both directions:
| column | bounds actually cut? | old FE decision | result |
|---|---|---|---|
| `VARCHAR(65533)` holding short values | no, bounds are exact | refused |
push-down lost for no reason |
| `VARCHAR(512)` full to its declared length | **yes** | allowed (`512 >
512` is false) | `MAX(v)` returned the raised prefix — a string the table never
held |
The declared length is a table-definition ceiling; whether a bound was cut
depends on the longest value actually written into *that segment*, which only
the storage layer can see.
### What is changed
**Write path** (`zone_map_index.cpp`) — check that a cut max is UTF-8 before
raising it; give up the range for that zone otherwise.
**Read path** (`ZoneMap::from_proto`) — lower the raised byte back and run
the same check on the same bytes, so segments written before this give up their
range instead of ruling out rows they cover.
Both sides judge *the bytes the data held*, not the raised sentinel. That
matters in both directions, measured by exhaustive sweep over all 256 trailing
bytes:
* raising a valid UTF-8 byte can itself produce non-UTF-8 bytes (`0x7f` →
`0x80`, `0xbf` → `0xc0`), so checking the stored max would wrongly give up 3 of
the 256 bounds the writer accepts;
* the wrapped `0x00` **is** valid UTF-8, so checking the stored max would
miss the actual bug.
**MIN/MAX push-down** — FE (`AggregateStrategies`) drops the declared-length
test and pushes every string column down; `segment_zone_maps_can_answer_agg`
skips the statistics iterator for segments whose stored bounds are 512 bytes,
per segment. Both `min` and `max` are checked there: a cut min alone would make
`MIN()` return a truncated string, and a cut min with an uncut max is reachable
(values `repeat('a',600)` and `'b'`).
`enable_pushdown_string_minmax` keeps its role and now covers
`CHAR`/`VARCHAR` as well as `STRING`. **Its default changes from `false` to
`true`** so the push-down stays on; turning it off stops string MIN/MAX
push-down entirely.
### Trade-off
The 512-byte cut is a byte cut, so it can split a character and leave a
bound that is not UTF-8 even though the data is. Those zones give up their
range too — measured with the repo's own `validate_utf8`, about **two thirds**
of pages holding long CJK text, **three quarters** for four-byte characters.
This costs pruning (page skipping, segment skipping, and `MIN`/`MAX` push-down
for those segments); it never costs rows.
### Known limitations, both of which predate this PR
* `DataTypeStringSerDeBase::from_olap_string()` runs `strnlen` on the stored
bound, so a bound holding an embedded `0x00` parses short and `MIN()`/`MAX()`
can return a truncated string. Such a bound is under 512 bytes, so neither
check here covers it.
* `JSONB` is in neither `PrimitiveType.isCharFamily()` (FE) nor
`is_string_type(FieldType)` (BE), so neither check covers it. Whether JSONB
zone map bounds can be cut is not verified here.
### Release note
Fix rows going missing from a string column whose zone map bound was cut at
512 bytes and ended in `0xff`. `MIN`/`MAX` push-down now decides per segment
from the stored bounds instead of the declared column length, and
`enable_pushdown_string_minmax` now defaults to `true` and covers
`CHAR`/`VARCHAR` as well as `STRING`.
### Check List (For Author)
- Test
- [x] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
BE UT — `run-be-ut.sh --run
--filter='ColumnZoneMapTest.*:VGenericIteratorsTest.*:StatisticsIteratorStringBoundsTest.*'`,
34/34 pass, 5 new:
```
ColumnZoneMapTest.CutMaxThatIsNotUtf8GivesUpTheRange
ColumnZoneMapTest.FromProtoGivesUpTheRangeForAWrappedCutMax
StatisticsIteratorStringBoundsTest.ShortBoundsAnswerFromTheZoneMap
StatisticsIteratorStringBoundsTest.CutBoundsFallBackToReadingTheData
StatisticsIteratorStringBoundsTest.BoundsCutExactlyAtTheLimitFallBack
```
Regression, on a local cluster built from this branch —
`test_pushdown_string_minmax` (new), `test_pushdown_explain`, `test_analyze`,
all pass. Every other suite asserting `pushAggOp=` was checked; the rest assert
`COUNT_ON_INDEX` or `NONE` and are unaffected.
- Behavior changed:
- [ ] No.
- [x] Yes. `enable_pushdown_string_minmax` defaults to `true` instead of
`false` and now gates `CHAR`/`VARCHAR` alongside `STRING`. String `MIN`/`MAX`
push-down no longer depends on the declared column length. Segments whose
string bounds were cut fall back to a normal read, which costs pruning on long
non-ASCII text as described above.
- Does this need documentation?
- [ ] No.
- [x] Yes. `enable_pushdown_string_minmax` changes default and meaning.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01HRURWMJMLsbyNUkQj8vqdE
--
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]
