Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Combine length/precision/scale/collation... from both operands in type inference #32333

Open
roji opened this issue Nov 17, 2023 · 4 comments
Open

Comments

@roji
Copy link
Member

roji commented Nov 17, 2023

When e.g. a concatenation operation happens over two string operands which have a type mapping (e.g. two columns), the left one is arbitrarily picked; this means that the inferred type mapping for [p].[ThreeCharacterProperty] + ';' + [p].[FiveCharacterProperty] will be char(3). This is incorrect and can cause bugs such as #32325, where the inferred type mapping is applied to the result of OPENJSON, and therefore incorrectly truncates.

We should improve our type mapping inference logic to take facets into account: in this case we'd add the MaxLength facets together for the resulting MaxLength (3 + 1 = 5 = 9). This requires some thinking since different logic applies e.g. for MaxLength and for Precision/Scale, etc. Also, we need to have logic handilng concatenation with constant/parameter - for the former we can just add the constant's length, but for the latter we'd need to switch to e.g. nvarchar(max).

Note the relationship with #15586, which is about a compatibility chart between different types; this issue is only about proper handling of facets on the same base type.

@CardenInsurance

This comment was marked as resolved.

@roji

This comment was marked as resolved.

@ajcvickers ajcvickers added this to the Backlog milestone Nov 30, 2023
@roji roji removed this from the Backlog milestone Dec 2, 2023
@roji roji changed the title Combine length/precision/scale from both operands in type inference Combine length/precision/scale/collation... from both operands in type inference Dec 2, 2023
@roji
Copy link
Member Author

roji commented Dec 2, 2023

Note - this should include bubbling up the collation, once it's part of the type mapping (#29620). Note that in my early investigations, all databases fail once there are two explicit collations involved (since it's impossible to know which one to use), but it's a good idea to confirm this.

@clement911
Copy link
Contributor

I find the sp_describe_first_result_set procedure quite useful for this.

Given an arbitrary statement, it shows the exact resulting type that sql server has inferred, including the max length, precision, scale and collation.

Example:
image

This might be useful for some integration tests that compare the EF inferred type with the SQL inferred type.

ajcvickers pushed a commit that referenced this issue Dec 4, 2023
ajcvickers added a commit that referenced this issue Dec 4, 2023
* Work on type mapping inference for string concatenation

Fixes #32325, see also #32333

* Tweaks, update baselines add more tests and cleanup

* Update baselines, add more tests, some cleanup.

---------

Co-authored-by: Shay Rojansky <[email protected]>
@ajcvickers ajcvickers self-assigned this Dec 4, 2023
ajcvickers added a commit that referenced this issue Dec 5, 2023
* Work on type mapping inference for string concatenation

Fixes #32325, see also #32333

* Tweaks, update baselines add more tests and cleanup

* Update baselines, add more tests, some cleanup.

---------

Co-authored-by: Shay Rojansky <[email protected]>
@ajcvickers ajcvickers added this to the Backlog milestone Dec 6, 2023
wtgodbe pushed a commit that referenced this issue Jan 3, 2024
…ts (#32510) (#32520)

* Infer type mapping suitable for concatenating two results (#32510)

* Work on type mapping inference for string concatenation

Fixes #32325, see also #32333

* Tweaks, update baselines add more tests and cleanup

* Update baselines, add more tests, some cleanup.

---------

Co-authored-by: Shay Rojansky <[email protected]>

* Qwerk

* Fix quirk.

---------

Co-authored-by: Shay Rojansky <[email protected]>
@ajcvickers ajcvickers removed their assignment Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants