[jira] [Comment Edited] (CALCITE-2537) Make sure assertions are enabled before calling VolcanoPlanner#validate

["Laurent Goujon (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CALCITE-2537?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16605069#comment-16605069 ] 

Laurent Goujon edited comment on CALCITE-2537 at 9/6/18 12:05 AM:
------------------------------------------------------------------

It's not just the logging but the fact that {{validate}} checks the whole planner tree (from sets to subsets to then validating cost of each node in the subset) which take most of the time AND also throw an {{AssertionError}} (because this is not supposed to happen), so this is more than just some extra logging. I would prefer that checking log level is reserved when actually printing out log statements (which is not the case here).

When working with external users to diagnose issue, asking them to turn all the logs to debug have caused us to have a big slowdown and also from time to time to get a different error from what we were currently trying to debug (see my other ticket CALCITE-2538 as an example).

{{Objects.requireNonNull}} is more of a runtime check/preconditions than an assertion so I wouldn't classify it the same way. My suggestion is to add both the logger check and the assertion check to guard validate (It would actually be nice to have it enabled for tests whatever the log level, but the performance hit is really big).


was (Author: laurentgo):
It's not just the logging but the fact that validate checks the whole planner tree (from sets to subsets to then validating cost of each node in the subset) which take most of the time AND also throw an AssertionError (because this is not supposed to happen), so this is more than just some extra logging.

When working with external users to diagnose issue, asking them to turn all the logs to debug have caused us to have a big slowdown and also from time to time to get a different error from what we were currently trying to debug (see my other ticket CALCITE-2538 as an example).

{{Objects.requireNonNull}} is more of a runtime check/preconditions than an assertion so I wouldn't classify it the same way. My suggestion is to add both the logger check and the assertion check to guard validate (It would actually be nice to have it enabled for tests whatever the log level, but the performance hit is really big).

> Make sure assertions are enabled before calling VolcanoPlanner#validate
> -----------------------------------------------------------------------
>
>                 Key: CALCITE-2537
>                 URL: https://issues.apache.org/jira/browse/CALCITE-2537
>             Project: Calcite
>          Issue Type: Improvement
>          Components: core
>            Reporter: Laurent Goujon
>            Assignee: Julian Hyde
>            Priority: Minor
>
> {{VolcanoPlanner#validate()}} is invoked if the log level is debug or lower. But considering how enabling this method slows down the whole planning (changing it for running tests make the whole duration several hours), and this is some kind of assertion (since it throws {{AssertionError}}, I would suggest to only enable it when assertions are enabled



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)