Doris-Breakwater commented on issue #69865: URL: https://github.com/apache/doris/issues/69865#issuecomment-6503365190
Source review supports a **VARIANT V2 write/read validation bug**: canonical dotted paths can exceed the depth accepted by reconstruction. This merits storage correctness/availability triage. The issue currently has no labels; suggested area: BE / VARIANT V2. I inspected the configured repository's `4.1.4` tag (commit `ad35a140c7fd0b842f18c23300bac581f7d04326`) and current public master (`2a4191c1dc94e789e1725d6cb345f46c31a17272`). This is source-level verification; I have not run the reported BE unit test or the SQL sketch. **Verified mechanism** - JSON collection advances depth when visiting a child value; dots inside an object key do not increment that depth. [`JsonTreeCollector`](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/exprs/function/parse/variant_string_parse.cpp#L142-L195) - `VariantShredder::resolve_child_path()` joins the parent and key, then reconstructs `PathInData(child.get_path())` without a depth check. The string constructor splits at every dot. Thus the proposed flat scalar key with 200 components becomes a 200-part storage path despite shallow input nesting. The threshold is **129 parts (128 dots)**, not “more than 128 dots.” [`shredder`](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/be/src/storage/segment/variant/v2/variant_shredder.cpp#L173-L220), [`path splitting`](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/util/json/path_in_data.cpp#L35-L47) - Materialized paths encounter the depth check during `VariantAssembler::create()` through `build_materialized_slots()`, before row assembly; `MergeValue::set_materialized()` also checks it. Master has the reported checks at lines 362 and 651; the inspected 4.1.4 source has them at 362 and 644. `DORIS_CHECK_LE` uses a debug assertion without `NDEBUG` and an exception-producing check in release. [`assembler initialization`](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/be/src/storage/segment/variant/v2/variant_assembler.cpp#L619-L665), [`check definitions`](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/common/check.h#L120-L153) **Impact needs qualification** Whole-object reconstruction involving an over-depth materialized path reaches the failing invariant. Sparse/doc paths already have an explicit `Status::Corruption` depth check, so their failure mode differs. Direct leaf reads can bypass the assembler, and compaction with extracted columns has a separate flat-leaf plan. Consequently, “every read” and “all compaction” are broader than the evidence supports; compaction that requires the affected reconstruction remains a concern. [`sparse/doc validation`](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/be/src/storage/segment/variant/v2/variant_assembler.cpp#L144-L153), [`compaction planning`](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/storage/segment/variant/variant_column_reader.cpp#L829-L837), [`leaf iterator`](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/storage/segment/variant/variant_column_reader.cpp#L1014-L101 9) **Recommended next steps** 1. Reject an over-limit **canonical full path**, including its parent prefix, in the shredder before registering/caching it. Return `InvalidArgument` with a clearly defined row index, actual/maximum part counts, and a bounded key/path prefix. Preserve the existing failed-state propagation so a rejected append cannot later publish partial output. Checking raw JSON nesting or only the current key is insufficient. 2. Validate stored materialized paths through the assembler's status-returning initialization path and return `Corruption` before either depth assertion can fire. Keep sparse/doc handling consistent. This makes existing malformed segments fail cleanly; it does not make their data readable or repair them. Preserve existing dotted-key canonicalization semantics. 3. Add shredder-to-assembler coverage for 128, 129, and 200 components, including real nesting combined with dotted keys, ordinary materialized storage, sparse storage, and doc mode. Add a pre-existing over-depth materialized-path fixture that returns `Corruption` in debug and release. Existing `DepthBoundaries` coverage checks valid materialized depth and excessive sparse depth, leaving the excessive materialized case uncovered. [`existing test`](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/test/storage/segment/variant_assembler_legacy_test.cpp#L986-L1028) **Evidence still needed** Please attach the reproducing BE test and its command/output, exact BE build SHA and build type, and an executed SQL reproduction with full `SHOW CREATE TABLE`, V2 settings, materialization/subcolumn limit, and doc-mode setting. Include the successful load result and first failing query's error/stack. To establish compaction impact, provide the failing compaction log/stack and whether it used hierarchical reconstruction or flat-leaf reads. A performance profile is not needed to establish this depth-validation failure. Pending a fix, rejecting over-depth canonical paths upstream can prevent new affected writes. It does not repair existing segments. No repository code was modified during this investigation. Breakwater-GitHub-Analysis-Slot: slot_28047bdabe0a -- 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]
