eldenmoon commented on PR #69761:
URL: https://github.com/apache/doris/pull/69761#issuecomment-6598520201
## Review of `c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455`
**Recommendation: request changes.** Two merge-blocking issues remain: one
newly identified BUILD INDEX integration defect, and one native-compaction
consistency limitation already acknowledged in the PR description. The latter
is explicitly identified as known, not presented as a new discovery.
This is a static review of the pinned head against merge base
`339e087b11f760e9e45ce33d94862b4b63f1d9ee`. All 60 changed files were covered,
followed by an independent counterargument pass on both findings. **The
reproduction sequences and results below are source-derived; I did not execute
SQL, builds, tests or benchmarks.**
### F-01 — Blocker: BUILD of an ordinary string index can terminate the BE
when values and numeric indexes share an SNII container
A supported ordinary-index operation can force the generic SNII
`IndexBuilder` to rebuild an existing VARIANT values index, although that
builder cannot handle VARIANT.
**Concrete trigger:** local/shared-nothing mode with the default
`enable_light_index_change=true`. Create the values and numeric indexes, write
an old rowset, and only then add the string index:
```sql
SET default_variant_enable_nested_group=false;
CREATE TABLE vvi_build (
id INT, v VARIANT, s STRING, n INT,
INDEX values_idx(v) USING INVERTED
PROPERTIES("variant_index_scope"="values", "parser"="english"),
INDEX n_idx(n) USING INVERTED
) DUPLICATE KEY(id)
DISTRIBUTED BY HASH(id) BUCKETS 1
PROPERTIES(
"replication_num"="1",
"inverted_index_storage_format"="SNII",
"disable_auto_compaction"="true"
);
INSERT INTO vvi_build VALUES
(1, parse_to_variant('{"x":"hello"}'), 'text', 1);
ALTER TABLE vvi_build ADD INDEX s_idx(s) USING INVERTED
PROPERTIES("parser"="english");
-- Wait for the light ADD metadata job to complete.
BUILD INDEX s_idx ON vvi_build;
```
The ordering is essential: the historical rowset must lack `s_idx`,
otherwise BUILD can skip it as already indexed. No ADD/BUILD of the values
index itself is requested, so the existing FE restriction does not prevent this
sequence.
The reachable failure chain is:
1. The numeric SNII writer persists a BKD blob in the source container.
2. `plan_snii_index_rewrite` cannot inherit unchanged logical indexes from a
container containing a blob. It schedules **all resident logical indexes** for
rebuild, including `values_idx`, not just the requested `s_idx`.
3. `_build_snii_indexes_for_segment` checks generic support for VARIANT with
`DORIS_CHECK` **before** its local `try`.
4. Generic index writing rejects VARIANT. Debug builds log FATAL; in release
the fatal exception escapes the actual BUILD callback/thread-pool/pthread-entry
chain, which has no enclosing catch-and-convert boundary. The consequence can
therefore be BE process termination, not merely a failed index job.
Primary evidence:
- [Historical rowset
selection](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/be/src/storage/tablet/tablet.cpp#L1376-L1404)
and [FE BUILD restriction on the requested
column](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java#L2595-L2606).
- [BKD blob
registration](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/be/src/storage/index/inverted/snii/snii_bkd_index_writer.cpp#L242-L244)
and [container-wide rewrite
planning](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/be/src/storage/task/index_builder.cpp#L78-L140).
- [VARIANT support check before the local
try](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/be/src/storage/task/index_builder.cpp#L929-L944)
and [generic unsupported-type
check](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/be/src/storage/index/index_writer.cpp#L34-L35).
- [Actual BUILD
callback](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/be/src/agent/task_worker_pool.cpp#L824-L835)
and [pthread
entry](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/be/src/util/thread.cpp#L430-L463).
**Suggested fix:** make the container rebuild plan account for every
resident logical index. Either rebuild a values index through its specialized
writer using a complete root assembled from stored values, or provide a
blob-capable container rewrite that safely preserves that unchanged values
index. Add an exception-to-Status boundary as well; catching the exception
alone still leaves a supported BUILD operation unable to complete.
**Required regression:** values + numeric/BKD indexes, historical insert,
ADD/BUILD ordinary STRING index, then verify both the new STRING postings and
the existing values postings. This is a newly exposed integration defect on
values-enabled tables; it is not a claim that existing ordinary tables without
values indexes regress.
### F-02 — Major, known PR-declared limitation: native compaction can make
indexed and index-disabled MATCH return different row sets
The PR already acknowledges boolean/numeric token drift. I still consider it
merge-blocking because the values index is consumed as an **exact predicate
result**, while its postings can cease to describe the final stored value.
A concrete case uses ORDINARY storage, a materialized dynamic path `n`, and
two separately written rowsets:
```text
id=1: {"n":true}
id=2: {"n":1}
```
The source physical types are BOOLEAN and BIGINT. With native inverted-index
compaction and vertical VARIANT subcolumn expansion, data compaction selects
BIGINT and casts `true` to numeric `1`. Native values-index merging only remaps
source postings, so id 1 retains token `true` and does not gain token `1`.
**Source-derived results after that compaction:**
| Predicate | Index enabled | Index disabled |
|---|---|---|
| `WHERE v MATCH_ANY 'true'` | id 1 | no rows |
| `WHERE v MATCH_ANY '1'` | id 2 | id 1 and id 2 |
These are respectively a false positive and a false negative relative to the
final reconstructed values.
For a regression, explicitly set ORDINARY mode, disable NestedGroup, use
`default_variant_max_subcolumns_count=100` so the sole `n` path materializes,
disable automatic compaction until both rowsets exist, and disable
query/condition/index caches. Run a real full compaction with:
```text
enable_vertical_compaction=true
enable_ordered_data_compaction=false
inverted_index_compaction_enable=true
enable_vertical_compact_variant_subcolumns=true
```
The existing regression suite provides [the full-compaction
helper](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/regression-test/suites/variant_p0/v2/with_index/test_variant_values_index_compaction.groovy#L100).
Both source logical indexes must be healthy and must not have
`skip_index_compaction` set. Compare identical WHERE queries with:
```sql
SET enable_match_without_inverted_index=true;
SET enable_inverted_index_query=true;
SELECT id FROM vvi_bool_native WHERE v MATCH_ANY 'true' ORDER BY id;
SELECT id FROM vvi_bool_native WHERE v MATCH_ANY '1' ORDER BY id;
SET enable_inverted_index_query=false;
SELECT id FROM vvi_bool_native WHERE v MATCH_ANY 'true' ORDER BY id;
SELECT id FROM vvi_bool_native WHERE v MATCH_ANY '1' ORDER BY id;
```
A plain MATCH projection is **not** a reliable scalar oracle: it can itself
use an index-backed scan virtual column. Explicitly disable index execution for
any projection used as a scalar control.
The mechanism is established by:
- [Values-root native
admission](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/be/src/storage/compaction/compaction.cpp#L1679-L1683)
and [the planner selecting rebuild only when the pair is absent from the
native
set](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/be/src/exec/common/variant_util.cpp#L1175-L1183).
- [Physical leaf conversion to the output
type](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/be/src/storage/segment/segment_iterator.cpp#L3105-L3112)
and [native indexes being excluded from fresh
writing](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/be/src/storage/segment/vertical_segment_writer.cpp#L200-L208).
- [Exact MATCH bitmap
consumption](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/be/src/storage/segment/segment_iterator.cpp#L1539-L1545)
and [filter
removal](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/be/src/storage/segment/segment_iterator.cpp#L1643-L1647).
There is no scalar residual to correct stale positives; a positive-only
recheck could not recover rows already removed by false-negative postings
anyway.
**Suggested fix:** require a per-root guarantee that source-to-output
conversion and visibility changes preserve canonical leaf text before retaining
native merge. Otherwise use the existing whole-root rebuild path. A
conservative temporary fallback is to exclude values roots from native merging,
with the performance tradeoff made explicit. A finer veto can retain native
merging for proven-safe roots. The corrected native set must consistently drive
schema expansion, writer exclusion and posting merge; marking a root for
rebuild while leaving its pair native would still suppress the necessary writer.
This does **not** establish newly lost committed raw values: underlying
VARIANT type promotion is pre-existing. The defect is silent
query-plan-dependent filtering against the final values. The separately
documented empty-array/sparse visibility limitation also needs to be covered by
the eventual semantic-stability guarantee; it is not being presented as another
new finding here.
**Coverage gap:** [the mixed native/rebuild BE
test](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/be/test/storage/variant/index_storage_variant_compaction_read_test.cpp#L342-L383)
explicitly forces its bool/int root to rebuild. [The SQL compaction
suite](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/regression-test/suites/variant_p0/v2/with_index/test_variant_values_index_compaction.groovy#L49-L52)
explicitly avoids bool/numeric mixing. Neither tests this native case. Add the
two-rowset input without a skip flag and assert index-enabled/index-disabled
parity for both tokens before and after compaction.
### Additional performance, maintainability and compatibility notes
- The shared final-value assembler and leaf visitor are useful boundaries to
retain. The earlier repeated-compaction-planning concern is resolved:
eligibility is computed once after filtering and before expansion.
- The [new scalar MATCH
path](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/be/src/exprs/function/match.cpp#L127-L143)
collects and sorts all row tokens, approximately `O(T log T + Q log T)` work
with `O(T)` token storage. Consider reusing [query-term membership and
early-exit
behavior](https://github.com/apache/doris/blob/c7fec9b1ff79cd0fbc4296189f0ccedd9ee98455/be/src/exprs/function/match.cpp#L672-L765)
from existing MATCH implementations. This is an optimization opportunity, not
a measured slowdown claim.
- The 4096-row writer batch bounds rows rather than bytes. Add
feature-specific measurements for wide documents/high token counts, peak
memory, ingest, scalar MATCH and native-versus-rebuild compaction cost.
- Current-version legacy root/path behavior and the historical CAST/alias
issue were rechecked; no additional defect was confirmed there. Reusing the
existing property map and wire fields does not establish mixed-version or
downgrade safety. Enable the feature after the relevant FE/BE nodes are
upgraded unless explicit mixed-version support is established.
- Existing tests inspect real data, schema, postings and MoW deletion
behavior. The two decisive missing integration gates are F-01's historical
BUILD with a resident blob and F-02's native type-promotion parity case.
Please address F-01/F-02 and add their targeted regressions before merging.
--
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]
