fix: Embed JSON-aliased VARCHAR columns as nested JSON (#38) - #39
Merged
Merged
Conversation
- Unit test in query_executor_test.cpp exercises a column whose DuckDB type is the JSON logical alias over VARCHAR. Asserts the response is a nested Object (not a JSON-escaped String), covers nested struct/array values, JSON arrays, NULL, and a plain-VARCHAR regression guard. - Integration test boots the issue's reproduction endpoint and verifies the same shape end-to-end through Crow's response serialiser. These tests are expected to fail before the production change lands — the existing convertVectorVarcharToJson wraps the raw text as a string and Crow escapes it on output. Closes #38 (test side)
DuckDB's JSON type is a logical-type alias over VARCHAR, so duckdb_get_type_id() returns DUCKDB_TYPE_VARCHAR for it. The previous switch in QueryResult::convertVectorEntryToJson therefore dispatched JSON cells to convertVectorVarcharToJson, which wraps the raw text as a crow::json::wvalue(str). Crow then escaped the string at serialisation time, forcing API consumers to JSON.parse the value a second time. Inspect the logical type's alias before dispatching. When it equals the literal "JSON" that DuckDB itself sets on LogicalType::JSON(), route to a new convertVectorJsonToJson helper that parses the bytes with crow::json::load and embeds the parsed value via the wvalue(const rvalue&) constructor, so nested objects and arrays travel through the response unchanged. Malformed JSON in a source cell degrades to the raw string rather than nulling the row. Also destroy the logical type allocated at the top of convertVectorEntryToJson on every exit path. The dead duckdb_destroy_logical_type below the switch was unreachable, leaking one logical-type allocation per emitted cell (~24 bytes each, visible under ASAN). Eliminating that leak unblocks future leak-clean runs of the suite. Closes #38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #38.
When a query returns a column whose DuckDB logical type is
JSON, flAPI emitted the raw text as a JSON-escaped string in the REST response, forcing consumers toJSON.parsethe value a second time. The reporter (BigQuery tree with recursivechildrenarrays typed asJSON) correctly identified the dispatch site:QueryResult::convertVectorEntryToJsonswitches onduckdb_get_type_id(type), which returns the physical type id. DuckDB'sJSONis a logical-type alias overVARCHAR, so the cell fell into theDUCKDB_TYPE_VARCHARbranch and got wrapped as acrow::json::wvalue(str).Fix
duckdb_logical_type_get_aliasbefore dispatching VARCHAR. When it equals the literal"JSON"that DuckDB itself sets onLogicalType::JSON(), route to a newconvertVectorJsonToJsonhelper that parses the bytes withcrow::json::loadand embeds the parsed value via thewvalue(const rvalue&)constructor. Nested objects and arrays now travel through the response unchanged. Malformed JSON degrades to the raw string rather than nulling the row.convertVectorEntryToJsonon every exit path. The deadduckdb_destroy_logical_typebelow the switch was unreachable, leaking one logical-type allocation per emitted cell (~24 bytes each, visible under ASan: removed 13/15 leak allocations the suite reports).Both changes are scoped to
src/query_executor.cppandsrc/include/query_executor.hpp. The Arrow output path has the same physical-id dispatch inarrow_serializer.hppbut Arrow conventionally represents JSON via metadata/extension types, so it is intentionally out of scope for this PR.Tests
Built TDD-style — failing tests landed first, then the fix.
Unit tests (
test/cpp/query_executor_test.cpp, new[json]tag):CAST(NULL AS JSON)stays Null.Integration test (
test/integration/test_json_column.tavern.yaml+ supportingsqls/json_demo.{yaml,sql}):GET /json-demo/returns the exact "Expected" shape from issue JSON-aliased VARCHAR columns are returned as escaped strings instead of nested JSON in API responses #38.End-to-end verification with a debug-instrumented server confirmed the alias path fires (
type_id=17 alias=JSON is_json=1) and the response body now matches the issue's expected output:{"data":[{"id":1,"payload":{"a":1,"b":[10,20],"c":{"nested":true}}}],"total_count":1,"next":""}Test plan
flapi_tests "QueryExecutor JSON column"— 23/23 assertions).[query_executor]suite green (1092/1092 assertions, 7 cases).ctestsuite — 586/587 pass; the lone failure (CachingFileProvider thread safety_test) is pre-existing and reproduces on stash-popped pre-change code.GET /json-demo/matches the issue's expected JSON.Generated by Claude Code