calcite
calcite copied to clipboard
[CALCITE-6071] RexCall should carry source position information for runtime error reporting
This PR adds source position (SqlParserPos
) information to two kinds of nodes:
- RexCall
- AggregateCall
Not all RexCall nodes and AggregateCall operations need to have meaningful source position information. The most useful nodes are the ones that could lead to runtime errors. So operations like addition (which can overflow) or casts (which can fail when the target type is too narrow) need to carry such information. Nodes where the operation cannot fail dynamically (e.g., MAX, comparisons, or IS_NULL) may have the source position set to ZERO.
In this PR I have carefully audited the entire code base and tried to thread the flow of the position information everywhere, but there are a few places where I couldn't figure out where it should come from. There are, for example, a few casts inserted by the rewrite rules which have no direct correspondence to source constructs. Such casts have now a TODO comment wondering about their runtime safety.
In the process I have also removed a few @Deprecated
functions which would have required a signature change. I figured that since they are deprecated there is no point to update them, since no one can call the variant with the new signature.
For methods where there are many calls where the position information may legitimately be ZERO I have left variants of the original methods without source position information (e.g., see RexBuilder.makeCall)
Currently there are no uses of this information, but I have inspected the IR with a debugger after optimizations to check that the information is present. A future commit (part of this PR or another one) should add an example runtime error that displays this information. This is not trivial to do for the Calcite Linq4j-based executor, so I haven't done it yet.
None of this should affect the behavior of any of the compiled programs, but I noticed that it's easy to mess up something in practice a few times. Currently no tests need to change.
My plan is to merge #3522 first, then try to use this infrastructure for one of these runtime cast failures to give a better error message at runtime. But I hope that the part that I have done so far can be reviewed on its own as well. It's not guaranteed to be complete, but we will only know that when we try to use it.
Quality Gate passed
Issues
12 New issues
0 Accepted issues
Measures
0 Security Hotspots
73.0% Coverage on New Code
0.8% Duplication on New Code
Quality Gate passed
Issues
13 New issues
0 Accepted issues
Measures
0 Security Hotspots
71.7% Coverage on New Code
0.5% Duplication on New Code
Quality Gate passed
Issues
13 New issues
0 Accepted issues
Measures
0 Security Hotspots
71.3% Coverage on New Code
0.5% Duplication on New Code
I really would like to merge this PR, but no one wants to review it. Since it doesn't change any of the functionality of Calcite (only adds a new orthogonal feature) the risk is low. But it is a bit tedious, I agree.
During development I verified that there are no missing cases by deleting the old functions without source position and re-enabling them only after I checked that the existing ones cover all cases. I will rebase. The tests should pass.
I have resolved the conflicts, rebased and optimistically squashed the commits too
Quality Gate passed
Issues
12 New issues
0 Accepted issues
Measures
0 Security Hotspots
71.4% Coverage on New Code
0.5% Duplication on New Code
I have resolved the conflicts, rebased and optimistically squashed the commits too
As soon as the last tests finish successfully you can go ahead and merge. Thanks for your great work @mihaibudiu!