feat: cover null-typed bind parameter columns#257
Conversation
Adds a test that binds a batch whose parameter column has Arrow type null and asserts NULL is bound per row. Signed-off-by: Fredrik Fornwall <fredrik@fornwall.net>
fornwall
changed the title
| cursor.adbc_statement.set_sql_query( | ||
| f"INSERT INTO {sample_table} ({id_}, {value}) " | ||
| f"VALUES ({driver.bind_parameter(1)}, {driver.bind_parameter(2)})" | ||
| ) |
There was a problem hiding this comment.
Shouldn't these queries be overridable?
There was a problem hiding this comment.
I was thinking that queries should be overridable when there is a need (an upfront knowledge if the sql is not portable, or if a driver has found a need to override it), and not every query needs to be overridable.
Not every existing query is overridable.
But let me know if you think every new query should be overridable (or at least if this new one should), and I'll add it here!
| result = pyarrow.RecordBatchReader._import_from_c(handle.address).read_all() | ||
| assert result[0].to_pylist() == [2, 3, 4, 5] | ||
|
|
||
| @pytest.mark.requires_features(["statement_bind"]) |
There was a problem hiding this comment.
This also uses prepare, albeit bind probably implies prepare already
There was a problem hiding this comment.
Right, no drawback with being explicit? Added a dependency on statement_prepare: 70de981
| value = driver.quote_identifier("value") | ||
| parameters = pyarrow.RecordBatch.from_pydict( | ||
| { | ||
| "0": pyarrow.array([7001], type=pyarrow.int64()), |
There was a problem hiding this comment.
Why do we need this column anyways?
There was a problem hiding this comment.
I reused the sample_table test fixture (defined here), instead of creating a new one. I think that makes sense, but let me know if you think otherwise!
2 participants
Add a test that binds a batch whose parameter column has the Arrow type null.
Verified that existing drivers for sqlite, postgres, mysql, duckdb and flightsql passes.
Context: Had a bug/confusion around this myself when implementing a new driver, so this test would have helped.