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

Handle IN optimizer transformation of ordering to constant #16380

Merged
merged 1 commit into from
Jul 2, 2019
Merged

Conversation

roji
Copy link
Member

@roji roji commented Jul 1, 2019

Our InExpressionValuesExpanding visitor can turn an an IN expression to a constant (i.e. when the values list is empty), which is unsupported in orderings in SqlServer. Detect this and turn these expressions to a simple constant expression detectable by the SQL generator, which will generate (SELECT 1).

Fixes #15713

Some notes:

  • These changes actually seem to be SqlServer-specific. Sqlite doesn't mind ordering by constants, and PostgreSQL is OK to order by bools (in fact, Npgsql can remove InExpressionValuesExpandingExpressionVisitor entirely and send an array parameter instead of expanding the array to a constant). However, the current architecture doesn't allow for this: RelationalShapedQueryCompilingExpressionVisitor isn't really extensible (lots of private etc.).
  • Note Query: IN expression optimization of constant should move to compilation phase #16375 about moving expansion of constant InExpression values to the compilation phase (optimization).

@roji roji requested a review from smitpatel July 1, 2019 17:01
Copy link
Contributor

@smitpatel smitpatel left a comment

Choose a reason for hiding this comment

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

Don't forget to react in cosmos tests since you are modifying SimpleQueryTestBase.

Our InExpressionValuesExpanding visitor can turn an an IN expression
to a constant (i.e. when the values list is empty), which is
unsupported in orderings in SqlServer. Detect this and turn these
expressions to a simple constant expression detectable by the
SQL generator, which will generate (SELECT 1).

Fixes #15713
@roji roji merged commit 767564b into master Jul 2, 2019
@ghost ghost deleted the OrderBy1 branch July 2, 2019 18:43
protected override Expression VisitExtension(Expression extensionExpression)
{
var visited = base.VisitExtension(extensionExpression);
if (visited is OrderingExpression orderingExpression)
Copy link
Contributor

Choose a reason for hiding this comment

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

While this may not be cause of perf regression reported #16405
Let's improve it somewhat since this is runtime and not compile time.

  • Only visit orderingExpression if inner expression was actually changed.
  • Initialize the visitor only once in the ctor

Copy link
Contributor

Choose a reason for hiding this comment

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

@smitpatel should we track this in an issue so that we don't miss it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am reverting this commit. Hit an issue in group by translation. This logic is actually incorrect.
ORDER BY COUNT(*) is valid and does not reference any column expression.

Copy link
Member Author

Choose a reason for hiding this comment

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

Submitted #16422.

Thanks for the perf suggestions, have made the changes. Note that constant searching now only occurs when an expression was changed to constant (other changes do not trigger this).

For ORDER BY COUNT(*), have modified the visitor to also recognize ParameterExpression (and added a test). I also added SqlFragmentExpression which could contain a column - or not - but it seems better to assume it's non-constant (better to get a "constant ordering" error from the DB than wrong results).

I think that should cover it - do you see any other case where this would wrongly identify something as constant?

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.

Query: Generate OrderBy (Select 1)
3 participants