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

Translate ToString() over enums #33706

Conversation

Danevandy99
Copy link
Contributor

@Danevandy99 Danevandy99 commented May 13, 2024

Fixes #33635 and #20604

  • I've read the guidelines for contributing and seen the walkthrough
  • I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
  • The code builds and tests pass locally (also verified by our automated build checks)
  • Commit messages follow this format:
        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber
  • Tests for the changes have been added (for bug fixes / features)
  • Code follows the same patterns and style as existing code in this repo

@Danevandy99 Danevandy99 changed the title Fixes #33635 - Translate ToString on enums with number store types as a CASE/WHEN expression Fixes #33635 and #20604 May 16, 2024
@Danevandy99
Copy link
Contributor Author

@roji @cincuranet This is ready to review again.

@roji roji changed the title Fixes #33635 and #20604 Translate ToString() over enums May 16, 2024
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

In addition to the below comments, one more general note...

Translations where we duplicate an expression multiple times are problematic, since the expression may be arbitrarily complex. For example, if instance here is a complex subquery, that subquery would get evaluated by the database N times (once for each CASE); this is bad enough for perf that we refrain from translating in most such cases (e.g. entity equality).

At least for now, we should do the same here, i.e. perform the CASE/WHEN translation only when instance is a column or a parameter - can you please add that check?

&& converter.GetType().IsGenericType
&& converter.GetType().GetGenericTypeDefinition() == typeof(EnumToStringConverter<>))
{
return _sqlExpressionFactory.MakeUnary(ExpressionType.Convert, instance, typeof(string));
Copy link
Member

Choose a reason for hiding this comment

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

Since the enum is already mapped to a string in the database, an actual SQL cast shouldn't be necessary here. However, there's still a CLR type change here, and we currently lack the ability to "change" the CLR type without a corresponding SQL expression node.

Can you please leave a TODO comment here, referencing #33733?

Copy link
Contributor Author

@Danevandy99 Danevandy99 May 16, 2024

Choose a reason for hiding this comment

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

Yep! I did attempt to just return the instance without the cast at one point, but ran into issues that seemed larger than the scope of this issue.

if (methodCallExpression.Object != null
&& @object!.Type.IsNullableType()
&& methodCallExpression.Method.Name != nameof(Nullable<int>.GetValueOrDefault))
&& methodCallExpression.Method.Name != nameof(Nullable<int>.GetValueOrDefault)
&& methodCallExpression.Method.Name != nameof(Nullable<int>.ToString))
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why this change is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling ToString on a nullable value type like an enum returns an empty string if the value is null, rather than null: https://learn.microsoft.com/en-us/dotnet/api/system.nullable-1.tostring?view=net-8.0 Without that check, the in-memory query returns null if the value of the nullable enum is null, rather than an empty string like C# usually does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like that might also allow ToString to be called on a reference type, since @object!.Type.IsNullableType() returns true for non-value types and we're only comparing the method names. The new check should probably include @object!.Type.IsNullableValueType()

Copy link
Member

Choose a reason for hiding this comment

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

See my new review comment about not returning an empty string as the default case, but rather a string representation of the integer enum value (which is what .NET does). If we do that, then the NULL should simply bubble out through the conversion, which I think is the behavior we want, right? If so, then this change shouldn't be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the change you mentioned in your other comment seems like it should make this change unnecessary. I'll remove it and let you know if that doesn't end up working.

@Danevandy99 Danevandy99 requested a review from roji May 18, 2024 20:51
@Danevandy99 Danevandy99 force-pushed the translate-to-string-on-enums-with-number-store-types-to-case-expression branch from 475fd88 to 5333cd5 Compare May 27, 2024 21:52
@Danevandy99 Danevandy99 changed the base branch from release/8.0 to main May 27, 2024 21:53
@Danevandy99 Danevandy99 force-pushed the translate-to-string-on-enums-with-number-store-types-to-case-expression branch from 9f353a0 to 56841ce Compare May 27, 2024 22:57
if (methodCallExpression.Object != null
&& @object!.Type.IsNullableType()
&& methodCallExpression.Method.Name != nameof(Nullable<int>.GetValueOrDefault))
&& methodCallExpression.Method.Name != nameof(Nullable<int>.GetValueOrDefault)
&& methodCallExpression.Method.Name != nameof(Nullable<int>.ToString))
Copy link
Member

Choose a reason for hiding this comment

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

See my new review comment about not returning an empty string as the default case, but rather a string representation of the integer enum value (which is what .NET does). If we do that, then the NULL should simply bubble out through the conversion, which I think is the behavior we want, right? If so, then this change shouldn't be necessary.

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Thanks, this is starting to get close. I'll be available in the coming time to push this to completion (I was traveling quite a lot in recent weeks).

@Danevandy99 Danevandy99 force-pushed the translate-to-string-on-enums-with-number-store-types-to-case-expression branch from 7efbfdf to 126c618 Compare June 3, 2024 23:39
@Danevandy99
Copy link
Contributor Author

@roji GitHub wasn't letting me reply to this comment (#33706 (comment)), but the behavior we want is for the result to be an empty string if we call ToString on a nullable enum, since that's what C# does (https://learn.microsoft.com/en-us/dotnet/api/system.nullable-1.tostring?view=net-8.0). I tested it, and that change is still necessary, otherwise the code inside that if block bypasses the ToString call and just returns null instead of the empty string.

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Good catch with the empty-string returning behavior of Nullable.ToString().

This is almost done - see some final comments. There are also some test failures.

AssertSql(
"""
SELECT CASE
WHEN "g"."Rank" = 0 THEN 'None'
Copy link
Member

Choose a reason for hiding this comment

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

This still needs to be updated (causes test failures)

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turnaround and for the patience! LGTM.

@roji roji merged commit 80e9dce into dotnet:main Jun 4, 2024
3 of 7 checks passed
@Danevandy99
Copy link
Contributor Author

@roji Of course! Thanks for all your guidance. Is there anything I need to do for this to get it into a release build (I'm assuming it'll be in one of the .NET 9 previews)?

@roji
Copy link
Member

roji commented Jun 4, 2024

Nope, it's merged and will appear in a coming preview!

@Danevandy99
Copy link
Contributor Author

Awesome, thanks!

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

Successfully merging this pull request may close these issues.

Translate ToString on enums with number store types as a CASE/WHEN expression
3 participants