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

Log detail name for optimizers of RowExpressionRewriteRuleSet and StatsRecordingPlanOptimizer #22765

Merged
merged 1 commit into from
May 17, 2024

Conversation

feilong-liu
Copy link
Contributor

@feilong-liu feilong-liu commented May 15, 2024

Description

Currently optimizer log will record the class name of the optimizer, however this can be a problem for row expression optimizers, which extends RowExpressionRewriteRuleSet. All such optimizers will have the same name in the log, like "FilterRowExpressionRewrite", "ValuesRowExpressionRewrite" etc. These give no meaningful information on which optimizers are triggered.

This PR will log more verbose information for these optimizers, by including the rewriter class name.
For example instead of "ProjectRowExpressionRewrite" we will log RemoveMapCastRule$Rewriter:ProjectRowExpressionRewrite" now.

Similar for StatsRecordingPlanOptimizer, it's a wrapping optimizer, and it's the delegate optimizer within it doing the work. After the change, it will show as "StatsRecordingPlanOptimizer:HistoricalStatisticsEquivalentPlanMarkingOptimizer" instead of "StatsRecordingPlanOptimizer".

Motivation and Context

To make optimizer log more meaningful.

Impact

Improve optimizer log

Test Plan

Test locally end to end

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Improve logging for RowExpressionRewriteRuleSet and StatsRecordingPlanOptimizer optimizers to include more information :pr:`22765`

@jaystarshot
Copy link
Member

jaystarshot commented May 15, 2024

This also happens for StatsRecordingPlanOptimizer i.e optimizers which extend that show up as StatsRecordingPlanOptimizer, wondering if we could fix that too but that would be a different PR

else if (rule instanceof RowExpressionRewriteRuleSet.TableFinishRowExpressionRewrite) {
ruleName = ((RowExpressionRewriteRuleSet.TableFinishRowExpressionRewrite) rule).getOptimizerNameForLog();
}
else if (rule instanceof RowExpressionRewriteRuleSet.TableWriterRowExpressionRewrite) {
Copy link
Member

Choose a reason for hiding this comment

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

On adding a new rule you will have to extend this, wondering if there was a better way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was trying to find a better way to achieve this but cannot think of one. Open to suggestions if there are better ways.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would the following patch work for you @feilong-liu ? I think you can just extend the rule interface to add a method which does this, and extend all of the rules using the class with the method instead, and check if it is an instance of the abstract class e.g.

    public abstract class RowExpressionRewriteRule<T> implements Rule<T> {
        public String getOptimizerNameForLog() {
            String rewriterName = rewriter.getClass().getName();
            return format("%s:%s", rewriterName.substring(rewriterName.lastIndexOf('.') + 1), this.getClass().getSimpleName());
        }
    }

// rule definition
public final class ProjectRowExpressionRewrite
            extends RowExpressionRewriteRule<ProjectNode>
...
// name check 
        if (rule instanceof RowExpressionRewriteRuleSet.RowExpressionRewriteRule) {
            ruleName = ((RowExpressionRewriteRuleSet.RowExpressionRewriteRule) rule).getOptimizerNameForLog();
        }

simplify-rulename.patch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion. Applied the change.

@feilong-liu
Copy link
Contributor Author

This also happens for StatsRecordingPlanOptimizer i.e optimizers which extend that show up as StatsRecordingPlanOptimizer, wondering if we could fix that too but that would be a different PR

This is not a big change, added in this PR too.

@feilong-liu feilong-liu changed the title Log detail name for optimizers of RowExpressionRewriteRuleSet Log detail name for optimizers of RowExpressionRewriteRuleSet and StatsRecordingPlanOptimizer May 15, 2024
jaystarshot
jaystarshot previously approved these changes May 17, 2024
Copy link
Member

@jaystarshot jaystarshot 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 see an immediate solution to better log rules, maybe rules need to have a field with info about the optimizer which is using but this can be refactored if needed in future.

@jaystarshot
Copy link
Member

@ClarenceThreepwood Do you have any other ideas?

@feilong-liu feilong-liu merged commit 568784e into prestodb:master May 17, 2024
56 checks passed
@feilong-liu feilong-liu deleted the log_rewriterule branch May 17, 2024 18:54
@wanglinsong wanglinsong mentioned this pull request Jun 25, 2024
36 tasks
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