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
Fix issue with unwanted Error when --log-each-select is set to false #680
base: main
Are you sure you want to change the base?
Fix issue with unwanted Error when --log-each-select is set to false #680
Conversation
@bajinsheng could you please review this PR, since you have recently worked on the logic for writing out query plans? |
@@ -72,8 +72,10 @@ protected void executeEpilogue(Query<?> q, boolean success, ExecutionTimer timer | |||
if (success && getOptions().printSucceedingStatements()) { | |||
System.out.println(q.getLogString()); | |||
} | |||
if (logExecutionTime) { | |||
getLogger().writeCurrent("// " + timer.end().asString()); | |||
if (getOptions().logEachSelect()) { |
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.
I think the logic would be slightly easier to understand if you have the logic in one if statement (here and above): if (getOptions().logEachSelect() && logExecutionTime)
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.
Sure! Fixed in a newer commit.
@@ -20,7 +20,7 @@ protected void executeEpilogue(Query<?> q, boolean success, ExecutionTimer timer | |||
if (success && getOptions().printSucceedingStatements()) { | |||
System.out.println(q.getQueryString()); | |||
} | |||
if (logExecutionTime) { | |||
if (getOptions().logEachSelect() && logExecutionTime) { |
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.
Perhaps it is more logical clear to put this condition into the logExecutionTime()
function.
@@ -156,9 +156,6 @@ public boolean printSucceedingStatements() { | |||
} | |||
|
|||
public boolean logExecutionTime() { | |||
if (!logEachSelect) { | |||
throw new AssertionError(); |
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.
We just need to return false here. I think we do not need to update other code locations.
Attempt to fix #679
Manually tested on MySQL