Skip to content

Review follow-up: trait cleanup, MSSQL 2022 floor, naming and test feedback - #580

Open
thomasp85 wants to merge 6 commits into
review/dedupfrom
review/feedback
Open

thomasp85 wants to merge 6 commits into
review/dedupfrom
review/feedback

Conversation

@thomasp85

Copy link
Copy Markdown
Collaborator

Stacks on #578. Addresses the remaining review comments from @teunbrand on #569 that the earlier follow-ups didn't cover.

Trait cleanup (SqlDialect)

  • Removed the *_type_name accessors (number_type_name(), …) — call sites now read dialect.type_names().number etc. directly, per the review suggestion. type_name_for stays as the CastTargetType dispatcher.
  • Grouped the trait's ~50 methods into labelled sections: capabilities & self-knowledge, types & identifiers, scalar SQL translations, statement generators & DDL, spatial SQL.

Behaviour changes

  • MSSQL now requires SQL Server 2022+ and uses native GREATEST/LEAST instead of the CASE emulation (reviewer endorsed the version floor). Documented in the dialect's module docs; golden re-blessed.
  • rewrite_namespaced_sql quotes through the dialect instead of naming::quote_ident. Not load-bearing today (only DuckDB/SQLite call it), but removes the latent quoting bug.

Naming

  • GgsqlParams → ConnectParams.

Docs and tests

  • split_cte_prefix documents its machine-generated-input assumption: no T-SQL bracket identifiers, comments, or MySQL backslash escapes, and None conflates "no WITH" with "unparseable WITH".
  • Exasol's module docs now explain why spatial is disabled (the sql_st_* surface, above all reprojection, hasn't been verified against a live instance) and mark enabling as a follow-up.
  • New duckdb-gated test evaluates case_greatest/case_least output against native GREATEST/LEAST, so the emulation is checked as executed SQL.
  • URI param parsing nitpick applied (Some((k, v)) => (k, Some(v.to_string())), simpler match arms).

@thomasp85
thomasp85 added this pull request to stack #579 October 8, 2026 13:00
Call sites read fields off type_names() directly. Trait methods are now
grouped into capabilities, types/identifiers, scalar translations,
statement generators/DDL, and spatial sections.
…_prefix limits; evaluated test for case_greatest/case_least

This branch has not been deployed

No deployments
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