[ https://issues.apache.org/jira/browse/CALCITE-3386?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16946408#comment-16946408 ]
Julian Hyde commented on CALCITE-3386: -------------------------------------- I don't think you should back out the change. We should continue discussing here. There is a pattern where exceptions are used for control flow. Unlike {{FileNotFoundException}}, which signals the state of an external system, they are completely internal. For example, consider how {{class Util.OverFinder}} always throws {{FoundOne.NULL}}, and how it is always caught. We throw an exception because it is more concise than having each function call return a special value. As a developer, if I see that the stack trace of an exception contains "<clinit>" (as I suspect this one did), that is a good clue that the exception is static. Then I go to the exception class and I see that the javadoc states that the exception is used for signaling. That's an idiom, but once you know the idiom, I think that is pretty clear. The stackoverflow link, above, justifies the change in terms of the error given to the end user, but I don't think that justification applies here. The metadata system throws and catches CyclicMetadataException in order to do a form of backtracking. That algorithm is fairly subtle. Understanding how CyclicMetadataException is used for control flow - i.e. is not really an exception - is key to understanding the algorithm. > CyclicMetadataException gives misleading stack trace > ---------------------------------------------------- > > Key: CALCITE-3386 > URL: https://issues.apache.org/jira/browse/CALCITE-3386 > Project: Calcite > Issue Type: Bug > Components: core > Reporter: Zuozhi Wang > Priority: Major > Labels: pull-request-available > Fix For: 1.22.0 > > Time Spent: 1.5h > Remaining Estimate: 0h > > Calcite currently reuses the same instance when throwing > CyclicMetadataException as shown below. > {code:java} > /** Singleton instance. Since this exception is thrown for signaling > purposes, * rather than on an actual error, re-using a singleton instance > saves the * effort of constructing an exception instance. */ > @SuppressWarnings("ThrowableInstanceNeverThrown") > public static final CyclicMetadataException INSTANCE = new > CyclicMetadataException(); > {code} > > Reusing the same exception instance gives the wrong stack trace, see: > [https://stackoverflow.com/questions/15090664/is-it-safe-to-store-an-instance-of-an-exception-and-reuse-it] > The misleading stack trace causes many confusions when debugging. The > potential performance impact because of object construction is minor compared > to the time spent on debugging. Therefore it should be changed to not reuse > the singleton instance. > A pull request have been opened to fix this issue: > [https://github.com/apache/calcite/pull/1484] > -- This message was sent by Atlassian Jira (v8.3.4#803005)