Skip to content

feat: cover dictionary-encoded bind parameter columns#256

Open
fornwall wants to merge 3 commits into
adbc-drivers:mainfrom
fornwall:test/bind-dictionary
Open

feat: cover dictionary-encoded bind parameter columns#256
fornwall wants to merge 3 commits into
adbc-drivers:mainfrom
fornwall:test/bind-dictionary

Conversation

@fornwall

@fornwall fornwall commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Add a test for binds a dictionary-encoded column (what pandas often produce).

Dictionary encoding is an encoding of the same logical values, not a different logical type (Arrow columnar format, Dictionary-encoded Layout), so a driver that binds plain string columns should accept the dictionary-encoded equivalent, decoding it if the database has no native counterpart:

https://arrow.apache.org/docs/format/Columnar.html#dictionary-encoded-layout

Verified against existing drivers: sqlite, postgres, mysql and flightsql (for the example server it needed a fix: apache/arrow-go#936).

Context: Missed this when implementing a new driver, so having this test would have helped.

Add a test for binds a dictionary-encoded string column (what pandas
often produce).

Dictionary encoding is an encoding of the same logical values, not a
different logical type (Arrow columnar format, Dictionary-encoded
Layout), so a driver that binds plain string columns should accept the
dictionary-encoded equivalent, decoding it if the database has no
native counterpart:

> https://arrow.apache.org/docs/format/Columnar.html#dictionary-encoded-layout

Signed-off-by: Fredrik Fornwall <fredrik@fornwall.net>
@fornwall
fornwall force-pushed the test/bind-dictionary branch from 5f79835 to e980017 Compare July 14, 2026 09:53
@fornwall fornwall changed the title test(statement): cover dictionary-encoded bind parameter columns feat: cover dictionary-encoded bind parameter columns Jul 14, 2026

@lidavidm lidavidm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be just a .txtcase or derive from an existing .txtcase (e.g. see the ingest tests where ingest/string is used with some Python code to test other things while allowing some flexibility to override the test data)

@fornwall

Copy link
Copy Markdown
Contributor Author

I think this should be just a .txtcase or derive from an existing .txtcase (e.g. see the ingest tests where ingest/string is used with some Python code to test other things while allowing some flexibility to override the test data)

👍 Made the transition to .txtcase in abe8932, adding the necessary dictionary support to the test machinery. I didn't see a clean fit for deriving from an existing .txtcase, but let me know if you think otherwise.

@fornwall
fornwall requested a review from lidavidm July 15, 2026 07:34
@lidavidm

Copy link
Copy Markdown
Contributor

Sorry, I should've explained more...This is what I mean:

def test_replace_schema(
self,
driver: model.DriverQuirks,
conn: adbc_driver_manager.dbapi.Connection,
query: Query,
) -> None:
subquery = query.query
assert isinstance(subquery, model.IngestQuery)
data = subquery.input()
expected = subquery.expected()
# Create a table in the default schema
default_data = data
# Create different data for the secondary schema
data = data.slice(0, 2)
data2 = data.slice(0, 1)

It's "parametrized" but the parameterization only ever uses a single test case. The test itself then transforms the data before doing something with it. That way it effectively inherits whatever customization was done for that test case without requiring a separate test case.

@fornwall

Copy link
Copy Markdown
Contributor Author

Thanks! I transitioned to parameterization in 2ac53ff - does that match what you meant?

I found that support for dictionary encoded types other than strings is lacking in at least the sqlite/postgres driver (looking into a PR for that), so for now I'm just checking string & large_string.

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.

2 participants