[mihaibudiu]

Jira Link

CALCITE-7870

Changes Proposed

The code for deciding which casts are lossless has been factored out in a separate function and scrutinized for bugs.
The rules should not necessarily assume that the Calcite Java-based runtime is used.

[mihaibudiu]

I mistakenly converted this to draft; this PR is in fact ready for review.
There are lots of comments explaining each of the small changes, so I expect it should not be a difficult review.

[asolimando]

LGTM, left two questions on a few details I am not sure of, but nothing major, thanks Mihai!

[asolimando]

Since branch A already handles outer == source, this clause only ever fires for same-name/different-scale, e.g. DECIMAL(5,2)::VARCHAR::DECIMAL(3,1) -> DECIMAL(5,2)::DECIMAL(3,1). I wonder if parsing '12.25' into (3,1) vs rescaling 12.25 may round differently?

[mihaibudiu]

Are you saying we are missing a case which we should reject?

This code runs in the simplifier, so it should be dialect-independent. The simplifier generally should not make assumptions about how the runtime will do something. It should remove casts only when it's guaranteed they will not change results under any reasonable runtime. If we are not sure, we should not remove the cast.

The compiler does depend on the runtime in the reduce_* functions used in the rewrite passes, but the simplifier should not.

[asolimando]

That's basically my worry, yes, I wouldn't know how to effectively check if some cases like the one I mentioned wouldn't cause problem for any possible runtime system.

To be clear, I think this PR is a net improvement, it's more a general "testability/validation" question, it's easier when you have a reference target system, supporting any system is trickier.

[mihaibudiu]

I cannot guarantee that we won't find more exceptions in the future. But I think the ones added now are justified. Calcite's flexibility makes this difficult to test.

[asolimando]

That would be equal to ask "no bugs" in code, that wasn't my point, and as I said the PR is already a solid improvement that I will take.

I was interested to hear if you had opinions on how to test this more broadly, possibly with the help of agents. I know that in your stack you have DataFusion (Arrow types) and a Rust DataFlow runtime, I was wondering if there was a way to come up with a test harness for an agent to test all combinations with runtime values and spot more exceptions, like trying to find a counter example (something like https://arxiv.org/pdf/2403.03193 is doing to "validate" rewrite rules, finding a minimal counter example).

If we can solve this for one given runtime (say even DF and Arrow), we can write down what was done and users with different runtimes can do the same and open tickets/PRs to refine what we have.

It's a way broader discussion beyond this PR, feel free to disregard if you don't have time, I will approve the PR regardless as it's a solid improvement, just curious to hear your thoughts on this.

[mihaibudiu]

Despite what that paper claims, I think that formal verification is rather hopeless for real implementations of of real SQL because the SQL spec is rather loose, and each dialect makes different choices. Perhaps if you fix ONE dialect then you can do something by proving that some rewrites do not produce plans with equivalent semantics, but Calcite is supposed to be very flexible and can emulate features from many dialects based on various configuration flags.

Agents do change the equation, since they never get tired writing test cases or debugging test failures; they are seemingly also good at writing proofs; tying proofs to the actual code is another matter, though. I don't think Java has good enough tooling for this; many people are using Verus these days for Rust.

In my recent testing I have been asking agents to generate "all possible combinations" of tests for some reduced test matrices and to compare our results against postgres. This works very well when the Calcite semantics aligns with postgres. I have also been comparing our Rust runtime with the calcite constant folding implementation -- by evaluating the same expression on constants or on runtime data -- by turning on and off the REDUCE_* passes in our Calcite optimizer. Any discrepancy requires investigation: could be our bug or a Calcite bug. In the case of this PR the agent figured out that these rewrites happen even without the constant folding... so these are really bugs.

I am not sure how useful this answer is. I am still learning how to best do things.

[mihaibudiu]

I asked one of these clever agents to analyze all combinations and it found one more, which I will add as D.5, involving VARIANT:

tinyintExpr :: smallint :: variant => must not be changed
    //      a VARIANT keeps the type of its value; TYPEOF gives
    //      right: SMALLINT, wrong: TINYINT

[mihaibudiu]

I have added a second commit with the VARIANT rule.

[asolimando]

(I guess your agent force-pushed, it happened to me too in the past, I successfully prevented it by asking the agent to add a global instruction disallowing it for in-flight reviews)

[asolimando]

LGTM, thanks Mihai for this improvement

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
98.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud