[fix](case) Relax show data compaction size check#65391
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review found one blocking test-coverage issue.
Critical checkpoint conclusions:
- Goal/test proof: the PR aims to relax a brittle
SHOW DATAsize equality, but the large-documents part now drops the comparison between the enabled and disabled inverted-index compaction paths, so the test no longer proves the intended equivalence. - Scope/focus: the change is small and test-only.
- Concurrency/lifecycle: no new runtime concurrency, locking, lifecycle, persistence, or transaction behavior is introduced.
- Config/session: the test still toggles the existing BE config and restores it in
finally; the issue is that the two config paths are no longer compared for the documents table. - Parallel paths: the
inverted_index_compaction_enable=trueandfalsecompaction implementations are parallel paths and should remain compared; see the inline comment. - Test standards: no
.outupdate is involved, no expected-error case is involved, andgit diff --check -- regression-test/suites/inverted_index_p2/test_show_data.groovypassed locally. I did not run the regression suite because this checkout is missing.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protoc. - User focus:
.code-review.LlcWw9/review_focus.txtsaid there was no additional user-provided focus.
Subagent conclusions: optimizer-rewrite found no optimizer/rewrite candidate because the patch only changes a regression assertion around DDL, stream load, SHOW DATA, sync, and compaction helpers. tests-session-config proposed TSC-1; I independently verified it and accepted it as MAIN-1. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same final ledger/comment set.
| assertTrue(another_with_index_size != "wait_timeout") | ||
|
|
||
| logger.info("with_index_size is {}, another_with_index_size is {}", with_index_size, another_with_index_size) | ||
| assertEquals(another_with_index_size, with_index_size) |
There was a problem hiding this comment.
This still needs to compare the two config paths. The old assertion checked that the table compacted with inverted_index_compaction_enable=true reported the same SHOW DATA size as the table compacted with the config disabled. After this change, each value is only checked against an independent range, so a regression like 170 KB vs 200 KB in non-cloud mode, or 55 KB vs 70 KB in cloud mode, would pass even though the two implementations disagree materially. Please keep the absolute sanity range if it is needed for cloud/non-cloud variance, but also compare with_index_size and another_with_index_size with a relative tolerance, for example by reusing assert_show_data_size_close for the documents table.
|
/review |
|
Address review feedback by keeping the relative size comparison between the documents table paths while preserving the cloud/non-cloud sanity ranges.\n\nrun buildall |
There was a problem hiding this comment.
Automated review completed for PR #65391 at head e4755e5.
I did not find any new non-duplicate issue to raise. The existing inline thread about preserving the documents-table comparison between inverted_index_compaction_enable=true and false was checked against the current head; the PR now adds assert_show_data_size_close.call(with_index_size, another_with_index_size, "documents table") after the mode-specific range checks, so I did not duplicate that thread.
Critical checkpoint conclusions:
- Goal/test coverage: the change is limited to relaxing flaky
SHOW DATAsize assertions while still checking absolute mode-specific ranges and relative agreement between the two compaction paths. - Scope: one regression test file changed; the patch is focused.
- Concurrency/lifecycle/transaction/persistence/FE-BE protocol: not applicable to this test-only assertion change.
- Configuration paths: existing BE config toggling is still restored in
finally; both enabled and disabled compaction paths remain exercised. - Parallel/special paths: cloud and non-cloud ranges are both covered, and the cross-config comparison remains present.
- Test result files: none added or modified.
- Performance/observability: no production-path impact.
User focus: no additional user-provided review focus was supplied.
Subagent conclusions: optimizer-rewrite and tests-session-config both completed the initial pass with no new valuable findings. After the main ledger recorded the existing-thread duplicate/resolved status and an empty proposed inline comment set, both subagents returned NO_NEW_VALUABLE_FINDINGS in convergence round C-001.
Validation: reviewed GitHub PR metadata/diff and fetched-base diff; ran git diff --check FETCH_HEAD..HEAD -- regression-test/suites/inverted_index_p2/test_show_data.groovy. I did not run the regression suite because this review checkout is missing .worktree_initialized and thirdparty/installed.
Summary
test_show_data_with_compactionso it no longer requires exactSHOW DATAequality after full compaction.Testing
git diff --check -- regression-test/suites/inverted_index_p2/test_show_data.groovy./run-regression-test.sh --run -d inverted_index_p2 -s test_show_data_with_compaction -parallel 1