Revert "Generate the fuzzer dictionary from a release binary instead … - #113649
Revert "Generate the fuzzer dictionary from a release binary instead …#113649rienath wants to merge 2 commits into
Conversation
…of committing it"
|
Workflow [PR], commit [402753f] Summary: ✅
AI ReviewSummaryThis PR reverts the release-binary-based fuzzer dictionary generation and goes back to committing the fuzzing dictionaries and corpus inputs in-tree. That rollback still leaves three correctness gaps in the fuzzing pipeline: the Findings
Final Verdict
|
clickhouse-gh
Bot
added
the
pr-not-for-changelog
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
|
|
||
| # copy fuzzer options | ||
| # copy fuzzer options and dictionaries | ||
| cp $SRC/tests/fuzz/*.dict $OUT/ |
There was a problem hiding this comment.
This copy makes the arm_fuzzers artifact depend on tests/fuzz/*.dict again, and the seed corpus ZIPs below still depend on tests/queries/0_stateless/*.sql, but the revert simultaneously deletes the special_build_jobs digest override in ci/defs/job_configs.py. The shared build digest does not hash either tree, so a commit that changes all.dict, a .options file, or the seed corpus can now cache-hit an older ARM_FUZZERS artifact and NightlyFuzzers will fuzz the previous revision's inputs.
I think we need to keep the arm_fuzzers-only digest extension even if we revert the binary-generation part.
| "${CURRENT_DIR_IN_BINARY}/clickhouse.g" | ||
| COMMAND ./update.sh "${CURRENT_DIR_IN_BINARY}/codegen.dict" | ||
| DEPENDS "${CURRENT_DIR_IN_BINARY}/codegen.dict" | ||
| COMMAND ./update.sh "${ClickHouse_SOURCE_DIR}/tests/fuzz/all.dict" |
There was a problem hiding this comment.
This reverted rule lost its DEPENDS edge entirely. After the first build, ninja has no reason to rerun ./update.sh when tests/fuzz/all.dict changes, so clickhouse.g and the generated out.cpp can stay stale in an incremental tree and codegen_select_fuzzer silently lags behind the committed dictionary.
If we go back to a checked-in all.dict, this command still needs an explicit DEPENDS on that file, and preferably on update.sh and clickhouse-template.g too.
| "WITH ADMIN OPTION" | ||
| "WITH CHECK" | ||
| "WITH FILL" | ||
| "STALENESS" |
There was a problem hiding this comment.
The checked-in dictionary set is already inconsistent here: tests/fuzz/update_dict.sh defines all.dict as the sort -u merge of the component dictionaries, but keywords.dict now contains "STALENESS" while tests/fuzz/all.dict jumps from "START TRANSACTION" to "STATISTICS" and never includes it. The parser fuzzers only consume all.dict via their .options files, so this keyword is silently missing from the actual fuzz input set. Please regenerate all.dict from the committed pieces (or drop the extra token if it is not supposed to be fuzzed).
|
📊 Cloud Performance Report ✅ AI verdict: no significant changes detected. K_source=6 K_base=30 flagged=0/65 clickbench🟢 No significant changes tpch_adapted_1_official🟢 No significant changes Debug info
|
1 participant
…of committing it"
It breaks fuzzers, see #111025 (comment)
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
...