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

Query: translate stringColumn.FirstOrDefault() using SUBSTRING on Sql… #20840

Merged
merged 1 commit into from
May 15, 2020

Conversation

dvoreckyaa
Copy link

…Server (#10912)

Summary of changes:

  • Added translation of FirstOrDefault, LastOrDefault for Sql Server, SqLite, Cosmos.

The corresponding tests have been implemented, but tests for the Cosmos are disabled, for a while.

@dnfclas
Copy link

dnfclas commented May 5, 2020

CLA assistant check
All CLA requirements met.

@dvoreckyaa dvoreckyaa closed this May 5, 2020
@dvoreckyaa dvoreckyaa reopened this May 5, 2020
@dvoreckyaa
Copy link
Author

Summary of changes:

  • Added translation of FirstOrDefault, LastOrDefault for Sql Server, SqLite, Cosmos.

  • The corresponding tests have been implemented, but tests for the Cosmos are disabled, for a while.

  • Some cosmos tests have been updated: FirstOrDefault in projection does not do client eval.

"SUBSTRING",
new[] { argument, _sqlExpressionFactory.Constant(1), _sqlExpressionFactory.Constant(1) },
nullable: true,
argumentsPropagateNullability: new[] { false, true, true },
Copy link
Contributor

@maumar maumar May 11, 2020

Choose a reason for hiding this comment

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

this should be {true, true, true} - we want the nullability of the argument to propagate to the entire function, so that we can do the following optimization:

SUBSTRING(foo, 1, 1) IS NULL -> foo IS NULL

typeof(int)),
_sqlExpressionFactory.Constant(1) },
nullable: true,
argumentsPropagateNullability: new[] { false, true, true },
Copy link
Contributor

@maumar maumar May 11, 2020

Choose a reason for hiding this comment

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

same here (actually this one doesn't matter functionally, because we already propagate nullability from the second argument, which is the same as the first, but we can do it for consistency)

Copy link
Author

Choose a reason for hiding this comment

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

I've fixed issue. Could you please clarify how SUBSTRING or RIGHT/LEFT impacts on query translation. I mean SUBSTRING does not accept NULL as the argument, but RIGHT/LEFT does. Would is there be any difference If we used RIGHT/LEFT instead of SUBSTRING?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perf wise, there shouldn't be much difference. LEFT/RIGHT produces smaller sql, so I we could use them instead. SUBSTRING won't accept NULL directly, but column that contains nulls or a function that evaluates to null is fine, so we still need to take nullable scenarios into account.

Copy link
Author

Choose a reason for hiding this comment

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

Got it, thanks. Please review again.

@maumar
Copy link
Contributor

maumar commented May 11, 2020

also, please add the following tests to the NullSemanticsQueryTestBase:

        [ConditionalTheory]
        [MemberData(nameof(IsAsyncData))]
        public virtual Task Nullable_string_FirstOrDefault_compared_to_nullable_string_LastOrDefault(bool async)
        {
            return AssertQuery(
                async,
                ss => ss.Set<NullSemanticsEntity1>().Where(e => e.NullableStringA.FirstOrDefault() == e.NullableStringB.LastOrDefault()),
                ss => ss.Set<NullSemanticsEntity1>().Where(e => e.NullableStringA.MaybeScalar(x => x.FirstOrDefault())
                    == e.NullableStringB.MaybeScalar(x => x.LastOrDefault())));
        }

@maumar
Copy link
Contributor

maumar commented May 11, 2020

also (in FunkyDataQueryTestBase):

        [ConditionalTheory]
        [MemberData(nameof(IsAsyncData))]
        public virtual Task String_FirstOrDefault_and_LastOrDefault(bool async)
        {
            return AssertQuery(
                async,
                ss => ss.Set<FunkyCustomer>().OrderBy(e => e.Id).Select(e => new
                {
                    first = (char?)e.FirstName.FirstOrDefault(),
                    last = (char?)e.FirstName.LastOrDefault()
                }),
                ss => ss.Set<FunkyCustomer>().OrderBy(e => e.Id).Select(e => new
                {
                    first = e.FirstName.MaybeScalar(x => x.FirstOrDefault()),
                    last = e.FirstName.MaybeScalar(x => x.LastOrDefault())

                }),
                assertOrder: true,
                elementAsserter: (e, a) =>
                {
                    AssertEqual(e.first, a.first);
                    AssertEqual(e.last, a.last);
                });
        }
``

@maumar
Copy link
Contributor

maumar commented May 11, 2020

@dvoreckyaa please rebase-squash on latest master and apply the suggested changes - looks good otherwise

…Server (dotnet#10912)

Summary of changes:
- Added translation of FirstOrDefault, LastOrDefault for Sql Server, SqLite, Cosmos.

The corresponding tests have been implemented, but tests for the Cosmos  are  disabled, for a while.
@maumar
Copy link
Contributor

maumar commented May 15, 2020

@dvoreckyaa thank you for the contribution!

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

Successfully merging this pull request may close these issues.

3 participants