Repository navigation
sql: Final tweaks to TRY_CAST - #39834
Conversation
antiguru
left a comment
antiguru
left a comment
There was a problem hiding this comment.
I think this is good, my bot has some comments. Please take a look before merging.
| /// become NULL. A function that cannot error has nothing to fall back | ||
| /// from and is returned as is, keeping its nullability. | ||
| pub fn try_cast(inner: UnaryFunc<E>) -> UnaryFunc<E> { | ||
| pub fn try_cast_if_fallible(inner: UnaryFunc<E>) -> UnaryFunc<E> { |
There was a problem hiding this comment.
You could take inner: Into<UnaryFunc<E>> and avoid the into() calls on the arguments.
| // like any other failure and the error is logged instead. | ||
| Err(EvalError::Internal(message)) => { | ||
| mz_ore::soft_panic_no_log!( | ||
| mz_ore::soft_panic_or_log!( |
There was a problem hiding this comment.
This runs once per row, so in production soft_panic_or_log! becomes a tracing::error! for every row. A single planner bug that hands inner the wrong datum kind on a large collection would flood the logs, and probably Sentry too. Consider logging once per process, for example behind a static AtomicBool, or rate-limiting it. Keep the soft panic for CI.
Posted by Claude Code.
| /// `inner` can error: [`UnaryFunc::try_cast_if_fallible`] is the only constructor, and it | ||
| /// returns a function that cannot error unwrapped. The wrapper's nullability | ||
| /// relies on this, since a wrapped function always has a NULL outcome. |
There was a problem hiding this comment.
TryCast still derives Deserialize, so try_cast_if_fallible is not the only constructor. A deserialized plan can hold a wrapper around an infallible inner. That causes no harm, because introduces_nulls() == true and the forced-nullable output type over-approximate for any inner. It does mean the nullability does not rely on this invariant. Either reword it to say that planner-built wrappers always hold a fallible inner and the forced nullability is conservative otherwise, or enforce the invariant with #[serde(try_from = ...)]. Also, "returns a function that cannot error unwrapped" reads ambiguously, and line 25 runs past the usual wrap width.
Posted by Claude Code.
| be typed on its own like the other constructors, and there is no cast | ||
| between two [`map`](../../types/map/) types to convert it afterward. Convert | ||
| it with `CAST` or `::` instead. | ||
| An empty constructor has no elements to convert, so it takes its type from |
There was a problem hiding this comment.
nit: The bullet above lists ROW, so this sentence reads as covering ROW() too. plan_cast has no Expr::Row arm, though, so an empty ROW() never takes its type from the target in either mode. Consider saying "An empty ARRAY, LIST, or MAP constructor".
Posted by Claude Code.
| bare => assert!( | ||
| !strict_stage.could_error() && bare == strict_stage, | ||
| "{source} to {to_name}: TRY_CAST left {strict_stage} unwrapped" |
There was a problem hiding this comment.
nit: This message also fires when the stage is infallible but bare != strict_stage, and in that case "left unwrapped" is misleading. Consider splitting the two conditions into separate asserts. Separately, the length assert_eq! before zip_eq is redundant, since zip_eq already panics on a length mismatch. It does give a clearer message, so keeping it is fine.
Posted by Claude Code.
| (Expr::Array(exprs), mode) if mode == CastFailureMode::Error || exprs.is_empty() => { | ||
| plan_array(ecx, exprs, Some(&to_scalar_type))? | ||
| } | ||
| (Expr::List(exprs), mz_expr::CastFailureMode::Error) => { | ||
| (Expr::List(exprs), mode) if mode == CastFailureMode::Error || exprs.is_empty() => { | ||
| plan_list(ecx, exprs, Some(&to_scalar_type))? | ||
| } | ||
| (Expr::Map(exprs), mz_expr::CastFailureMode::Error) => { | ||
| (Expr::Map(exprs), mode) if mode == CastFailureMode::Error || exprs.is_empty() => { |
There was a problem hiding this comment.
nit: The guard mode == CastFailureMode::Error || exprs.is_empty() appears three times. A small helper, such as let keeps_hint = |exprs: &[Expr<Aug>]| failure_mode == CastFailureMode::Error || exprs.is_empty();, would name the decision once. Reasonable people could disagree.
Posted by Claude Code.
- Keep the type hint for empty ARRAY, LIST, and MAP constructors under TRY_CAST, which have no element casts to protect. Only a non-empty MAP constructor is still rejected. - Make TryCast's inner function private, with UnaryFunc::try_cast_if_fallible as the only constructor, so a wrapper always holds a fallible function and is always nullable. - Use the parser's CastFailureMode throughout and drop the mz_expr copy. - Document the chain shape a cast template needs for TRY_CAST. - Pin how TRY_CAST plans with EXPLAIN over columns: a single fallible stage, an infallible cast, multi-stage chains, and the optimized plan. - Move the adapter's TRY_CAST tests into catalog/try_cast_tests.rs. - Put the refused-cast remedy in a hint, and log a swallowed internal error in production instead of staying silent. - Mark TRY_CAST as added in v26.46, and drop SqlScalarType::contains, whose only caller was the "char" exclusion the property test no longer needs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…es (#39898) Follow-up docs for `TRY_CAST` (#38845, #39834). - **Release gating** - `TRY_CAST` ships in v26.47, not v26.46. Its `version-added` in the function list now says so, so the list shows it as unreleased until then. - The `TRY_CAST` section of the CAST page, already live, gets an unreleased warning. - **Why `TRY_CAST` matters** - The CAST page says what a failing cast does: in a materialized view or an indexed view, one bad row fails every query against the view until the row is gone. - **Pointers from error-prone casts** - The Kafka JSON format details, the webhook JSON parsing section, and the sensitive-columns pattern suggest `TRY_CAST` and link to its section, which carries the unreleased warning. Pages to review: - https://preview.materialize.com/materialize/39898/sql/functions/cast/#try_cast - https://preview.materialize.com/materialize/39898/sql/create-source/kafka/#format-json - https://preview.materialize.com/materialize/39898/sql/create-source/webhook/#json-parsing - https://preview.materialize.com/materialize/39898/security/patterns/protect-sensitive-columns/#upstream-errors-reach-the-exposed-view 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
These tweaks didn't make it into
TRY_CAST#38845, which accidentally merged one step too early.