Skip to content

Do not bulk insert a VALUES list holding expressions - #1004

Closed
Kayvan-Zahiri wants to merge 2 commits into
ClickHouse:mainfrom
Kayvan-Zahiri:fix/934-bulk-insert-expressions
Closed

Do not bulk insert a VALUES list holding expressions#1004
Kayvan-Zahiri wants to merge 2 commits into
ClickHouse:mainfrom
Kayvan-Zahiri:fix/934-bulk-insert-expressions

Conversation

@Kayvan-Zahiri

Copy link
Copy Markdown

Closes #934.

_try_bulk_insert checked only that the statement contained VALUES, never what
the values list held. The block writer carries one value per named column, so an
expression in there is lost:

statement before after
VALUES (%s, hex(%s)) raw parameter written to the column, no error row by row, server evaluates hex
VALUES (%s, hex(unhex('AB'))) Insert data column count does not match column names row by row, inserts correctly

The first case is the bad one: wrong data, silently.

A values list now has to be parameter slots only. That means pyformat
placeholders and also server side {name:Type} bindings, which the existing
test_executemany_bulk_insert_keeps_percents_for_server_side_statements covers
and which my first attempt broke. Anything else falls through to the row by row
path, which already substitutes and lets the server evaluate the expression.

One case worth calling out: a bare VALUES with no list at all keeps the fast
path, because that is what the SQLAlchemy dialect emits and there is no
expression to lose. test_cursor_bulk_insert_forwards_settings caught that.

Ten parametrized tests added, five for the fallback and five pinning the fast
path. pytest tests/unit_tests goes from 1453 to 1463 passing with the 37
pre-existing failures unchanged.

_try_bulk_insert checked only that the statement contained VALUES, never
what the values list held. The block writer carries one value per named
column, so a function call was dropped: VALUES (%s, hex(%s)) wrote the
raw parameter into the column with no error, and VALUES (%s, hex(unhex(
'AB'))) raised a misleading column count error instead of falling back.

The values list must now be parameter slots only, either pyformat
placeholders or server side {name:Type} bindings, and anything else
falls through to the row by row path that substitutes and lets the
server evaluate it. A bare VALUES with no list keeps the fast path,
since that is what the SQLAlchemy dialect emits.

Closes ClickHouse#934
@joe-clickhouse

Copy link
Copy Markdown
Contributor

Closing this. After review it does fix the reported function-expression examples, but it does not adequately address the root cause. This effectively replaces one lossy boolean ("Values" in temp) with a slightly less lossy one but the actual defect is that the bulk path discards the original SQL and builds a Native block without proving the two operations are equivalent. The new regex still allows incorrect writes for reordered or repeated named parameters, mappings with different key order, and valid Map literals. It also drops clauses such as SETTINGS. Separately, the statements it does reroute now report rowcount == 0 instead of the written row count.

A solid fix needs a fail-closed insert parser or planner of some kind. It should recognize only a safe subset, preserve the ordered relationship between target columns and parameter slots, validate row cardinality, reject or preserve modifiers, and fall back whenever equivalence cannot be guaranteed. The regression tests also need to verify stored values and rowcount against a live server, not only which mock method was called.

I am leaving #934 open because the underlying bug still does exist and I am happy to discuss a fresh design there if you want to work through the details before implementing another patch. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DB-API executemany bulk insert path silently drops function calls and other expressions in the VALUES list

2 participants