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

SQL row value comparison syntax #26936

Closed
wants to merge 12 commits into from
Closed

SQL row value comparison syntax #26936

wants to merge 12 commits into from

Conversation

mrahhal
Copy link

@mrahhal mrahhal commented Dec 8, 2021

  • New EF.Functions extensions: LessThan, LessThanOrEqual, GreaterThan, GreatThanOrEqual.
  • Implement and support a new RowValueExpression that translates to SQL row value syntax.
    • RowValueExpression
    • RowValueTranslator
    • QuerySqlGenerator
    • SqlExpressionFactory
    • SqlNullabilityProcessor
    • SearchConditionConvertingExpressionVisitor
  • Override on SQL Server provider and expand to normal comparisons using RowValueComparisonTranslator

Closes #26822.

@mrahhal mrahhal marked this pull request as draft December 8, 2021 07:14
@mrahhal
Copy link
Author

mrahhal commented Dec 8, 2021

Current progress regarding things I had to do but I'm not 100% sure about:

  • I had to create a new ArrayExpression from NewArrayExpression or else the columns array argument of the extension method will result in a non translated expression.
  • Added RowValueExpandedTranslator (which extends RowValueTranslator) in Relational, which I'm using in SQL server provider when I override the a CreateRowValueTranslator virtual method on RelationalMethodCallTranslatorProvider. (I had to suppress the internal usage analyzer error for that here)
  • I can't figure out how to reach the actual values when an SqlParameterExpression is involved. Example:
    var a = 2;
    query.Where(e => EF.Functions.LessThan(new object[] { e.EmployeeID }, new object[] { a }))
    In the above snippet I'm getting an SqlParameterExpression instead of a constant expression for the values array because I'm using a variable. I couldn't figure out how I can get the actual values. And without those I can't really generate the row value syntax (or the expanded comparisons).

@roji
Copy link
Member

roji commented Dec 11, 2021

I had to create a new ArrayExpression from NewArrayExpression or else the columns array argument of the extension method will result in a non translated expression.

Let me discuss this with @smitpatel to see exactly how we want to do this. I recommend holding off on this PR for a couple days until we're agreed on this.

Added RowValueExpandedTranslator (which extends RowValueTranslator) in Relational, which I'm using in SQL server provider when I override the a CreateRowValueTranslator virtual method on RelationalMethodCallTranslatorProvider. (I had to suppress the internal usage analyzer error for that here)

I don't think there's much value in having RowValueExpandedTranslator extend RowValueTranslator - aside from looking up the relevant methods on EF.Functions, these translators shouldn't have anything in common really. We could also consider having RowValueExpandedTranslator in relational for any other provider where RowValueExpression isn't supported.

(BTW naming-wise I'd call the translator RowValueComparisonTranslator)

I can't figure out how to reach the actual values when an SqlParameterExpression is involved. Example:

Does it help if you add a [NotParameterized] attribute on the array parameters? Again, I'll confirm this one with @smitpatel as well.

Re SearchConditionConvertingExpressionVisitor, this is a SQL Server-only visitor, but there's never going to be a RowValueExpression in SQL Server since that's not supported there. So I don't think that visitor needs to be changed.

@mrahhal
Copy link
Author

mrahhal commented Dec 11, 2021

Let me discuss this with smitpatel to see exactly how we want to do this. I recommend holding off on this PR for a couple days until we're agreed on this.

👍🏻

I don't think there's much value in having RowValueComparisonTranslator extend RowValueTranslator

So it's ok to duplicate the shared code between them? That's looking up the EF.Functions, plus some other shared code like matching the method, unwrapping the arguments, etc. They're actually exactly the same except for the SqlExpressionFactory call. I wouldn't want to duplicate the match on method especially. I tried to find a translator with a similar scenario to check what you do but couldn't find any.

Does it help if you add a [NotParameterized] attribute on the array parameters?

That did it! It's giving me the same SqlConstantExpression now.

SearchConditionConvertingExpressionVisitor, this is a SQL Server-only visitor, but there's never going to be a RowValueExpression [...]

Alright, so I can just have it throw a NotImplementedException.

@mrahhal
Copy link
Author

mrahhal commented Dec 11, 2021

@roji I have an initial working implementation for both row value and expanded comparisons. It wasn't a lot of work after resolving the parameter issue, I implemented some of your feedback too. As you said, I'll wait for your response regarding the array thing before going any further.

@roji
Copy link
Member

roji commented Dec 14, 2021

@mrahhal we've discussed this internally, and unfortunately this raises some design questions which are more fundamental than initially thought. We're going to hold off on this for now since there's more highly-prioritized work that has to happen first - we'll revisit this in a few months based on where we are with the 7.0 release.

I still fully hope we can get this in for 7.0 - it's just that we need to focus on other areas first.

@mrahhal
Copy link
Author

mrahhal commented Dec 15, 2021

@roji Understood. I'll be ready to get this ready again in the future if necessary.

@roji
Copy link
Member

roji commented Dec 15, 2021

Thanks for understand @mrahhal!

@roji
Copy link
Member

roji commented Jan 19, 2022

@mrahhal I'm going to go ahead and close this for now, as discussed. We'll pick it up again when the proper tuple syntax becomes available.

@roji roji closed this Jan 19, 2022
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.

Allow users to express SQL row value comparison syntax
2 participants