calcite icon indicating copy to clipboard operation
calcite copied to clipboard

[CALCITE-6071] RexCall should carry source position information for runtime error reporting

Open mihaibudiu opened this issue 1 year ago • 4 comments

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.

mihaibudiu avatar Nov 07 '23 00:11 mihaibudiu

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.

mihaibudiu avatar Dec 06 '23 21:12 mihaibudiu

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.

mihaibudiu avatar Aug 13 '24 06:08 mihaibudiu

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.

mihaibudiu avatar Sep 20 '24 17:09 mihaibudiu

I have resolved the conflicts, rebased and optimistically squashed the commits too

mihaibudiu avatar Sep 20 '24 17:09 mihaibudiu

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!

asolimando avatar Sep 20 '24 18:09 asolimando