Repository navigation
Conversation
|
/review |
|
Codex automated review failed and did not complete. Error: Review setup stopped before context preparation ran; inspect the first failed or cancelled setup step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
run buildall |
CIR-22588: compaction promotes sparse paths to materialized subcolumns by a raw-byte lower_bound. Sparse keys written in component order were missed, filled with NULL, and removed from sparse, so the non-null values disappeared from the output file. The writer now publishes byte-ordered sparse keys; this suite verifies that after a full compaction the promoted tag_id/tag_name values are still present, using the customer's six-row shape and control rows.
506ab2f to
0c4b29f
Compare
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes for two independently verified issues: the added p0 suite fails on an unregistered FE setting before reaching its assertions, and its compaction check can pass after a failed rewrite. Both findings are inline.
Reviewed both changed files and traced the relevant FE configuration lookup, regression harness, sparse writer, path statistics, promotion reader, and local/cloud full-compaction paths at 0c4b29f. The initial hot fields have count 2 versus sparse target leaves at count 1; combining the two batches raises each target leaf to count 5 and selects both for materialization. The punctuation keys exercise the intended component-order versus byte-order inversion.
Critical checkpoint conclusions:
- Goal and proof: appropriate sparse-promotion preservation scenario; startup failure and missing successful-compaction proof prevent reliable coverage.
- Scope and clarity: focused, two-file regression-only change.
- Concurrency and locking: compaction is asynchronous; task termination alone does not prove success. No production threads or locks change.
- Lifecycle and static initialization: table is dropped before use and retained for debugging; session connections close at suite teardown. No static ownership changes.
- Configuration: no new settings; the attempted enable_variant_v2 override is absent at this head. Doc-mode and cache overrides are session-local.
- Compatibility: no production symbols or storage formats change.
- Thrift/protobuf: no definition or field-ID changes.
- Parallel paths: inspected shared helper and local/cloud compaction completion behavior; the success-verification gap applies to both.
- Conditional checks: no new conditionals; the relevant helper accepts either a success or failure timestamp change.
- Test coverage: six distinct IDs cover nested leaves, parent reconstruction, missing-path NULLs and newer-row controls; successful rewrite must also be established.
- Expected results: all three labels, ordered rows, NULLs and JSON strings are statically consistent with the inserts and projections.
- Observability: existing tablet/status logs and retained table support diagnosis; checking last full status would expose the missed failure.
- Transactions and persistence: no EditLog, replay or failover implementation changes.
- Writes and atomicity: two sequential inserts use distinct unique IDs, preserving row 1; a merge failure retains readable input rowsets, which explains the false pass.
- FE/BE propagation: no new variables or protocol fields.
- Performance and further issues: bounded six-row workload; no production cost change or further verified issue after recheck.
No additional user focus was supplied. No existing inline findings required deduplication. Static inspection only: no builds, tests, benchmarks, or runtime validation ran; generated-output provenance and the author's execution claims were not independently verified. Both reviewers completed the second round with NO_NEW_VALUABLE_FINDINGS; all candidates and risk focuses have explicit dispositions. Review converged in 2 rounds with no unresolved code questions or uncovered changed files.
Review converged after 2 round(s).
| // then removed from sparse, so the values disappear from the output file. The writer must | ||
| // publish sparse keys in raw-byte order; after full compaction the promoted values must | ||
| // still be present. | ||
| setFeConfigTemporary([enable_variant_v2: true]) { |
There was a problem hiding this comment.
[P1] Remove the obsolete enable_variant_v2 configuration wrapper
At this head enable_variant_v2 is not a registered FE configuration. setFeConfigTemporary first calls getFeConfig, which reads [0].Value from SHOW FRONTEND CONFIG LIKE 'enable_variant_v2'; that result is empty, so every run throws before CREATE TABLE or any assertion executes. Remove the obsolete wrapper so the regression can actually run on this branch.
| (5, parse_to_variant('{"tag-labeling":{"text_info":{"open":{"tag_id":"id5","tag_name":"open5"}}}}')), | ||
| (6, parse_to_variant('{"tag-labeling":{"text_info":{"open":{"tag_id":"id6","tag_name":"open6"}}}}')) | ||
| """ | ||
| trigger_and_wait_compaction(tableName, "full", 1800) |
There was a problem hiding this comment.
[P2] Verify that the full compaction succeeded
Once the obsolete config wrapper is removed, an asynchronous compaction that fails in merge_input_rowsets() before modify_rowsets() leaves the original rowsets readable. trigger_and_wait_compaction returns when either the full-success or full-failure timestamp changes, so all subsequent expected rows still match in that case and the suite passes without checking any promoted output. Check each tablet's last full status (and preferably rowset replacement), as the neighboring sparse_ancestor_promotion suites do, before the after-compaction queries.
|
run p0 |
|
run cloud_p0 |
|
run vault_p0 |
|
run external |
|
run nonConcurrent |
|
run external |
1 similar comment
|
run external |
|
run nonConcurrent |
|
run cloud_p0 |
|
run compile |
|
run feut |
|
run compile |
What problem does this PR solve?
CIR-22588: after a full compaction, a VARIANT row's non-null sparse values (
tag-labeling.text_info.open.tag_id/tag_name) became NULL in the materialized subcolumns and were removed from sparse, so the values disappeared from the output segment. The compaction path promotes sparse paths by a raw-bytelower_bound; sparse keys written in component order were missed and then dropped by the merge.What does this PR do?
Adds a regression suite only (no production code change). With the byte-ordered sparse writer (#70189) and the exact-cell promotion (#70212), the suite:
variant_max_subcolumns_count=2and MOW;tag_id/tag_name/the parent object after compaction, and that control rows 3/4 are intact.The suite forces
enable_variant_v2=truewithsetFeConfigTemporary, since the issue is specific to the V2 sparse Map.Verification
sparse_promotion_compaction, before/after/control all match).