fix(meerkat-core): prune redundant post-join SELECT * to stop ambiguous CSV export (ISS-301213)#272
Closed
jayachandranramadoss-devrev wants to merge 2 commits into
Closed
Conversation
…us CSV export (ISS-301213) CSV export from dashboard widgets failed with `Binder Error: Ambiguous reference to column name "<col>"` when the resolution-generated SQL left-joined a base query against multiple lookup tables that share a raw column name (e.g. `devrev_copy` across `dim_group` and `dim_devu`). The export wrapper emitted `SELECT <explicit cols>, * FROM (<join>)`; the trailing bare `*` re-exposed every joined table's raw columns, so a shared name became ambiguous and DuckDB rejected the COPY. The `*` is load-bearing — resolved lookup columns (e.g. `..__owner_id__fullname`) reach the outer query only through it, so it cannot simply be deleted. Add an AST pass, `pruneRedundantStarsOverJoins`, that: - serializes the final SQL via DuckDB `json_serialize_sql`, - finds each bare `*` whose enclosing SELECT reads from a JOIN, - replaces the `*` with explicit refs to the JOIN-exposed columns that are still referenced elsewhere and not already projected, dropping the unreferenced raw passthrough (e.g. `devrev_copy`) that caused the collision. The rewrite is surgical: it edits only the `*` character (located via the node's `query_location` byte offset, converted to a UTF-16 index so multibyte literals don't shift it) and leaves the rest of the SQL byte-for-byte intact, so existing generated SQL is otherwise unchanged. Any serialize/parse failure returns the original SQL untouched, so the export path never throws. Wired into `cubeQueryToSQLWithResolution` at the resolution exit in both meerkat-node and meerkat-browser. The live-query path is unaffected (resolution is skipped there). Tests: unit coverage for the pass (byte-offset location, leading-star comma handling, graceful degradation on reject/parse-failure, EXCLUDE untouched) and a node integration regression asserting no bare `*` over a JOIN plus a working CSV COPY with resolved values preserved. The prod Binder error is not directly reproducible locally because the bundled DuckDB auto-renames duplicate `*` columns, so the regression oracle is SQL shape + successful execution. work-item: ISS-301213 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jayachandranramadoss-devrev
requested review from
itsTalwar,
ujaval403,
vpbs2 and
zaidjan-devrev
as code owners
|
…ookup SQL (ISS-301213) Replace the 334-line pruneRedundantStarsOverJoins AST rewriter with a simpler preventive approach: narrow each resolution lookup table's SQL to only SELECT the columns needed for the join (joinColumn + resolutionColumns). This prevents unrelated columns (e.g. devrev_copy) from ever entering the JOIN scope, eliminating the ambiguity at its source rather than patching it after the fact. Net result: +45 / -645 lines, zero runtime cost (no AST parse/rewrite on every resolution call), same bug fix. work-item: ISS-301213 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
4 tasks
Contributor
Author
|
Superseded by #280, which fixes the same ISS-301213 ambiguity with a qualify approach (rename every declared lookup column to a unique table-qualified alias |
jayachandranramadoss-devrev
deleted the
ISS-301213-fix-export-ambiguous-star
branch
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
1 participant
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
Binder Error: Ambiguous reference to column namewhen resolution joins multiple lookup tables that share a raw column name (e.g.devrev_copyin bothdim_devuanddim_group)joinColumn+resolutionColumns), preventing unrelated columns from entering the JOIN scopepruneRedundantStarsOverJoins) with an 11-line preventive helper — simpler, zero runtime overhead, same fixChanges
generate-resolution-schemas.ts: AddbuildNarrowedLookupSqlthat wraps lookup SQL with explicit column projectionprune-redundant-stars/module (no longer needed)pruneRedundantStarsOverJoinscalls from both browser and node resolution entry pointsresolution.spec.ts(4 cases in core, 4 in node),generate-resolution-schemas.spec.ts(1 case), andresolution-shared-column-ambiguity.spec.ts(remove AST shape assertion, keep CSV execution assertion)DevRev
work-item: ISS-301213
Test plan
meerkat-node/resolution.spec.ts, 6/6 inmeerkat-core/resolution.spec.ts)🤖 Generated with Claude Code