Types and unwrapping panics

[frankmcsherry]

Our use of dynamic typing (or .. no typing?) leads to some awkward moments where types are not right and it is a bit hard to immediately see why.

From Zulip:

---

Frank McSherry: @jamie Brandon I've not fully debugged this, but I'm seeing a panic in unwrap_int32() in

match (aggregates[0].0).func {
    AggregateFunc::SumInt32 => {
        data.explode(|(key,val)| Some((key,val.unwrap_int32() as isize)))
            .count()
            .map(|(mut key,sum)| { key.push(Datum::Int32(sum as i32)); key })
}

where it claims we have a Datum::Int64 as input. It could easily be some screw-up on my part, but is it expected that all the types are correct all the way through?

Frank McSherry: Wait, NO! My fault sorry. It wasn't there, but rather that (elsewhere) I failed to wrap the output of Count as Datum::Int64 and had instead wrapped it as Datum::Int32.

---

This may just be an unavoidable pain of a language that needs to support a mix of numeric, boolean, and other (string) data in the same positions. However, anything we can think to do to detect these issues earlier might help.

[benesch]

I'm not sure I follow, @frankmcsherry. Our implementation of SQL is statically typed; SQL queries that are not well-typed are rejected before they are lowed to IR. It's true that bugs in our type checker/IR transformations (or miscommunications between the two) can cause type errors at runtime... but that's morally equivalent to bugs in LLVM that generate invalid assembly for valid C code, isn't it?

I suspect we're going to want to make this problem worse over time, not better. I.e., datums have a discriminant, so unwrap_int32() can take down the process if called on a non-int32 datum, while in the future, when we drop the discriminant for memory usage's sake, we'll probably happily transmute your string into an int32 (or at least we will in release mode).

I guess what I'm saying is it's not clear to me what more we could do. I realize that probably sounds pessimistic, and I'm definitely open to something more sophisticated! But short of formal verification it's not clear to me that there's anything else to do here besides test extensively, and we're already ahead of the curve on that.

[benesch]

Well, I suppose we could lean on the Rust compiler's type checker if we generated Rust code and compiled it via LLVM for every query, but that just trades one set of issues for another.

[frankmcsherry]

Sorry, I should have put more context. This was from Zulip, and to satisfy the request that this be committed to the public record. It is not exactly a well-considered proposal for how to make the world better (not yet!).

I think what I was getting at is that there are several opportunities for us (me, let's say) to get things wrong, and we could in principle work to minimize these. For example, we could unify i32 and i64 types into a common isize-ish type, and pass the responsibility / semantics on to the SQL layer who needs to make sure it is ok with e.g. the rounding behavior. We could drop the Avg aggregate expression and make it the responsibility of the SQL layer to produce both sum and count aggregates. These aren't necessarily good ideas, but they reduce the surface of invariants to maintain while working with the IR, and might make it more accessible to others.

No urgent failings that need to be fixed, but a gradient of footgun disarmament that we can idly consider. :D