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 display of operators in Explain output #347

Closed
Dandandan opened this issue May 15, 2021 · 8 comments · Fixed by #971
Closed

Improve display of operators in Explain output #347

Dandandan opened this issue May 15, 2021 · 8 comments · Fixed by #971
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@Dandandan
Copy link
Contributor

Dandandan commented May 15, 2021

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
Currently the plan shows the operators using the name instead of symbol. A readability improvement would be to use the operators symbols instead.

Old

Filter: #SUM(c) Gt Int64(10) And #b Gt Int64(10) And #SUM(c) Lt Int64(20)

New

Filter: #SUM(c) > Int64(10) AND #b > Int64(10) AND #SUM(c) < Int64(20)

Describe the solution you'd like
Change the Debug implementation to use Display for the operator + fix the tests (74 tests need to be updated).

image

Describe alternatives you've considered

Additional context

FYI @alamb

@Dandandan Dandandan added the enhancement New feature or request label May 15, 2021
@Dandandan Dandandan changed the title Improve display of operators in Explain Improve display of operators in Explain putput May 15, 2021
@Dandandan Dandandan changed the title Improve display of operators in Explain putput Improve display of operators in Explain output May 15, 2021
@alamb
Copy link
Contributor

alamb commented May 16, 2021

I think this is a great idea. Thank you

@alamb alamb added the good first issue Good for newcomers label May 16, 2021
@jorgecarleitao
Copy link
Member

IMO in general we should avoid relying on {:?} for printing things to end users. {:?} with #[derive(Debug)] is very useful to developers; usually users want less information.

@alamb
Copy link
Contributor

alamb commented May 16, 2021

I agree @jorgecarleitao

@Dandandan
Copy link
Contributor Author

Exactly 👍

IMO explain output also should be close or equal to SQL syntax, e.g. for the used expressions.

@alamb
Copy link
Contributor

alamb commented May 19, 2021

FWIW this likely requires implementing Display for Expr

@matthewmturner
Copy link
Contributor

I can give this a shot. To confirm - is the BinaryExpr the only variant of Expr that needs Display implemented?

@alamb
Copy link
Contributor

alamb commented Sep 9, 2021

I can give this a shot. To confirm - is the BinaryExpr the only variant of Expr that needs Display implemented?

Thanks @matthewmturner ! I think the intent is to have Display implemented for all variants of Expr (eventually) but it doesn't have to be all in one PR. I'll leave a comment on #971

@alamb
Copy link
Contributor

alamb commented Sep 9, 2021

(ps sorry for the belated reviews, but I am just catching up from being on vacation)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants