Wrap SIGN() in CAST(... AS int) for SQL Server#38260
Draft
roji wants to merge 2 commits into
Draft
Conversation
T-SQL SIGN() returns the same data type as its input, but Math.Sign() always returns int. When the input is decimal/double/float/long/short/sbyte, the materializer reads the column with GetInt32() and throws InvalidCastException. Fix by wrapping the SIGN() call in CONVERT(int, ...). Fixes #38249 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes SQL Server translation of Math.Sign(...)/MathF.Sign(...) to avoid InvalidCastException during materialization by ensuring the SQL SIGN(...) result is explicitly cast to int when the input type would otherwise cause SIGN to return a non-int SQL type.
Changes:
- Update SQL Server math translation to wrap
SIGN(...)in a cast-to-int(except when the argument is alreadyint). - Expand the spec test coverage for
Signto include both predicate and scalar projection cases, and add dedicated tests fordecimalandint. - Update provider-specific translation tests (SQL Server/Cosmos) and mark unsupported cases as translation failures for SQLite.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.SqlServer.FunctionalTests/Query/Translations/MathTranslationsSqlServerTest.cs | Updates expected SQL to include CAST(SIGN(...) AS int) and adds assertions for scalar projections plus new decimal/int cases. |
| test/EFCore.Sqlite.FunctionalTests/Query/Translations/MathTranslationsSqliteTest.cs | Updates expected SQL for scalar projections and marks Sign_decimal/Sign_int as unsupported translations for SQLite. |
| test/EFCore.Specification.Tests/Query/Translations/MathTranslationsTestBase.cs | Expands Sign coverage to include scalar projections and adds new Sign_decimal/Sign_int tests. |
| test/EFCore.Cosmos.FunctionalTests/Query/Translations/MathTranslationsCosmosTest.cs | Adds SQL assertions for scalar projections and new decimal/int cases. |
| src/EFCore.SqlServer/Query/Internal/Translators/SqlServerMathTranslator.cs | Implements the cast-to-int wrapping for SIGN(...) to match Math.Sign’s CLR return type. |
| || arg.Type == typeof(int) || arg.Type == typeof(long) || arg.Type == typeof(sbyte) || arg.Type == typeof(short)) | ||
| => TranslateFunction("SIGN", arg, nullTypeMapping: true), | ||
| // T-SQL SIGN returns the same type as its input, but Math.Sign always returns int; | ||
| // wrap with a CONVERT to avoid InvalidCastException at materialization time. |
…itor The SIGN function expression was created with method.ReturnType (int) and null type mapping. After type mapping inference, both the SIGN function and the Convert wrapper ended up with the same 'int' store type, causing SqlExpressionSimplifyingExpressionVisitor to strip the CAST as a no-op. Fix by creating the SIGN function with arg.Type and the argument's type mapping, which correctly reflects T-SQL SIGN's actual return type (same as input). This ensures the CAST wrapper has a different store type and is preserved. Also fix comment wording (CONVERT -> CAST) per review feedback, and remove unused nullTypeMapping parameter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
auto-merge was automatically disabled
May 12, 2026 18:18
Pull request was converted to draft
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
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.
T-SQL SIGN() returns the same data type as its input, but Math.Sign() always returns int. When the input is decimal/double/float/long/short/sbyte, the materializer reads the column with GetInt32() and throws InvalidCastException. Fix by wrapping the SIGN() call in CONVERT(int, ...).
Fixes #38249