[
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)
