Skip to content
New issue

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

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

Already on GitHub? # to your account

SQLite: Translate Math.Sign #30925

Merged
merged 1 commit into from
May 19, 2023
Merged

SQLite: Translate Math.Sign #30925

merged 1 commit into from
May 19, 2023

Conversation

bricelam
Copy link
Contributor

Part of #18843

@bricelam bricelam requested a review from a team May 18, 2023 18:10
@@ -78,7 +84,7 @@ public SqliteMathTranslator(ISqlExpressionFactory sqlExpressionFactory)
{
RelationalTypeMapping? typeMapping;
List<SqlExpression>? newArguments = null;
if (sqlFunctionName == "max" || sqlFunctionName == "max")
if (sqlFunctionName == "max" || sqlFunctionName == "min")
Copy link
Member

Choose a reason for hiding this comment

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

Oops - so we weren't inferring correctly for min previous...

Suggested change
if (sqlFunctionName == "max" || sqlFunctionName == "min")
if (sqlFunctionName is "min" or "max")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol, I reject your new age sorcery. (I'm going to change it in the big Math PR anyway.)

Oops - so we weren't inferring correctly for min previous...

Yeah, but we'd still infer it from the first argument which is most likely your column anyway. Also type-mappings are a lot less important on SQLite since you can just ask the ADO.NET provider for whatever type you want. So yeah, there was probably a corner-case involving value converters somewhere that this fixes.

Copy link
Member

Choose a reason for hiding this comment

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

lol, I reject your new age sorcery. (I'm going to change it in the big Math PR anyway.)

Oh that's nothing, take a look at what @maumar has to put up with 🤣

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants