You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2025/10/09 18:49:34 UTC
[I] Add defensive debug asserts in simplify to ensure schema doesn't change [datafusion]
alamb opened a new issue, #18001:
URL: https://github.com/apache/datafusion/issues/18001
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 https://github.com/apache/datafusion/pull/17981#pullrequestreview-3316979570_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Add defensive debug asserts in simplify to ensure schema doesn't change [datafusion]
Posted by "EeshanBembi (via GitHub)" <gi...@apache.org>.
EeshanBembi commented on issue #18001:
URL: https://github.com/apache/datafusion/issues/18001#issuecomment-3405031849
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 #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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Add defensive debug asserts in simplify to ensure schema doesn't change [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on issue #18001:
URL: https://github.com/apache/datafusion/issues/18001#issuecomment-3393190020
If we add this assertion, we're likely to see the same problem as #17813 pop up since the `coalesce` simplification introduced in #17357 currently violates the 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Add defensive debug asserts in simplify to ensure schema doesn't change [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on issue #18001:
URL: https://github.com/apache/datafusion/issues/18001#issuecomment-3405341265
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 caused by the schema change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org