-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Minor: comments that explain the schema used in simply_expressions #7747
Conversation
datafusion/optimizer/src/simplify_expressions/simplify_exprs.rs
Outdated
Show resolved
Hide resolved
The diagram is awesome, surely we can find a place for it in docs |
Co-authored-by: comphead <comphead@users.noreply.github.com>
I have included it in #7759 |
@Dandandan or @comphead could I ask one of you to approve this PR (it needs a review prior to being able to merge): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Thank you @comphead |
…pache#7747) * Minor: comments that explain the schema used in simply_expressions * Apply suggestions from code review Co-authored-by: comphead <comphead@users.noreply.github.com> --------- Co-authored-by: comphead <comphead@users.noreply.github.com>
Which issue does this PR close?
Related to #7699
Rationale for this change
I am somewhat embarrassed that I did not fully understand @Dandandan 's fix in https://github.com/apache/arrow-datafusion/pull/7699/files and then spent some non trivial time tracking the same issue down in IOx and filed a ticket #7746 only to find it had already been fixed
What changes are included in this PR?
Add some comments to try and explain the change more generally
BTW I made a diagram explaining what is going on, that I wanted to share even though I am not sure it belongs in the comments directly:
Are these changes tested?
Are there any user-facing changes?