[CALCITE-7728] Linq4j can simplify expressions without regards for 'safety' - #5192
Conversation
mihaibudiu
changed the title
| * | ||
| * <p>The analysis is deliberately cheap and conservative. Any method call or | ||
| * constructor may throw; so may division, remainder and checked arithmetic. | ||
| * Everything else - reading a variable, a field or an array element, and |
There was a problem hiding this comment.
accessing an array element could lead to IndexOutOfBoundException, should we be "hiding" those?
There was a problem hiding this comment.
After a complete audit of all operations that can throw, this visitor will become more complicated, but hopefully safe. I will submit a new commit.
|
@rubenada you were right, the analysis wasn't conservative enough. I added a second commit which covers all the Expression kinds supported today (and the future ones are treated conservatively). |
| assertMayThrow(Expressions.convert_(i, Integer.class), false); | ||
| assertMayThrow(Expressions.convert_(str, Object.class), false); | ||
|
|
||
| // A cast that may raise ClassCastException, and unboxing |
There was a problem hiding this comment.
nit: this comment seems incomplete? ... , and unboxing
Do you mean ... , and unboxing may throw NPE ?
There was a problem hiding this comment.
I will fix this and squash the commits at the same time.
rubenada
left a comment
rubenada
left a comment
There was a problem hiding this comment.
LGTM.
Thanks for the thorough tests that you added!
…afety' Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
mihaibudiu
added
the
LGTM-will-merge-soon
|
2 participants
Jira Link
CALCITE-7728
Changes Proposed
This PR adds a cheap visitor to approximate expression "safety", similar to the other layers of the representation. The visitor is used to avoid optimizing expressions that may throw.