fix(plugin-duckdb): render VARIANT columns as text#1702
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cfa85f3b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| static func isUnrenderable(_ type: duckdb_type) -> Bool { | ||
| switch type { | ||
| case DUCKDB_TYPE_TIMESTAMP_TZ, DUCKDB_TYPE_TIME_TZ, DUCKDB_TYPE_GEOMETRY: | ||
| case DUCKDB_TYPE_INVALID, DUCKDB_TYPE_TIMESTAMP_TZ, DUCKDB_TYPE_TIME_TZ, DUCKDB_TYPE_GEOMETRY: |
There was a problem hiding this comment.
Adding DUCKDB_TYPE_INVALID here routes VARIANT results through patchTzColumns, which re-executes the original query and then copies only the patched columns from that second result back into the first result by row index. For order-unstable or volatile queries, e.g. SELECT i, {'i': i}::VARIANT AS v FROM range(100) ORDER BY random(), the displayed VARIANT text can come from a different execution than the rest of the row, corrupting the result grid. Cast before decoding or replace the entire result from one wrapped execution so all columns come from the same run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adbda4d2ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| var raw = Self.extractResult(from: &result, startTime: startTime) | ||
| Self.patchTzColumns(&raw, query: query, connection: conn) | ||
| Self.patchCastedColumns(&raw, query: query, connection: conn) |
There was a problem hiding this comment.
When executeParameterized is used for a query that returns a VARIANT column and also has placeholders, this call sends only the original SQL into patchCastedColumns, not the bound parameter values. The patch path builds a wrapped query and runs it with duckdb_query, so a query such as SELECT v FROM variants WHERE id = ? fails on the unbound ?; the guard in patchCastedColumns then returns silently and the original VARIANT cell remains the nil value produced by duckdb_value_varchar, so parameterized VARIANT reads still display empty cells.
Useful? React with 👍 / 👎.
1 participant
Problem
DuckDB
VARIANTcolumns showed as empty cells in the macOS DuckDB plugin. Reported upstream at TablePlus/TablePlus#3872 ("Support for DuckDB VARIANT type").Root cause
The bundled DuckDB 1.5.6 lib's deprecated
duckdb_column_typeC API returnsINVALID (0)for a VARIANT column, not41, andduckdb_value_varcharreturns NULL for it. The plugin'sisUnrenderabledenylist only coveredTIMESTAMP_TZ/TIME_TZ/GEOMETRY, so the INVALID-typed variant column fell through toduckdb_value_varcharand rendered as null. (The upstream header definesDUCKDB_TYPE_VARIANT = 41and the issue reported "id: (41)", but our lib's C enum API never surfaces 41, it collapses unrecognized types to INVALID. Confirmed with a C probe againstLibs/libduckdb_arm64.a.)Fix
Treat
DUCKDB_TYPE_INVALIDas unrenderable andCAST(col AS VARCHAR)it through the existing wrapped-query path. This renders VARIANT as text and is forward compatible: any type the deprecated C enum does not recognize now shows its value instead of null. PlainSELECT NULL/SELECT 1reportINTEGER, not INVALID, so the change is targeted.The iOS
DuckDBDriveralready uses an allowlist that treated INVALID as needing a cast, so it rendered VARIANT correctly already and needs no change.Tests
testVariantRendersAsTexttoDuckDBDriverTests. The full 14-test DuckDB suite passes on the simulator, includingtestNullRendersAsNil(INVALID-casting does not disturb plain NULLs).TableProMobileTests), so the plugin's changed lines are verified by the C probe and a local build rather than an automated plugin test.Shipping
DuckDB is a registry-only plugin with no PluginKit ABI change, so this reaches users when the DuckDB plugin is re-released to the registry.
Update (refactor): Reworked the detection from a denylist (
isUnrenderable) to a render allowlist (isNativelyRenderable/requiresTextCast), matching the iOSDuckDBDriver's design. Any type not known to render through the value API (VARIANT/INVALID, TZ types, GEOMETRY, and future unknown types) is cast to VARCHAR in SQL. The allowlist is exactly the previously-rendered type set, so behavior is unchanged for every known type; only unknown/exotic types now render as text instead of null.patchTzColumnsrenamed topatchCastedColumnssince it no longer handles only timezone columns. Verified the wrapped projection end-to-end with a C probe (native columns pass through, variant casts, NULL stays NULL).