fix: from_text sets relation type earlier#6066
Conversation
prql-bot
left a comment
There was a problem hiding this comment.
The core fix looks right — I checked out the branch, ran the exact #5988 repro (zero-row from_text format:json joined side:left), and it now compiles to the expected SQL; the full from_text suite plus cargo test -p prqlc and clippy all pass. Setting ty on the result so the empty array resolves to Array(Some(Tuple(..))) instead of Array(None) is the clean place to fix it.
On your question about the "never supposed to happen" branches — this repo already has a convention for exactly that. Error::new_assert(details) renders as internal compiler error; <details>, and Error::new_bug(issue_no) adds a tracking-issue link (see the uses in prqlc/prqlc/src/semantic/lowering.rs and functions.rs:286). Using new_assert for the pure type-kind invariants (the into_tuple/into_function/into_array wrong-kind cases) frames them as compiler bugs rather than user errors, and keeps the {value:#?} debug dump behind that framing. The genuinely user-reachable cases (e.g. a join with that really has no columns) are fine as new_simple.
One thing worth reconsidering is the blanket impl From<TyKind> for Error (flagged inline) — scoping the conversion locally per call site would answer both this and the message-style question in one move.
Resolves #5988 by populating the type of the Expr returned by
from_textearlier in the resolver flow, and adds a covering test.While I was looking at it, I attempted to improve some of the error handling of
infer_type_of_special_func(where the initial crash was located). There are many.unwrap()calls in the various branches of thematchstatement; I converted some of them to the?try operator and/or replaced.unwrap()with.ok_or(...)?. The main challenge here is that many of these error conditions are "never supposed to happen" because the type system is supposed to catch bad user input. I don't know if there's a better/recommended strategy for handling this sort of situation -- crashing on unwrapping a None or Err is not good, but hand-writing an individual error message to replace each unwrap call seems excessive. Suggestions are welcome...