fix: wrap explain_query in the Raw { engine, format, payload, ... } shape (#89) - #111
Merged
Merged
Conversation
…hape (#89) explain_query returned the raw EXPLAIN JSON value directly, with a comment acknowledging the host would fall through to ExplainQueryOutput::Plan { plan: res } instead of the Raw variant the builtin driver emits. The host's plugin adapter only classifies a response as Raw when it finds engine/format/payload as strings (via .as_str()) plus an optional original_query -- any other shape, including a bare JSON array, falls through to Plan. Runtime-registered EXPLAIN parsers select on engine+format, so this plugin's output was never selectable by them, and the parsed-plan renderer ran instead of the raw-payload path the builtin uses. Wrapped the plan JSON in the exact shape the adapter checks: { engine: "postgres", format: "postgres-json", payload: <stringified JSON>, original_query: <the query> }. payload must be the JSON *string* form, not the live JSON value -- the adapter reads it with object.get("payload")?.as_str(), which silently returns None (not an error) for a nested object/array, dropping the response into the Plan fallback exactly like the pre-fix bare-array response did. TDD: added raw_explain_output unit tests to query_tests.rs, including one that specifically checks payload is Value::String (not a nested value) to catch that exact silent-fallback failure mode. Confirmed the new tests fail to even compile against the pre-fix code (the function didn't exist yet) before the fix. Verified live against a real PostgreSQL instance by simulating the host adapter's exact matching logic (object.get(field)?.as_str() for each of engine/format/payload) against real explain_query responses: confirmed the pre-fix binary's bare-array response is NOT classified as Raw (falls through to Plan, reproducing the bug), and the post-fix binary's response IS classified as Raw, with engine/format/ original_query correct and payload containing a real EXPLAIN plan (both without and with analyze=true, confirming actual execution stats like "Actual Rows" come through correctly in the ANALYZE case).
Version suggestionBased on this PR's title (
This is informational only — no tag or release is created automatically yet. |
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
explain_queryreturned the raw EXPLAIN JSON value directly, with a comment acknowledging the host would fall through toExplainQueryOutput::Plan { plan: res }instead of theRawvariant the builtin driver emits.tabularisplugins/driver.rs::explain_query) only classifies a response asRawwhen it findsengine/format/payloadas strings (via.as_str()) plus an optionaloriginal_query— any other shape, including a bare JSON array, falls through toPlan. Runtime-registered EXPLAIN parsers select onengine+format, so this plugin's output was never selectable by them, and the parsed-plan renderer ran instead of the raw-payload path the builtin uses.{ engine: "postgres", format: "postgres-json", payload: <stringified JSON>, original_query: <the query> }.payloadmust be the JSON string form, not the live JSON value — the adapter reads it withobject.get("payload")?.as_str(), which silently returnsNone(not an error) for a nested object/array, dropping the response into thePlanfallback exactly like the pre-fix bare-array response did.Fixes #89.
Test plan
raw_explain_outputunit tests toquery_tests.rs, including one that specifically checkspayloadisValue::String(not a nested value) to catch that exact silent-fallback failure mode. Confirmed the new tests fail to even compile against the pre-fix code (the function didn't exist yet) before the fix.cargo test --lib— 327 passedcargo clippy --all-targets -- -D warnings— cleancargo fmt --all -- --check— cleanobject.get(field)?.as_str()for each ofengine/format/payload) against realexplain_queryresponses:Raw(falls through toPlan, reproducing the bug)Raw, withengine/format/original_querycorrect andpayloadcontaining a real EXPLAIN plananalyze=true, and that real execution stats ("Actual Rows") come through correctly in theANALYZEcaseNote for
tabularis(not part of this PR)While reviewing the cross-repo parity suite (
src-tauri/tests/postgres_integration/parity_explain.rs), noticed its top-of-file comment says: "ExplainQueryOutput differs structurally between built-in (Raw variant) and plugin (Plan variant)" — documenting the exact divergence this PR fixes. The tests themselves only assertOk(not the response shape), so they'll keep passing either way, but that comment is now stale once this merges. Flagging for awareness; not something to fix in this repo.