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

Minor: improve documentation for CommonSubexprEliminate #9700

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Mar 19, 2024

Which issue does this PR close?

Part of #7013

Rationale for this change

In the context of looking at #9678 with @wiedld it was a little unclear what the CommonSubexprEliminate pass was doing without code exploration

What changes are included in this PR?

Add some more description of what CommonSubexprEliminate does along with an example

Are these changes tested?

By docs CI

Are there any user-facing changes?

No, just docs

@alamb alamb added the documentation Improvements or additions to documentation label Mar 19, 2024
@alamb alamb requested a review from waynexia March 19, 2024 19:26
@github-actions github-actions bot added optimizer Optimizer rules and removed documentation Improvements or additions to documentation labels Mar 19, 2024
/// refer to that new column.
///
/// ```text
/// ProjectionExec(exprs=[extract (day from new_col), extract (year from new_col)]) <-- reuse here
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @alamb just a question, so to read from the plan that Common Sub-expression Elimination optimization was applied we need to refer to new_col but how it differs from plan with projection alias but no optimization applied?

In short my question is it possible to read from the plan that optimization has been applied?

Copy link
Member

Choose a reason for hiding this comment

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

It is possible. The optimized plan will have an obvious intermediate Projection plan that does some computation and the result will be referred later by other following plans (this pattern might occur in other reasons though

Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

Thanks @alamb, this document is very clear 💯

/// refer to that new column.
///
/// ```text
/// ProjectionExec(exprs=[extract (day from new_col), extract (year from new_col)]) <-- reuse here
Copy link
Member

Choose a reason for hiding this comment

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

It is possible. The optimized plan will have an obvious intermediate Projection plan that does some computation and the result will be referred later by other following plans (this pattern might occur in other reasons though

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @alamb and @waynexia

@comphead comphead merged commit c5c9d3f into apache:main Mar 21, 2024
23 checks passed
@alamb alamb deleted the alamb/doc_subexpr_eliminate branch March 21, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimizer Optimizer rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants