Repository navigation
[CALCITE-5845] Select a very large predicate throw threw 'java.lang.StackOverflowError' exception in SqlNode to RelNode phase - #5384
Conversation
46754dc to
06d7666
Compare
mihaibudiu
left a comment
mihaibudiu
left a comment
There was a problem hiding this comment.
I see 6 changes and only 4 tests.
Does each change have coverage in the new tests?
Thank you for the point, these tests only covered the convertlet change; the rest got flat n-ary input (depth 1) and decomposeDisjunction wasn't reached at all. |
| List<RexNode> rexList, | ||
| List<RexNode> notList) { | ||
| if (rexPredicate == null || rexPredicate.isAlwaysTrue()) { | ||
| // Use an explicit stack to avoid StackOverflowError on deeply nested ANDs |
There was a problem hiding this comment.
only ANDs or deep expressions in general?
There was a problem hiding this comment.
General, not just ANDs, the loop handles nodes of any kind, and NOT delegates a deep OR operand to the also-iterative decomposeDisjunction. Comment updated to 'deeply nested expressions'.
| continue; | ||
| } | ||
| final RexCall call = (RexCall) current; | ||
| if (!isFlat(call.getOperands(), call.getOperator())) { |
There was a problem hiding this comment.
I haven't checked if this function may be recursive.
There was a problem hiding this comment.
It's non-recursive and it only loops over an explicit ArrayDeque, and the private list overload is the original non-recursive exists() check. testIsFlatDeeplyNested verifies it: overflows against the old code on a 256KB stack, passes in latest commit.
| * <a href="https://issues.apache.org/jira/browse/CALCITE-5845">[CALCITE-5845] | ||
| * StackOverflowError when SQL has a very large predicate with many ANDs</a>. | ||
| * | ||
| * <p>Verifies that {@link RexUtil#flattenAnd} does not recurse when given a |
There was a problem hiding this comment.
Strictly speaking it's not checking the lack of recursion, but the ability to compute with a small stack.
There was a problem hiding this comment.
Make sense, the test checks "completes on a small stack", which is the observable contract. Reworded all six javadocs accordingly.
| /** Runs {@code callable} on a thread with a small stack, so that a recursive | ||
| * implementation of the callee overflows quickly, while an iterative | ||
| * implementation completes. */ | ||
| private static <T> T runInSmallStack(Callable<T> callable) { |
There was a problem hiding this comment.
how does this differ from the other function with the same name?
There was a problem hiding this comment.
It doesn't differ, because they were identical copies. Extracted into a shared org.apache.calcite.test.SmallStack used by both classes In a recent commit.
|
…tackOverflowError' exception in SqlNode to RelNode phase
8f008a3 to
92e01a3
Compare
|
I had squashed the commits and would merge it if there no more comments in coming days. |
xuzifu666
added
the
LGTM-will-merge-soon
jira: https://issues.apache.org/jira/browse/CALCITE-5845
This pr is aimed to fix StackOverflowError, without this changes test such as
testLargeAndPredicatewould error out with 'java.lang.StackOverflowError'.