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

Improve expression debugging #31082

Merged
merged 1 commit into from
Jun 30, 2023
Merged

Improve expression debugging #31082

merged 1 commit into from
Jun 30, 2023

Conversation

roji
Copy link
Member

@roji roji commented Jun 15, 2023

  • Add [DebuggerDisplay] to non-SqlExpressions too (OrderingExpression, ProjectionExpression).
  • For SelectExpression, show the SQL only in [DebuggerDisplay] and add a DebugView for long/short versions; this is easier than manually calling ExpressionPrinter in the debugger.
  • Also for ShapedQueryExpression, print the QueryExpression in [DebuggerDisplay]

Some questions...

  • Should we just switch to printing the SQL only by default for ExpressionPrinter for SelectExpression (i.e. not the additional projection data)? I think this would maybe be a nicer default experience when e.g. seeing a SelectExpression embedded inside some other query. On the other hand, if you think that always seeing the projections is useful in embedded selects, we should leave it the way it is...
  • Any idea why our [DebuggerDisplay] are always inside #if DEBUG?

@roji roji requested a review from maumar June 15, 2023 09:49
@maumar
Copy link
Contributor

maumar commented Jun 15, 2023

I found projections to be quite useful in the printer output for SelectExpression, I'd strongly prefer to keep it.

@JamesNK
Copy link
Member

JamesNK commented Jun 15, 2023

Any idea why our [DebuggerDisplay] are always inside #if DEBUG?

Only including DebuggerDisplay on DEBUG builds would limit their use to the EF team. Intentional? It seems weird to improve debugging but not for all developers.

@roji
Copy link
Member Author

roji commented Jun 16, 2023

I found projections to be quite useful in the printer output for SelectExpression, I'd strongly prefer to keep it.

OK. I'll leave things as they are in this PR: the default ExpressionPrinter.Print() for SelectExpression will show the full details including projections. For [DebuggerDisplay] directly on SelectExpression, we'll show the short SQL since space there is short and it's harder to read multi-line strings (before this PR there was no [DebuggerDisplay] at all). It's now also always possible to just go into DebugView to get the full string.

Any idea why our [DebuggerDisplay] are always inside #if DEBUG?

Only including DebuggerDisplay on DEBUG builds would limit their use to the EF team. Intentional? It seems weird to improve debugging but not for all developers.

Yep, I've run into this when debugging from the external PostgreSQL provider. I'll remove these.

@maumar I've done a tiny bit more of cleanup/changes, including changing JsonScalarExpression to print col -> path, like the actual JSON lookup operator in all databases except for SQL Server; for JsonQueryExpression i've changed to col Q-> path to distinguish it.

Will let you review all the above before merging in case you prefer to change something.

@maumar maumar self-requested a review June 16, 2023 09:07
@maumar
Copy link
Contributor

maumar commented Jun 30, 2023

@roji looking good!

@roji roji merged commit 60eb40a into dotnet:main Jun 30, 2023
@roji roji deleted the Debugging branch June 30, 2023 21:38
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