Add defensive debug asserts in simplify to ensure schema doesn't change

[alamb]

Is it possible to be more defensive about this, such as a (debug) assert that checks this property at the callsite of simplify in the planner? 🤔

Originally posted by @Jefffrey in #17981 (review)

[pepijnve]

If we add this assertion, we're likely to see the same problem as #17801 pop up since the coalesce simplification introduced in #17357 currently violates the proposed assertion. PR #17813 is an attempt to resolve that by ensuring the nullability of the original coalesce expression and the case based simplification output match.

[EeshanBembi]

I've investigated the codebase and identified where the defensive debug assertions should be added.
The key location is in https://github.com/apache/datafusion/blob/main/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs#L1579, where udf.simplify() is called for ScalarFunction expressions.

The assertion should check that both the data type and nullability remain unchanged after
simplification, using the SimplifyInfo trait's get_data_type() and nullable() methods that are
already available in the context.

@pepijnve is absolutely right about the coalesce issue. Adding this assertion will indeed surface
the #17813 problem immediately, which is actually a good thing, it will force us to properly fix
the underlying schema contract violations rather than allowing them to silently persist.

Proposed approach:

1. Add the debug assertion to catch schema changes during simplification
2. Fix the coalesce nullability issue (as addressed in #17801 Improve nullability reporting of case expressions #17813)
3. Audit other UDF simplify implementations to ensure they maintain schema consistency

The debug assertion would look something like:

  debug_assert!(
      original_data_type == simplified_data_type &&
      original_nullable == simplified_nullable,
      "UDF simplify() must not change schema (data type or nullability). \
       Original: {:?}, Simplified: {:?}. See issue #18001",
      original_expr, simplified_expr
  );

This approach will help us maintain the simplify contract while identifying violations during
development.

Benefits:

• Catches schema violations early in development
• Zero runtime cost in release builds
• Forces proper fixes rather than masking issues
• Maintains backward compatibility

Would you like me to prepare a PR with this implementation?

[pepijnve]

Just for completeness' sake, it looks like I got my numbers mixed up. The issue I had meant to refer to is issue #17801 which was about a panic in one of the planner benchmarks that was caused by the schema change.