Skip to content

Fix strict Fabric/TSQL validation for PostgreSQL dates and subqueries - #396

Open
VaibhaveS wants to merge 2 commits into
tobilg:mainfrom
VaibhaveS:vsekar/fix-fabric-strict-subqueries
Open

Fix strict Fabric/TSQL validation for PostgreSQL dates and subqueries#396
VaibhaveS wants to merge 2 commits into
tobilg:mainfrom
VaibhaveS:vsekar/fix-fabric-strict-subqueries

Conversation

@VaibhaveS

Copy link
Copy Markdown
Contributor

Summary

  • inspect through PostgreSQL text casts when validating TO_DATE literals against the T-SQL/Fabric DATE domain
  • reject strict PostgreSQL-to-T-SQL/Fabric transpilation when an aggregate argument contains a subquery that the target would reject with Msg 130
  • preserve valid casted date boundaries, dynamic date inputs, scalar subqueries, and aggregates contained inside scalar subqueries
  • add mirrored Fabric and T-SQL regression coverage

Fixes #393
Fixes #395

Validation

  • cargo fmt --all --check
  • complete fabric_regression: 150 passed
  • complete tsql_regression: 149 passed
  • focused issue tests pass for both Fabric and T-SQL targets

The broader all-dialects crate command was also attempted locally, but the filesystem ran out of space while linking unrelated test binaries; the two complete affected target suites finished successfully.

tobilg commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Thanks for putting this together. The intended ::text canonicalization case from #393 is handled, and the aggregate-subquery validation for #395 looks appropriately scoped.

I found one additional cast edge that seems worth addressing before merge. postgres_tsql_literal_string currently unwraps every type accepted by is_string_data_type, including length-constrained CHAR(n) and VARCHAR(n). In PostgreSQL, those explicit casts can truncate or pad the literal before TO_DATE receives it, so inspecting the underlying literal is not always equivalent to inspecting the cast result.

For example:

SELECT to_date('0001-01-01'::char(1), 'YYYY-MM-DD'::text)

The PR build currently accepts this in strict mode for both targets and emits:

SELECT CONVERT(DATE, CAST('0001-01-01' AS CHAR(1)), 23)

PostgreSQL reduces the first argument to '0'; TO_DATE treats year zero as 1 BC, which is outside the T-SQL/Fabric DATE domain. The range check instead sees the pre-cast '0001-01-01' value and considers it valid.

A focused adjustment could either:

  • unwrap only value-preserving PostgreSQL casts (at minimum ::text, which covers the reported pg_get_querydef form), or
  • apply the cast's length semantics before validating the literal.

It would also be helpful to add a bounded CHAR/VARCHAR regression to the existing Fabric and T-SQL regression files, and to assert the exact generated SQL for accepted ::text boundary cases.

The rest of the patch looked good in review. cargo fmt --all --check and the diff check passed; the complete focused suites passed (Fabric 150/150, T-SQL 149/149), as did 1,142 core library tests. GitHub's quality, Python, and Rust jobs were green when checked.

@VaibhaveS

Copy link
Copy Markdown
Contributor Author

Addressed in 1321542. Thank you for catching this.

I chose the cast-semantics option: postgres_tsql_literal_string now evaluates bounded PostgreSQL character casts before validating the date literal. CHAR(n) truncates and blank-pads, while bounded VARCHAR(n) truncates; unbounded/value-preserving text casts remain unchanged. This also handles nested forms such as literal::char(n)::text.

The mirrored Fabric and T-SQL tests now:

  • reject the reported to_date('0001-01-01'::char(1), ...) case,
  • reject the equivalent bounded varchar(1) case, and
  • assert exact generated SQL for accepted lower/upper ::text boundaries and a dynamic ::text input.

Validation after the change:

  • cargo fmt --all --check
  • core library: 1,142 passed, 1 ignored
  • Fabric regression: 150/150 passed
  • T-SQL regression: 149/149 passed
  • both focused date-domain tests re-run from the formatted tree and passed.

tobilg commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Thanks for the update—the earlier cast-semantics concern is addressed thoroughly. The reported CHAR(1) and equivalent VARCHAR(1) cases are now rejected, nested casts are evaluated, and the accepted ::text cases have exact output assertions.

While stress-checking the updated helper, I noticed one resource-use edge worth addressing before merge. postgres_string_cast_literal currently materializes every trailing space required by CHAR(n):

result.extend(std::iter::repeat_n(
    ' ',
    length.saturating_sub(value_length),
));

The cast parser currently stores the parsed length through an i64 as u32 conversion. I confirmed that both of these inputs parse with a CHAR length of 4294967295:

SELECT to_date('1'::char(-1), 'YYYY-MM-DD')
SELECT to_date('1'::char(4294967295), 'YYYY-MM-DD')

If either reaches strict PostgreSQL-to-T-SQL/Fabric generation, the one-character literal can cause an attempted allocation of roughly 4 GiB of padding before the date-range check runs. Large valid widths also create work proportional to the declared width even though the range checker immediately trims the padding.

A focused adjustment could keep the effective truncated prefix without physically constructing trailing spaces, since those spaces do not affect this year-domain check. Alternatively, validating or safely bounding the width before allocation would avoid the proportional allocation. A regression covering a very large or negative declared width would help ensure this path remains controlled.

Everything else in the update looks good. Formatting and diff checks pass, both focused suites pass (Fabric 150/150 and T-SQL 149/149), and the complete GitHub CI run is green across quality, Rust, Python, SDK, and Go.

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

Labels

None yet

Projects

None yet

2 participants