forked from duckdb/duckdb
-
Notifications
You must be signed in to change notification settings - Fork 0
Add RapidCheck property-based tests that find real DuckDB bugs #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
peterxcli
wants to merge
2
commits into
main
Choose a base branch
from
property-tests-rapidcheck
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| cmake_minimum_required(VERSION 3.15) | ||
| project(duckdb_property_tests CXX) | ||
|
|
||
| # Property-based tests for DuckDB using RapidCheck | ||
| # (https://github.com/emil-e/rapidcheck). | ||
| # | ||
| # This is a standalone project that links against an existing DuckDB build | ||
| # directory: | ||
| # | ||
| # cmake -S test/property -B build/property -DDUCKDB_BUILD_DIR=build/relassert | ||
| # cmake --build build/property RC_PARAMS="max_success=500" | ||
| # build/property/property_test | ||
| # | ||
| # RapidCheck is fetched via FetchContent unless RAPIDCHECK_SOURCE_DIR points to | ||
| # a checkout. | ||
|
|
||
| set(CMAKE_CXX_STANDARD 17) | ||
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | ||
| set(CMAKE_EXPORT_COMPILE_COMMANDS ON) | ||
|
|
||
| get_filename_component(DUCKDB_ROOT "${CMAKE_CURRENT_SOURCE_DIR}/../.." ABSOLUTE) | ||
| set(DUCKDB_BUILD_DIR | ||
| "${DUCKDB_ROOT}/build/relassert" | ||
| CACHE PATH "DuckDB build directory containing src/libduckdb") | ||
| set(RAPIDCHECK_SOURCE_DIR | ||
| "" | ||
| CACHE PATH "Local RapidCheck checkout (optional)") | ||
| option( | ||
| PROPERTY_TEST_SANITIZE | ||
| "Build with -fsanitize=address,undefined (required when linking a sanitized libduckdb)" | ||
| ON) | ||
|
|
||
| if(NOT EXISTS "${DUCKDB_BUILD_DIR}/src") | ||
| message( | ||
| FATAL_ERROR | ||
| "DUCKDB_BUILD_DIR=${DUCKDB_BUILD_DIR} does not look like a DuckDB build directory" | ||
| ) | ||
| endif() | ||
|
|
||
| # Locate the shared library | ||
| find_library( | ||
| DUCKDB_LIBRARY | ||
| NAMES duckdb | ||
| PATHS "${DUCKDB_BUILD_DIR}/src" | ||
| NO_DEFAULT_PATH) | ||
| if(NOT DUCKDB_LIBRARY) | ||
| message(FATAL_ERROR "Could not find libduckdb in ${DUCKDB_BUILD_DIR}/src") | ||
| endif() | ||
|
|
||
| # RapidCheck | ||
| if(RAPIDCHECK_SOURCE_DIR) | ||
| add_subdirectory("${RAPIDCHECK_SOURCE_DIR}" | ||
| "${CMAKE_CURRENT_BINARY_DIR}/rapidcheck" EXCLUDE_FROM_ALL) | ||
| else() | ||
| include(FetchContent) | ||
| FetchContent_Declare( | ||
| rapidcheck | ||
| GIT_REPOSITORY https://github.com/emil-e/rapidcheck.git | ||
| GIT_TAG master | ||
| GIT_SHALLOW TRUE) | ||
| FetchContent_MakeAvailable(rapidcheck) | ||
| set(RAPIDCHECK_SOURCE_DIR "${rapidcheck_SOURCE_DIR}") | ||
| endif() | ||
|
|
||
| # RapidCheck's Catch adapter includes <catch2/catch.hpp>; DuckDB vendors Catch | ||
| # v2 at third_party/catch/catch.hpp. | ||
| file(MAKE_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/catch_shim/catch2") | ||
| file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/catch_shim/catch2/catch.hpp" | ||
| "#include \"${DUCKDB_ROOT}/third_party/catch/catch.hpp\"\n") | ||
|
|
||
| file(GLOB PROPERTY_TEST_SOURCES "${CMAKE_CURRENT_SOURCE_DIR}/*.cpp") | ||
| add_executable(property_test ${PROPERTY_TEST_SOURCES}) | ||
| target_include_directories( | ||
| property_test | ||
| PRIVATE "${CMAKE_CURRENT_SOURCE_DIR}/include" | ||
| "${DUCKDB_ROOT}/src/include" | ||
| "${DUCKDB_ROOT}/third_party/catch" | ||
| "${DUCKDB_ROOT}/third_party/fmt/include" | ||
| "${CMAKE_CURRENT_BINARY_DIR}/catch_shim" | ||
| "${RAPIDCHECK_SOURCE_DIR}/extras/catch/include") | ||
| target_link_libraries(property_test PRIVATE rapidcheck "${DUCKDB_LIBRARY}") | ||
| target_compile_definitions(property_test | ||
| PRIVATE DUCKDB_ROOT_DIR="${DUCKDB_ROOT}") | ||
| if(PROPERTY_TEST_SANITIZE) | ||
| target_compile_options(property_test PRIVATE -fsanitize=address,undefined | ||
| -fno-omit-frame-pointer) | ||
| target_link_options(property_test PRIVATE -fsanitize=address,undefined) | ||
| endif() | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,146 @@ | ||
| # Bugs and quirks found by the property tests | ||
|
|
||
| Found against `v1.6.0-dev13151` (relassert build, macOS arm64). Ordered roughly by severity. | ||
| Each entry that the tests must work around is referenced by a `KNOWN ISSUE` comment in the test code. | ||
|
|
||
| ## 1. `last_day` at the maximum date raises an INTERNAL error (invalidates the database) | ||
|
|
||
| ```sql | ||
| SELECT last_day(DATE '5881580-07-10'); | ||
| -- INTERNAL Error: Scalar function ""last_day"" threw an execution error, | ||
| -- but the function is not marked as fallible - the function must call SetFallible(). | ||
| -- Error: Date out of range: 5881580-8-1 | ||
| ``` | ||
|
|
||
| `last_day` computes first-of-next-month, which is out of range in the maximum month. Because the function is | ||
| not marked fallible, the recoverable conversion error escalates to an `InternalException`, which invalidates | ||
| the whole database. Fix: mark the function fallible (or compute the result without materializing the | ||
| out-of-range date). | ||
|
|
||
| ## 2. `date_trunc('week', ...)` near the minimum date: signed integer overflow (UB) | ||
|
|
||
| ```sql | ||
| SELECT date_trunc('week', DATE '5877642-06-25 (BC)'); | ||
| SELECT date_trunc('isoyear', DATE '5877642-06-25 (BC)'); -- same overflow | ||
| -- UBSan: src/include/duckdb/common/types/date.hpp:58: signed integer overflow: -2147483646 - 3 | ||
| ``` | ||
|
|
||
| `date_t::operator-` does raw `int32` arithmetic; the week/isoyear truncation paths subtract the weekday offset | ||
| without a range check. Undefined behavior in release builds; aborts under `-fsanitize=undefined -fno-sanitize-recover`. | ||
|
|
||
| ## 3. TIMETZ offsets with zero minutes but nonzero seconds format incorrectly | ||
|
|
||
| ```sql | ||
| SELECT '12:00:00-05:00:59'::TIMETZ; | ||
| -- prints 12:00:00-05:59 (which denotes offset -05:59:00, a different value) | ||
| ``` | ||
|
|
||
| `StringCast::Operation(dtime_tz_t)` (src/common/operator/string_cast.cpp) only prints the minutes field | ||
| `if (mm)`, but prints seconds independently, so `-05:00:59` collapses to `-05:59`. Displayed values are wrong | ||
| and the text round trip yields a different value. Fix: print minutes whenever minutes or seconds are nonzero. | ||
|
|
||
| ## 4. `list_sort` orders INTERVALs inconsistently with comparisons and ORDER BY | ||
|
|
||
| ```sql | ||
| SELECT INTERVAL '31 days' > INTERVAL '1 month'; -- true | ||
| SELECT i FROM (VALUES (INTERVAL '31 days'), (INTERVAL '1 month')) t(i) ORDER BY i; | ||
| -- 1 month, 31 days | ||
| SELECT list_sort([INTERVAL '31 days', INTERVAL '1 month']); | ||
| -- [31 days, 1 month] <- disagrees with both | ||
| ``` | ||
|
|
||
| `create_sort_key` encodes intervals by raw `(months, days, micros)` without the normalization | ||
| (1 month = 30 days, 1 day = 24 h) that comparisons, ORDER BY, aggregates and window functions apply. | ||
| Affects `list_sort`, `list_reverse_sort`, `list_grade_up`, `list_distinct` and any other `create_sort_key` consumer. | ||
|
|
||
| ## 5. `Value::ToSQLString()` produces unparseable/wrong SQL for several cases | ||
|
|
||
| - STRUCT keys are not escaped: `Value::STRUCT({{"k'", ...}})` renders as `{'k'': ...}` → parser error. | ||
| (src/common/types/value.cpp, `ret += "'" + name + "': "`) | ||
| - BIT values are rendered bare: `Value::BIT("10")` renders as `10`, which re-parses as INTEGER and casts to a | ||
| 32-bit bit string. Should be `'10'::BIT`. | ||
| - UNION values render as `union_value(tag := x)`, which loses the other members; | ||
| `SELECT [union_value(a := 1), union_value(b := 'x')]` then fails to bind. | ||
|
|
||
| ## 6. INTERVAL text output cannot always be parsed back (hours ≥ 10 digits) | ||
|
|
||
| ```sql | ||
| SELECT to_microseconds(9223372036854775807)::VARCHAR; -- '2562047788:00:54.775807' | ||
| SELECT '2562047788:00:54.775807'::INTERVAL; -- Conversion Error | ||
| ``` | ||
|
|
||
| `Interval::ToString` emits up to 10-digit hour counts, but `Time::TryConvertInternal` | ||
| (src/common/types/time.cpp, "Allow up to 9 digit hours") rejects more than 9 digits. | ||
|
|
||
| ## 7. `list_contains` and `list_position` disagree on NULL semantics | ||
|
|
||
| ```sql | ||
| SELECT list_contains([false, true, NULL], NULL); -- NULL | ||
| SELECT list_position([false, true, NULL], NULL); -- 3 | ||
| ``` | ||
|
|
||
| `list_position` treats a NULL needle as matchable (IS NOT DISTINCT semantics), `list_contains` propagates NULL. | ||
|
|
||
| ## 8. Week-based date parts fail on the minimum year | ||
|
|
||
| ```sql | ||
| SELECT weekofyear(DATE '5877642-06-25 (BC)'); | ||
| -- Conversion Error: Date out of range: -5877641-1-1 | ||
| ``` | ||
|
|
||
| `weekofyear`/`isoyear`/`yearweek` (and `date_trunc('year'/'quarter'/'month')`) materialize Jan 1 of the year | ||
| as a `date_t`, which is out of range for dates in the minimum year even though the requested result is | ||
| representable. | ||
|
|
||
| ## 9. Sort binds `decode_sort_key` by round-tripping types through SQL strings | ||
|
|
||
| `Sort::Sort` passes column types as strings that `DecodeSortKeyBind` re-parses with `Parser::ParseColumnList` | ||
| (src/function/scalar/create_sort_key.cpp). Types whose string form is not parseable break sorting; e.g. an | ||
| API-created ENUM with a value containing a NUL byte (possible via Arrow dictionaries): | ||
|
|
||
| ``` | ||
| list_sort(<LIST(ENUM('a', chr(0), 'b'))>) | ||
| -- Invalid Input Error: Value "ENUM('a', chr(0), 'b')" can not be converted to a DuckDB Type | ||
| ``` | ||
|
|
||
| ## 10. JSON → DECIMAL loses precision (goes through a double) | ||
|
|
||
| ```sql | ||
| SELECT '883406386745030.3'::JSON::DECIMAL(16,1); -- 883406386745030.2 | ||
| ``` | ||
|
|
||
| JSON numbers with a fractional part are parsed as doubles before conversion to DECIMAL; parsing the raw JSON | ||
| number text directly into DECIMAL would be exact. | ||
|
|
||
| ## 11. `INT_MIN % -1` errors instead of returning 0 | ||
|
|
||
| ```sql | ||
| SELECT (-32768)::SMALLINT % (-1)::SMALLINT; | ||
| -- Out of Range Error: Overflow in division of -32768 / -1 | ||
| ``` | ||
|
|
||
| The mathematical result (0) is representable (PostgreSQL returns 0); the error message also says "division" | ||
| for a modulo. | ||
|
|
||
| ## 12. `levenshtein`/`damerau_levenshtein`/`hamming` operate on bytes, not characters | ||
|
|
||
| ```sql | ||
| SELECT levenshtein('中', 'a'); -- 3 (bytes), documentation says "single-character edits" | ||
| ``` | ||
|
|
||
| Multi-byte UTF-8 characters count once per byte. (Also: `damerau_levenshtein` implements the unrestricted | ||
| distance, not the more common optimal-string-alignment variant, and `hamming` rejects empty strings.) | ||
|
|
||
| --- | ||
|
|
||
| ### Notable non-bugs the tests must account for (DuckDB semantics) | ||
|
|
||
| - the nested-value text format is lossy by design (strings inside lists/structs are not quoted), so only | ||
| "safe" strings round trip through `CAST(v AS VARCHAR)` inside nested types; | ||
| - `VARCHAR → UNION` casts only try members that VARCHAR *implicitly* casts to; | ||
| - `substring` counts negative starts from the end and interprets negative lengths as a window before the start; | ||
| - prepared-statement parameters may bind to wider types than the passed value (`-$1` with a SMALLINT parameter | ||
| binds INTEGER negation); | ||
| - `epoch_ms` rounds half away from zero rather than flooring; | ||
| - `millisecond()`/`microsecond()` include the seconds component (PostgreSQL semantics); | ||
| - HUGEINT and UHUGEINT literals in one list unify to DOUBLE (documented promotion rule). |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| # Property-based tests (RapidCheck) | ||
|
|
||
| Property-based tests for DuckDB using [RapidCheck](https://github.com/emil-e/rapidcheck), in the spirit of | ||
| Rust's `proptest` (compare [apache/arrow-rs#10352](https://github.com/apache/arrow-rs/pull/10352)). | ||
|
|
||
| Instead of fixed inputs, each test states a *property* ("any value must survive a cast to VARCHAR and back", | ||
| "`LIKE` must agree with a 20-line reference matcher", "integer arithmetic must error exactly when the `__int128` | ||
| result is out of range") and RapidCheck runs it against hundreds of randomly generated inputs, shrinking failures | ||
| to minimal counterexamples. | ||
|
|
||
| ## Building | ||
|
|
||
| This is a standalone CMake project that links against an existing DuckDB build directory, so iterating on tests | ||
| does not require rebuilding DuckDB: | ||
|
|
||
| ```bash | ||
| # build DuckDB with assertions + sanitizers (recommended for bug hunting) | ||
| GEN=ninja CORE_EXTENSIONS='json' make relassert | ||
|
|
||
| # configure + build the property tests | ||
| cmake -S test/property -B build/property -G Ninja -DDUCKDB_BUILD_DIR=$PWD/build/relassert | ||
| cmake --build build/property | ||
| ``` | ||
|
|
||
| RapidCheck is fetched via CMake `FetchContent` by default; pass `-DRAPIDCHECK_SOURCE_DIR=/path/to/rapidcheck` | ||
| to use a local checkout. When linking a sanitized DuckDB build the tests are built with | ||
| `-fsanitize=address,undefined` as well (`-DPROPERTY_TEST_SANITIZE=OFF` to disable). | ||
|
|
||
| ## Running | ||
|
|
||
| The binary is a regular Catch2 test runner; RapidCheck is configured through the `RC_PARAMS` environment variable: | ||
|
|
||
| ```bash | ||
| # run everything (100 cases per property by default) | ||
| build/property/property_test "[property]" | ||
|
|
||
| # more iterations, bounded value sizes | ||
| RC_PARAMS="max_success=1000 max_size=50" build/property/property_test "[property]" | ||
|
|
||
| # one suite | ||
| build/property/property_test "[strings]" | ||
|
|
||
| # reproduce a failure deterministically | ||
| RC_PARAMS="seed=12345" build/property/property_test "VARCHAR cast round trip" | ||
|
|
||
| # disable shrinking (useful for a fast first survey; shrinking large nested values can be slow) | ||
| RC_PARAMS="max_success=1000 noshrink=1" build/property/property_test "[property]" | ||
| ``` | ||
|
|
||
| Set `PROPERTY_TEST_TMP` to control where `[storage]` writes its temporary database files (default `/tmp`). | ||
|
|
||
| ## Layout | ||
|
|
||
| | file | contents | | ||
| |---|---| | ||
| | `include/property_test.hpp` | `PropDB` (in-memory/persistent connection helpers), value comparison, assertion macros, generator declarations | | ||
| | `generators.cpp` | random `LogicalType`s (incl. nested STRUCT/LIST/ARRAY/MAP/UNION/ENUM) and random `Value`s for any type, plus UTF-8 string, numeric and temporal generators with adversarial special values | | ||
| | `test_roundtrip.cpp` | value → VARCHAR/SQL-literal/JSON/table → value round trips | | ||
| | `test_strings.cpp` | LIKE/ILIKE vs a reference matcher; substring/split/pad/trim/translate/distance/encoding functions vs oracles | | ||
| | `test_lists.cpp` | list_sort/contains/position/distinct/slice/resize/concat/aggregates, range/generate_series vs closed forms | | ||
| | `test_arithmetic.cpp` | integer/HUGEINT/DECIMAL arithmetic vs `__int128` oracles (overflow must error exactly when out of range) | | ||
| | `test_datetime.cpp` | date parts vs independent civil-calendar algorithms, epoch round trips, strftime/strptime | | ||
| | `test_storage.cpp` | persistent DB round trip across `force_compression` settings, incl. update/delete + checkpoint + reopen | | ||
|
|
||
| ## Known issues found by these tests | ||
|
|
||
| Failures caused by already-identified DuckDB bugs are skipped with `RC_PRE(...)` guards marked `KNOWN ISSUE`, | ||
| so the properties keep hunting for new bugs. See `FINDINGS.md` for the list of bugs these tests have found, | ||
| with minimal reproductions. | ||
|
|
||
| ## Writing a new property | ||
|
|
||
| ```cpp | ||
| TEST_CASE("my property", "[property][mytag]") { | ||
| PropDB db; | ||
| rc::prop("what must hold", [&] { | ||
| auto type = *GenType(2); // random type, nested up to depth 2 | ||
| auto v = *GenValue(type, 0.1); // random value, 10% NULLs | ||
| auto out = db.Scalar("SELECT ...", {v}); | ||
| PROP_ASSERT_VALUES_EQUAL(out, v); | ||
| }); | ||
| } | ||
| ``` | ||
|
|
||
| Gotchas: | ||
| - `RC_PRE`/`RC_ASSERT` use expression decomposition, which silences `||`/`&&` short-circuiting — | ||
| compute the condition into a `bool` first. | ||
| - prefer an *independent oracle* (a small reference implementation) over comparing DuckDB with itself; | ||
| where that is impossible, comparing two DuckDB code paths (constant vs parameterized, stored vs computed) | ||
| still finds inconsistencies. | ||
| - `duckdb::vector` (pulled in by `using namespace duckdb`) bounds-checks `back()`/`operator[]` and throws | ||
| `InternalException` — do not index blindly in test code. |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
RAPIDCHECK_SOURCE_DIRis not supplied, every clean configuration fetches whatever commitmasterreferences at that moment. An upstream API change, force-push, or regression can consequently make the same DuckDB commit stop building or change its property-generation behavior without any repository change. Use a specific commit or release tag so the test harness remains reproducible.Useful? React with 👍 / 👎.