fix: by-name column access returns column1/column2 for joins and compounds (#709) - #720
Open
dpsiderius wants to merge 1 commit into
Open
dpsiderius wants to merge 1 commit into
dpsiderius wants to merge 1 commit into
Conversation
…#709) output_column_names_joined (src/codegen/select/order_by.rs) generalizes the existing single-table "alias, else bare column name, else columnN" rule to a joined FROM: a bare column reference already names itself independent of its table qualifier (SELECT a.x carries name: "x" regardless of the "a." prefix), so the only real gap was */table.* expansion, which now draws from each joined table's own schema in FROM-clause order. A compound SELECT already had the right machinery internally (compile_select_compound resolves its own trailing ORDER BY against the leftmost arm's names) — it just wasn't reachable from header derivation, which is now unconditional instead of requiring select.compound.is_empty(). derive_headers (src/bin/sqlite-rs/repl.rs) wires both cases in; the CLI test that asserted the old column1/column2 fallback for a join (tests/unit/repl_dot_commands.rs::select_with_join_falls_back_to_positional_headers) is inverted to assert the real names instead. Known pre-existing gap, not introduced or widened here: an unaliased *computed* expression (`a + 1`) still falls back to columnN rather than the oracle's own expression-text rendering, in both the single-table and joined paths alike — this crate has no expression-to-SQL-text printer yet. Not covered by this ticket's acceptance criteria. Not done (api.rs is PR #705, unmerged, out of scope here): - Row::get_by_name/result_column_names in the embedding API itself — once #705 lands, its result_column_names should delegate to output_column_names/output_column_names_joined rather than reimplementing the rule, and tests/unit/api_statement_test.rs::joins_and_compounds_report_positional_column_names (named in this ticket's acceptance criteria, but not present on this branch) should be added/inverted the same way repl_dot_commands.rs's test was here. - Spec 013 Requirement 3's "scoped to single-table selects" sentence: left in place, since spec 013's text lives on the unmerged branch too and touching it here isn't safe — flagged for removal in the same PR that merges #705. spend: roughly matched the medium estimate.
7 tasks
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.
Stacked on #707 (fix/707-sqlite-master-select) — merge that first.
What
output_column_names_joined(src/codegen/select/order_by.rs)generalizes SQLite's naming rule — alias if present, else the bare
column name, else the expression text, with a compound taking its names
from the left-most arm — across joined tables'
*/table.*expansion.Wired into
derive_headers; theselect.compound.is_empty()restriction that scoped this to single-table selects is gone (the
leftmost-arm rule already existed internally).
Before/after
Not done, explicitly out of scope:
src/api.rs/Row::get_by_name(PR #705 unmerged) — noted here for whoever lands #705 to delegate to
output_column_names_joined, and to invert/addtests/unit/api_statement_test.rs::joins_and_compounds_report_positional_column_names.Spec 013 Req 3's "scoped to single-table selects" sentence should be
removed in that same follow-up; left untouched here.
Known pre-existing gap flagged, not fixed: an unaliased computed
expression (
a+1) still falls back tocolumnNinstead of the oracle'sexpression-text rendering — true in both the old single-table and new
joined paths, since there is no expr-to-SQL printer. Not in this ticket's
acceptance criteria.
Test plan
tests/corpus/result_column_names_test.rs— 9 oracle-diffed shapes:2/3-table join, UNION, UNION ALL, EXCEPT, expression columns,
t.*expansion, subquery-in-FROM, alias, qualified name, duplicate names.
tests/unit/repl_dot_commands.rs::select_with_join_falls_back_to_positional_headers→select_with_join_reports_real_column_names.make test,make lint,make test-corpus,make check-mvl-limit,make check-mod-files,make assuranceall pass.Refs: 013/Req-3, 013/Req-6, #705
Closes #709
spend: matched estimate