[CALCITE-3460] Poor performance in RexReplacer for large queries #1547
Conversation
We have queries that have tens of thousands of RexCalls. reducibleExps.indexOf(call) is an (O) operation, which takes 50% of the running time, causing the query runs for ever until timed out. In RexShuttle, ImmutableList iterator creation in visitList takes another 5~7% of running time, and it is creating millions of temporary iterator object, not only time consuming, but also memory consuming.
| int i = reducibleExps.indexOf(call); | ||
| if (i == -1) { | ||
| Integer i = reducibleExpsMap.get(call); | ||
| if (i == null) { |
There was a problem hiding this comment.
Why not just use a HashSet ?
There was a problem hiding this comment.
the value i indicates the index to find value in reducedValues
| for (RexNode expr : exprs) { | ||
| outExprs.add(expr.accept(this)); | ||
| for (int i = 0; i < exprs.size(); i++) { | ||
| outExprs.add(exprs.get(i).accept(this)); |
There was a problem hiding this comment.
If the ImmutableList iteration has bad performance, maybe we should promote and fix ImmutableList, then more code would benefit from that fix.
There was a problem hiding this comment.
There is nothing wrong with ImmutableList iterator. It is just not a good fit here.
| this.reducedValues = reducedValues; | ||
| this.addCasts = addCasts; | ||
| for (int i = 0; i < reducibleExps.size(); i++) { | ||
| reducibleExpsMap.put(reducibleExps.get(i), i); | ||
| } |
There was a problem hiding this comment.
LGTM ~
shall we also save reducedValues by map ? Thus to save memory.
Please ignore if it gains little.
| int i = reducibleExps.indexOf(call); | ||
| if (i == -1) { | ||
| Integer i = reducibleExpsMap.get(call); | ||
| if (i == null) { | ||
| return null; | ||
| } | ||
| RexNode replacement = reducedValues.get(i); |
There was a problem hiding this comment.
Depending on the implementation of List, reducedValues.get(i) and addCasts.get(i) can be O(n) complexity as well. Right now we are using ArrayList, which is fine. But could be a problem potentially in the future.
There was a problem hiding this comment.
+1, better to update it too.
There was a problem hiding this comment.
reducedValues is not a local variable, and it would be a non-trival task to refactor this code. So I suggest we merge the PR as is.
|
It would be great if you can provide a test case that shows the performance improvement. |
danny0405
force-pushed
the
master
branch
2 times, most recently
from
80f411d
to
ca27fe9
Compare
|
LGTM |
|
In case you missed my comment in the JIRA case, we must have a unit test for this change. |
vlsi
force-pushed
the
master
branch
2 times, most recently
from
49cb002
to
8768a23
Compare
We have queries that have tens of thousands of RexCalls.
reducibleExps.indexOf(call) is an (O) operation, which takes 50% of the running
time, causing the query runs for ever until timed out. In RexShuttle,
ImmutableList iterator creation in visitList takes another 5~7% of running
time, and it is creating millions of temporary iterator object, not only time
consuming, but also memory consuming.