Add SQLite Input/Output format#49966
Conversation
al13n321
left a comment
There was a problem hiding this comment.
FastTest fails because it builds without sqlite. Wrap everything in #if USE_SQLITE like in StorageSQLite.h et al.
StyleCheck fails for various reasons, e.g. tabs, see output on github. If it's not clear what's wrong on a line, play with the regex in utils/check-style/check-style.
| } | ||
| */ | ||
|
|
||
| std::string select_query = fmt::format("SELECT * FROM {};", table_name); |
There was a problem hiding this comment.
I wonder if we can reuse SQLiteSource (or its parts) here instead of querying and deserializing by hand. It does useful things:
- It selects only the requested columns.
- It applies filters from WHERE when possible. (Currently this is not possible with input formats, but Parquet filter pushdown #52951 makes it possible.)
- It reads results in binary form rather than parsing from text.
There was a problem hiding this comment.
Not done yet, leaving this open. SQLiteSource is built around a real SQLite connection opened from a file path (it can select only the requested columns, push down WHERE, and read values in binary form), whereas this input format reads an arbitrary ReadBuffer through a custom read-only VFS, so the two don't share a connection abstraction today. As a smaller correctness step, NULLs are now handled via sqlite3_column_type/sqlite3_column_bytes rather than text length. Reusing SQLiteSource (column projection, predicate push-down, binary reads) would be a good follow-up.
|
This is not ready, please continue. |
|
Dear @nikitamikhaylov, this PR hasn't been updated for a while. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself. |
# Conflicts: # src/Core/Settings.h # src/Core/SettingsChangesHistory.h # src/Formats/FormatFactory.cpp # src/Formats/registerFormats.cpp
Adapt the format classes to the current code base after merging master: the `IOutputFormat` and `IRowInputFormat` constructors now take a `SharedHeader`, `IOutputFormat::flush` is no longer virtual, and format settings moved to `FormatFactorySettings.h`. Address review feedback from the original review: - Move the read-only VFS implementation out of `SQLiteInputVFS.h` and into `SQLiteInputVFS.cpp`, with the global state in an anonymous namespace and the registration guarded by `std::call_once`, so the VFS is registered exactly once even under concurrent reads. - Pass the `arrow::io::RandomAccessFile` pointer to the VFS as a portable lowercase hex string via `getHexUIntLowercase`/`unhexUInt`, instead of relying on platform-dependent pointer formatting with `operator<<`. - In `SQLiteOutputFormat`, throw a clear exception when the output is not a regular file: only writing to files is supported, and `fstat` is used to rule out pipes, sockets and terminals. - Build the rows in `SQLiteOutputFormat` with a prepared statement and parameter binding instead of concatenating SQL text, which avoids quoting and escaping bugs, and wrap the inserts in a single transaction. - Handle SQL `NULL` on input by checking `sqlite3_column_type` and reading the exact byte length with `sqlite3_column_bytes`. Add a stateless test that round-trips a database through the output and input formats, verifies the produced file is a real SQLite database, reads a database created by the `sqlite3` CLI by table name, and checks that writing to a non-file is rejected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ClickHouse type names such as `Nullable(UInt64)` are not valid SQLite type names, so creating the `result` table with them failed with a syntax error. SQLite is dynamically typed, so the columns are now declared without a type: this gives them no affinity and the values bound as text are stored as is, which round-trips faithfully. Also fix the `sqlite3` CLI quoting in the stateless test and add the reference file. Verified end to end with a freshly built binary: write via the output format, read back via the input format (including NULLs), cross-check with the `sqlite3` CLI, read by table name, and reject writing to a non-file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… in the test Extend `04309_sqlite_format_input_output` with two cases: * Reading a table whose name contains a double quote, verifying the name is correctly quoted as an SQLite identifier. * Providing a structure with the wrong number of columns, verifying the input format rejects it with `INCORRECT_NUMBER_OF_COLUMNS`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pushed
The job failed on a single clang-tidy error in Initialized the local as Review threads addressed
The
|
The test asserts that ProfileEvents['UserThrottlerSleepMicroseconds'] is greater than half of the target query duration. The throttler only sleeps when the natural read rate exceeds the configured throttle limit; when the natural rate drops close to the limit the token bucket never goes negative and the throttler skips its sleep path (see `Throttler::throttle` in `src/Common/Throttler.cpp`). PR ClickHouse#103586 added `no-random-settings` to remove S3-prefetch settings as one source of slow natural rate, but contention on the `Stateless tests (amd_tsan, parallel, 2/2)` runner still occasionally drives the natural S3 read rate close to or below the previous 1 MB/s limit. Post-merge CIDB: `pull_request_number != 0 AND check_start_time > 2026-06-05` shows 4 sightings (PRs ClickHouse#106222, ClickHouse#102039, ClickHouse#49966, ClickHouse#106184) all on amd_tsan parallel 2/2 with the `read 1 1 0` shape (duration ok, bytes ok, sleep below threshold). All four PRs already include the `no-random-settings` tag, so the remaining flakiness is contention-driven, not random-settings driven. Drop the throttle limit and dataset 5x so the throttle is well below any plausible natural rate (200 KB/s vs ~0.9 MB/s observed worst case = 4.5x safety margin). Test wall-clock stays at ~8s. Local 10/10 runs against an S3 disk produced sleep_us between 16.8s and 17.3s, comfortably above the 3.5s threshold. Closes: ClickHouse#103422 Related: ClickHouse#103586 Related: ClickHouse#106184 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The input format used `arrow::io::RandomAccessFile` (via `asArrowFile`) purely as a seekable byte source for the read-only SQLite VFS. This coupled the `SQLite` format to Arrow, which is disabled on some platforms, so the build failed on `amd_freebsd`, `riscv64` and `loongarch64` with `'arrow/io/interfaces.h' file not found`. Replace the Arrow dependency with ClickHouse's own `SeekableReadBuffer`: the VFS now reads through `seek`/`read` and reports the size cached in a small `SQLiteReadSource`. When the input buffer is seekable and its size is known, the database is read with true random access; otherwise (a pipe, a stream of unknown length) it is loaded into a `ReadBufferFromOwnString`. Also fix two NULL-handling issues in `readRow`: - A SQLite `TEXT` value equal to the textual null marker (e.g. `NULL` or `ᴺᵁᴸᴸ`) in a `Nullable` column was mistaken for SQL `NULL`, because `deserializeWholeText` on a `Nullable` serialization treats the marker as null. Known-non-null values are now deserialized through the nested (non-nullable) serialization. - SQL `NULL` in a non-nullable column silently became the type default, ignoring `input_format_null_as_default`. It now throws `INCORRECT_DATA` when the setting is disabled, matching other input formats. The stateless test was renumbered to `04326` (the `04309` prefix collided with tests merged from `master`) and extended with both NULL cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`03251_insert_sparse_all_formats` and `02187_async_inserts_all_formats` iterate over every format that is both input and output and round-trip data through an HTTP socket / async-insert pipe. The `SQLite` format cannot participate: its output requires a regular, seekable file (writing to a pipe or socket throws `NOT_IMPLEMENTED`) and reading requires random access, so it cannot stream. Add `SQLite` to the existing exclusion lists, next to the other formats that cannot do a generic streaming round-trip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pushed Build on The input format used
Both iterate over every input+output format and round-trip through an HTTP socket / async-insert pipe. Review threads
The The stateless test was renumbered |
The style check flagged a `catch (...)` in `memRead` that silently swallowed exceptions. Save the exception into `SQLiteReadSource` instead, so the caller of the SQLite API can rethrow the original error (e.g. a network read failure) rather than reporting a generic SQLite I/O error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Rethrow the exception saved by the VFS read callback in the format's error paths, so the original error is reported instead of a generic SQLite one. - Handle `LowCardinality(Nullable)` columns: use `isNullableOrLowCardinalityNullable` instead of `isNullable`, insert SQL NULL as NULL, and deserialize known-non-NULL values with the non-nullable nested serialization (a text value equal to the textual null marker is not mistaken for SQL NULL). - When `input_format_null_as_default` consumes a NULL for a non-nullable column, mark the column as not read in `RowReadExtension`, so that `AddingDefaultsTransform` computes the table DEFAULT expression for it. - Honor `input_format_allow_seeks` (`FormatSettings::seekable_read`): when disabled, buffer the whole database in memory instead of seeking. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed two commits:
The extended test passes against a local build. |
`sqlite3_open_v2` can leave the database handle non-null even when it returns an error, and that handle still has to be released with `sqlite3_close_v2`. Previously the pointer was wrapped into the owning `db` only after the status check, so every failed open leaked the connection object; malformed input files could hit this path repeatedly. Wrap the handle into `db` immediately after the call so the connection is released on every path. The same fix is applied in both `SQLiteInputFormat` and `SQLiteOutputFormat`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Merged @al13n321, your earlier |
`SQLiteOutputFormat::writePrefix` is not just a prefix: it opens the SQLite connection, creates the `result` table, starts the transaction and prepares the insert statement. Because the format was registered without `markFormatHasNoAppendSupport`, `StorageFile` treated an existing non-empty `.db` file as appendable and called `doNotWritePrefix` before writing, so the next `consume` ran with `db`/`insert_stmt` unset and a second `INSERT INTO FUNCTION file(..., SQLite)` against the same file failed in a confusing way. Mark `SQLite` as not append-supported, like `Parquet`/`ORC`, so `StorageFile` produces the standard `CANNOT_APPEND_TO_FILE` error. Add a regression to `04326_sqlite_format_input_output` with two inserts into the same file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pushed
The |
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered by tests: 231/313 (73.80%) | Lost baseline coverage: none · Uncovered code |

Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add supports of SQLite input/output format (reading and saving tables from/to SQLite3 .db files).
Documentation entry for user-facing changes