eldenmoon commented on PR #67551: URL: https://github.com/apache/doris/pull/67551#issuecomment-5712447003
Addressed the automated review (1 P1 + 6 P2) in `2566abcb8c1`. None of these change the baseline numbers already recorded in `BASELINE.md` — they only add validation, fix failure-path safety, or correct prose (details in the commit message). 1. **[P1] Sparse import route not validated outside timing** — added a deterministic value/type checksum built from the untimed generated input, checked by reopening every written segment after all writers join. Building it surfaced a real, benign edge case: `parse_to_variant` infers a whole-numbered double (e.g. the generator's `"203932.0000"`) as an integer subcolumn, so a naive int-vs-double-typed checksum flagged a false positive on that value. Fixed by canonicalizing every JSON number through the same fixed-point format regardless of its parsed subtype — this still fails on an actual dropped/misrouted value (verified: before that fix, the check correctly caught the real mismatch; after it, all 20 `BM_VariantSparseImport` cases pass at 5,000 rows with `error_occurred: false`). 2. **[P2] `configured_rows()` accepts row counts the compaction oracle can't satisfy** — `VariantCompactionBenchmarkFixture::prepare()` now rejects `rows_per_rowset < CANDIDATE_PATHS` early with a clear `Status`, instead of failing deep inside `validate_layout()` after a wasted run. Verified: `DORIS_VARIANT_BENCHMARK_ROWS=10` now fails immediately with a descriptive message; `=2000` (200 rows/rowset) still passes all 7 scenarios. 3. **[P2] `real_time` timing boundary vs. documented phases** — documented (comment + this reasoning) that `real_time`/`concurrent_wall_ns_per_row` cover the whole concurrent phase end-to-end (thread spawn, writer work, join), not only `VerticalSegmentWriter` init/append/finalize; `cpu_s_per_1m_rows` and the `*_ns_per_row` counters remain the phase-scoped numbers for comparison. Left the timing code itself alone since narrowing it would change `real_time`, and this baseline never cited that counter. 4. **[P2] Worker threads not join-safe** — added a `Defer`-based join guard (so a later `std::thread` construction throwing doesn't leave earlier threads un-joined) and `std::exception`/catch-all handling alongside the existing `doris::Exception` catch. 5. **[P2] `BASELINE.md` causal claim** — corrected: that run pinned all 8 `Writers8` threads to one logical CPU (`taskset -c 16`), so it cannot show cross-core contention; the "matches this PR's description" claim is replaced with an explicit caveat that a real contention comparison needs ≥8 distinct physical CPUs. 6. **[P2] Executable not bound to source snapshot** — `environment.txt` now logs that the binary's sha256 is not independently checked against `git_head`/`source.diff`/thirdparty fingerprints, and to retain the exact binary for a trustworthy comparison (the lighter of the two options offered, since building a real build-manifest binding felt like more machinery than this benchmark script needs). 7. **[P2] CMakeCache probe misses `build_Release`** — now probes both `be/build_RELEASE` and `be/build_Release` and reports when neither has a cache. `build-support/clang-format.sh` and `build-support/check-build-hygiene.sh` both pass on the changed file. -- 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]
