Follow ups for functions stress test by alexey-milovidov · Pull Request #100270 · ClickHouse/ClickHouse · GitHub
Skip to content

Follow ups for functions stress test#100270

Merged
alexey-milovidov merged 51 commits into
masterfrom
follow-up-functions-stress-test-93543
Apr 10, 2026
Merged

Follow ups for functions stress test#100270
alexey-milovidov merged 51 commits into
masterfrom
follow-up-functions-stress-test-93543

Conversation

@alexey-milovidov

@alexey-milovidov alexey-milovidov commented Mar 21, 2026

Copy link
Copy Markdown
Member

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

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

  • Merged into: 26.4.1.796

alexey-milovidov and others added 3 commits March 21, 2026 10:18
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>
@clickhouse-gh

clickhouse-gh Bot commented Mar 21, 2026

Copy link
Copy Markdown
Contributor

@clickhouse-gh clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Mar 21, 2026
Comment thread src/Functions/readWkb.cpp Outdated
…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>
Comment thread src/Core/SettingsChangesHistory.cpp Outdated
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>
@CLAassistant

CLAassistant commented Mar 21, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@alexey-milovidov alexey-milovidov force-pushed the follow-up-functions-stress-test-93543 branch from fb2b966 to 4bc9d3b Compare March 21, 2026 22:21
Comment thread src/Functions/h3GetIndexesFromUnidirectionalEdge.cpp
`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>
Comment thread src/Functions/randDistribution.cpp
alexey-milovidov and others added 6 commits March 23, 2026 00:39
…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>
Comment thread src/Functions/tests/gtest_functions_stress.cpp Outdated
alexey-milovidov and others added 3 commits March 23, 2026 10:58
- 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>
Comment thread src/Functions/tests/gtest_functions_stress.cpp Outdated
alexey-milovidov and others added 4 commits March 30, 2026 11:28
…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>
@nihalzp nihalzp self-assigned this Mar 31, 2026
Comment thread src/Functions/tests/gtest_functions_stress.cpp
Comment thread src/Functions/tests/gtest_functions_stress.cpp Outdated
Comment thread src/Functions/tests/gtest_functions_stress.cpp Outdated
al13n321 and others added 2 commits April 6, 2026 14:34
- 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 al13n321 assigned al13n321 and unassigned nihalzp 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>
Comment thread src/Functions/randDistribution.cpp Outdated
alexey-milovidov and others added 3 commits April 6, 2026 17:54
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).",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@alexey-milovidov

Copy link
Copy Markdown
Member Author

The failures of "Flaky check" in "functions_bad_arguments" will be fixed by #101994.

…-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
@alexey-milovidov

Copy link
Copy Markdown
Member Author

The flaky check failure is fixed in #102148, let's update the branch.

@clickhouse-gh

clickhouse-gh Bot commented Apr 10, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 84.00% 84.00% +0.00%
Functions 90.90% 90.90% +0.00%
Branches 76.50% 76.60% +0.10%

Changed lines: 94.20% (601/638) | lost baseline coverage: 15 line(s) · Uncovered code

Full report · Diff report

@alexey-milovidov alexey-milovidov added this pull request to the merge queue Apr 10, 2026
Merged via the queue into master with commit 54f4893 Apr 10, 2026
164 checks passed
@alexey-milovidov alexey-milovidov deleted the follow-up-functions-stress-test-93543 branch April 10, 2026 10:56
@robot-ch-test-poll robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label Apr 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants