Skip to content

feat: support qualified single-table select columns (rebased from #32) - #76

Open
JagritGumber wants to merge 1 commit into
mainfrom
feat/qualified-select-rebased
Open

feat: support qualified single-table select columns (rebased from #32)#76
JagritGumber wants to merge 1 commit into
mainfrom
feat/qualified-select-rebased

Conversation

@JagritGumber

@JagritGumber JagritGumber commented Apr 24, 2026

Copy link
Copy Markdown
Owner

Rebase + merge of #32 onto current main. Original PR was written before the INNER JOIN multi-table resolver landed, so the rebase had to combine:

Both now coexist in each dialect's resolve_return_columns. Single-table qualified projections like SELECT users.id, users.name AS user_name FROM users resolve against the inferred base table; multi-table qualified projections still go through the JOIN resolver.

Also dropped ensure_supported_select_expr and make_unknown_column from parser/mod.rs (no callers remain after the swap), and updated the stale joins.rs module docstring that described pre-INNER-JOIN state.

Closes #32.

Test plan

  • cargo test --workspace (203 pass — adds three parses_qualified_single_table_select parser tests + one CLI test, replaces the single_table_path_still_rejects_qualified_selects postgres test with the new single_table_path_accepts_qualified_selects)
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo fmt --all -- --check
  • CI green

Open in Devin Review

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

View 4 additional findings in Devin Review.

Open in Devin Review

[column] => strip_identifier_quotes(column).to_lowercase(),
[prefix, column] => {
let prefix = strip_identifier_quotes(prefix).to_lowercase();
if !allowed_prefixes.iter().any(|allowed| *allowed == prefix) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Case-sensitive alias comparison causes parse errors for non-lowercase table aliases

resolve_single_table_select_column lowercases the prefix from the expression (let prefix = strip_identifier_quotes(prefix).to_lowercase() at crates/sqlcx-core/src/parser/mod.rs:85) but then compares it with exact equality against allowed_prefixes (*allowed == prefix). The aliases in allowed_prefixes come from extract_table_alias, which returns the alias preserving its original case from the SQL string (e.g., postgres.rs:443 returns Some(alias) without lowercasing). This means any query using a non-lowercase alias like SELECT U.id FROM users U will fail: the alias "U" is pushed into allowed_prefixes as-is, but the prefix from the expression is lowercased to "u", so "U" == "u" is false and the query is rejected.

This is inconsistent with the multi-table JOIN path, where AliasMap correctly lowercases on both insert (joins.rs:52) and lookup (joins.rs:56). The same bug affects all three dialect parsers (postgres, mysql, sqlite) since they all pass un-lowered aliases into the shared resolve_single_table_select_column.

Suggested change
if !allowed_prefixes.iter().any(|allowed| *allowed == prefix) {
if !allowed_prefixes.iter().any(|allowed| allowed.eq_ignore_ascii_case(&prefix)) {
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +438 to +441
&& !matches!(
alias.to_lowercase().as_str(),
"where" | "join" | "order" | "group" | "limit" | "returning"
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 extract_table_alias misidentifies SQL keyword SET as a table alias for UPDATE queries

In all three extract_table_alias implementations (postgres, mysql, sqlite), the keyword filter list does not include "set". For UPDATE users SET name = $1 RETURNING ..., the pattern "update users " matches, and the remainder starts with SET. Since "set" isn't in the keyword reject list, it's returned as a table alias. While the practical impact is currently negligible (the spurious "SET" prefix is unlikely to be used as a column qualifier, and is further masked by BUG-0001's case mismatch), it becomes a latent correctness issue once BUG-0001 is fixed — set.column would be accepted as a valid qualified select expression.

The postgres version at postgres.rs:440 filters "where" | "join" | "order" | "group" | "limit" | "returning" but not "set". The mysql (mysql.rs:468) and sqlite (sqlite.rs:379) versions also omit it (and additionally omit "returning").

Suggested change
&& !matches!(
alias.to_lowercase().as_str(),
"where" | "join" | "order" | "group" | "limit" | "returning"
)
if !alias.is_empty()
&& !matches!(
alias.to_lowercase().as_str(),
"where" | "set" | "join" | "inner" | "left" | "right" | "full" | "cross" | "natural" | "on" | "order" | "group" | "limit" | "having" | "returning" | "values" | "using" | "union" | "intersect" | "except"
)
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant