Repository navigation
Honour SQL_NULL_DATA when a parameter value buffer is bound - #586
Conversation
A parameter was sent as NULL only when its value pointer was null. An application that keeps a value buffer bound and signals NULL through the length/indicator buffer -- the shape that reusing one buffer per column for every row produces -- got an empty string instead: '' stored in a String column, and a server-side parse error for Int32, Float64, Date, Decimal and Bool. Read the indicator alongside the value pointer, so that SQL_NULL_DATA sends \N as the query parameter and makes its data source type Nullable. The conversion helpers in type_info.h already honour the indicator; the two places in Statement that decide whether a parameter is NULL did not. Extend statement_parameter_binding_ut with the bound-buffer cases for an Int32 and a String parameter, and statement_parameter_bindings_it with NullValueForIntegerWithBoundBuffer and NullValueForStringWithBoundBuffer, which run SELECT isNull(?) with a value buffer bound.
|
Thank you for the review and the merge — much appreciated. For anyone finding this later: with a value buffer bound and the length/indicator set to I will re-run the reproducer from #582 against the first release that carries this and confirm here. On the rest of #582: the second defect — parameter arrays stopping after the first set, because Context on where the report came from: adbcBridge (https://github.com/singhpratech/adbcbridge) is a driver for ADBC, Apache Arrow's database connectivity API, that works over any ODBC driver, and binding a column of values with NULLs in it is the everyday path through it — which is how both of these turned up. |
A parameter array stopped after the first set on a statement that returns nothing. SQLExecute sent set 0 and left the rest to SQLMoreResults, which the driver uses to send one set per call; but an INSERT produces no result sets, so a conforming caller has no reason to call SQLMoreResults at all and the remaining sets were never sent. A five-set array inserted one row, under SQL_SUCCESS and with no diagnostic. ODBC has the driver execute the statement once per parameter set during SQLExecute; SQLMoreResults then walks the result sets those executions produced. executeQuery now keeps sending sets while the last one produced no result set, and stops as soon as one does, so the one-result-set-per- SQLMoreResults behaviour a SELECT with a parameter array relies on is unchanged. SQL_ATTR_PARAMS_PROCESSED_PTR was also assigned next_param_set_idx before the set was sent, while that index still named the set about to go, so it reported one less than the number processed: 0 after a five-set INSERT and 2 after a three-set SELECT. It is now written after the send, where the index equals the count completed. The existing TODO about output parameters is kept. Verified against ClickHouse 26.7.5.10 with two standalone ODBC programs: - INSERT, 5 sets: 1 row and params_processed=0 before, 5 and 5 after. - SELECT ? , 3 sets: 3 result sets in order both before and after, with params_processed going from 2 to 3. The four tests from #586 also still pass on this build.
This is the first of the two parameter-binding defects I reported in #582; the parameter-array
one is separate and I would be glad to follow up with it. I have left out the closing keyword
so the issue stays open for that second half.
What a caller sees. ODBC lets an application keep a value buffer bound and say "this row's
value is NULL" through the length/indicator buffer, which is what reusing one buffer per column
across rows amounts to. The driver only treats a parameter as NULL when the value pointer itself
is null, so with a buffer bound it sends an empty string. Into a
Nullable(Int32)column that isa server-side error:
Float64,Date,DecimalandBoolfail the same way.Stringis worse, because itsucceeds: the row is stored as
''rather than NULL, andDateTime64becomes the epoch. Anapplication cannot work around it by describing the parameter first either, since
SQL_DESCRIBE_PARAMETERisN.The cause. Two places in
Statementdecide nullability from the value pointer alone andnever look at the indicator:
driver/statement.cpp:87—if (binding_info.value == nullptr) value = "\\N";, so with abuffer bound the value goes through
readReadyDataTo(), whoseto_null()for astd::stringdestination yields an empty string.
driver/statement.cpp:336—type_info.is_nullable = (binding_info.is_nullable || binding_info.value == nullptr);,so the query parameter's data source type stays non-nullable; for the types that default to
SQL_NO_NULLS(the string-mapped ones) even a correct\Nwould be rejected.The conversion helpers in
driver/utils/type_info.halready readSQL_NULL_DATAfrom theindicator (
from_buffer<...>::to_value<...>::convert); only these two decisions did not.The change. Add a small
isNullParamValue()helper indriver/statement.cppthat treats aparameter as NULL when the value pointer is null or the indicator is
SQL_NULL_DATA, and useit in both places. A NULL value pointer keeps working exactly as before.
Test.
driver/test/statement_parameter_binding_ut.cppgains the bound-buffer cases for anInt32and aStringparameter, checking the query parameter is\Nand the placeholder typeis nullable; the existing
NullValueBindingasserted the empty string, so its assertion iscorrected, and a
NullValueBindingWithoutBuffercase pins the NULL-pointer path that alreadyworked.
driver/test/statement_parameter_bindings_it.cppgainsNullValueForIntegerWithBoundBufferandNullValueForStringWithBoundBuffer, which runSELECT isNull(?)with the value buffer bound and the indicator set toSQL_NULL_DATA. Allfour fail on master and pass with the change. Locally
clickhouse-odbc-utgoes from 545/545 to547/547 and
clickhouse-odbc-client-itfrom 387/388 to 389/390, the one failure in both runsbeing
AuthenticationTest.PasswordEncoding, which needs a ClickHouse admin account my testserver does not have.
Found while verifying ClickHouse for adbcBridge,
an ADBC driver over ODBC, where a row of bound parameters with a NULL in it is the ordinary case.