Repository navigation
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69995 Problem Summary: Several BE paths identified the internal sparse and doc-value columns of a Variant by substring, so a user key such as x__DORIS_VARIANT_SPARSE__y was treated as an internal column. With storage_format V3 the key's metadata was embedded in the root without a path-index entry and the reader ignored it, so its values read as NULL and a later compaction dropped them. FULL compaction could also pass such a scalar column to the sparse statistics code, which expects a Map, and a doc-marker key made the reader throw from std::stoi. Recognize only the relative names writers generate (the single sparse column and canonical .b<n> sparse and doc buckets) with one helper, and use it at every site. Read ordinary children embedded by older V3 writers as regular subcolumns; this is per-segment metadata work with no per-row cost. A key exactly equal to an internal name is out of scope. ### Release note Fix wrong reads, compaction failures and possible data loss for Variant keys that contain the internal sparse or doc-value column names, and make subcolumns hidden by older V3 segments readable again. ### Check List (For Author) - Test: Unit Test and Regression test. - BE UT: exact identity against writer-generated names and malformed bucket suffixes, V3 metadata classification, statistics, inline round trip, and recovery of a V3 segment written with the old classifier. Affected test classes: 894 passed, 4 skipped. - Regression: reserved_internal_path_key, DUP/MOW x V2/V3 x ordinary/doc x 1/3 shards, load-time materialization and two FULL compactions, checking each row's value and the whole Variant. - variant_p0: all suites pass except two that fail the same way on unmodified master. - Behavior changed: Yes. Keys containing internal column names are ordinary paths. - Does this need documentation: No. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
run buildall |
TPC-H: Total hot run time: 28537 ms |
TPC-DS: Total hot run time: 151615 ms |
ClickBench: Total hot run time: 24.26 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Static review completed for PR 70060 at head d5749a0, against base 61906ce. No new P0, P1, P2, or P3 findings were verified, and there are no existing blocking inline findings to carry forward. The user focus file is empty, so no additional focus concern applies.
The change's goal is met: exact writer-generated Variant sparse/doc names are classified consistently, marker-containing user paths remain ordinary data, and legacy V3 embedded ordinary children are recoverable. R1 was dismissed after tracing the shared bucket helper through writer dispatch, V2 doc parsing, external metadata classification, reader plans, statistics, path-stat admission, physical-name constructors, and compaction schema generation. R2 was dismissed after tracing root children, relative-path normalization, ordinal -1 handling, embedded-map precedence, external metadata loading, ColumnReader creation, cache access, and the existing metadata lock. R3 was dismissed after tracing V2 DOC shredding/finalization, row and null propagation, scalar versus Variant writer selection, sparse/doc readers, V3 externalization, old segment compatibility, compaction, and the added tests.
Critical review checkpoints: data correctness and visibility are preserved by the ordinary writer/reader path plus direct legacy metadata recovery; the patch is focused around one classifier and its call sites. The new metadata map is immutable after initialization, and existing shared/exclusive metadata locking covers the mutable tree and external loading. No unusual lifecycle or static-initialization issue was found. V2/V3 storage compatibility was specifically traced, including old embedded metadata. Parallel query, compaction, checksum, and cache paths were checked. Conditional parsing rejects malformed, signed, leading-zero, trailing, nested, and overflowing bucket suffixes while preserving exact generated names. The BE unit and regression changes provide targeted coverage for these paths, but they were inspected only and not executed. No new observability, transaction/EditLog, FE-BE variable, configuration, thrift/protobuf, or external write protocol change was introduced. The added map metadata accounting and parser work are bounded and consistent with the surrounding design.
Validation was static-only: authoritative diff, current source, required AGENTS.md files, review skill, tests, ledger, and live PR state were inspected; no build, test, benchmark, cluster, or runtime validation was run. All three assigned round-1 scopes returned no new valuable findings, every risk item has an explicit dismissal with evidence, and the review converged in round 1.
Review converged after 1 round(s).
What problem does this PR solve?
Issue Number: N/A
Related PR: #69995 (found during its review; this defect predates that PR and also exists on branch-4.1 and branch-4.2)
Problem Summary:
A user JSON key such as
x__DORIS_VARIANT_SPARSE__yis an ordinary Variant path. Several BE paths identify the internal sparse and doc-value columns by substring (find("__DORIS_VARIANT_SPARSE__"),find(DOC_VALUE_COLUMN_PATH)), so such a key is mistaken for a physical binary column:variant_ext_meta_writer.cpp): the user subcolumn is embedded in the root's children with no external path-index entry. The reader accepts only the exact sparse name or.b<n>buckets there and ignores the child, so the values read as NULL, and the next compaction no longer sees the path and drops them.variant_stats_calculator.cpp, any storage format): a materialized user column with the marker text is passed to the sparse statistics code, which casts it toColumnMap.std::stoi, which throws for a user key.Reproduction on an ASAN build of master, with
variant_max_subcolumns_count=1and three separate inserts{p:701,hot_a:11}/{hot_a:12},{p:702,hot_b:21}/{hot_b:22},{p:703,hot_c:31}/{hot_c:32}wherep = "x__DORIS_VARIANT_SPARSE__y":v[p]is NULL right after the loadBad cast from type:doris::ColumnNullable to doris::ColumnMap__DORIS_VARIANT_DOC_VALUE__in V3std::invalid_argumentfromstd::stoiFix:
variant_util::variant_binary_column_bucket, recognizes only the relative names writers generate: the single sparse column and canonical.b<n>sparse and doc buckets (.b01,.b1tail,.b1.childare user paths). It replaces the substring tests at every site and the separate bucket parsers in the reader and the doc compact writer.Out of scope: a user key exactly equal to an internal name (for example a top-level
__DORIS_VARIANT_SPARSE__) still collides with the physical column; on an ASAN build master stops at the reader'scol.has_variant_statistics()check.Release note
Fix wrong reads, compaction failures and possible data loss for Variant keys that contain the internal sparse or doc-value column names, and make subcolumns hidden by older V3 segments readable again.
Check List (For Author)
reserved_internal_path_key, DUP/MOW × V2/V3 × ordinary/doc × 1/3 shards. Each case checks load-time materialization and two FULL compactions (a new rowset in between; MOW also overwrites rows), comparing every row'sv[p]and the whole Variant.variant_p0: all suites pass exceptoutfile_csv_variant_doc_value(object storage 403) and the old-directorydouble_write_schema_change_with_variant(public1 vs true), which fail the same way on unmodified master.Check List (For Reviewer who merge this PR)
🤖 Generated with Claude Code