This is an automated email from the ASF dual-hosted git repository.
github-bot pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/datafusion.git
The following commit(s) were added to refs/heads/main by this push:
new c728d54819 refactor: Add `assert_or_internal_err!` macro for more
ergonomic internal invariant checks (#18511)
c728d54819 is described below
commit c728d54819ef33c9a3c5f0279cdcc4dd3d8b8661
Author: Yongting You <[email protected]>
AuthorDate: Sun Nov 9 11:15:56 2025 +0800
refactor: Add `assert_or_internal_err!` macro for more ergonomic internal
invariant checks (#18511)
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases.
You can link an issue to this PR using the GitHub syntax. For example
`Closes #123` indicates that this PR will close issue #123.
-->
- Closes https://github.com/apache/datafusion/issues/15492
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly
in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand
your changes and offer better suggestions for fixes.
-->
See issue for the rationale and example.
This PR introduces the following macros to make invariant checks and
throwing internal errors easier, and also let the error message include
more assertion details if it failed (what's the expected/actual value),
to make debugging easier.
- `assert_or_internal_err!()`
- `assert_eq_or_internal_err!()`
- `assert_ne_or_internal_err!()`
```rust
// before
if field.name() != expected.name() {
return internal_err!(
"Field name mismatch at index {}: expected '{}', found '{}'",
idx,
expected.name(),
field.name()
);
}
// after
assert_eq_or_internal_err!(
field.name(),
expected.name(),
"Field name mismatch at index {}",
idx
);
```
If the assertion fails, the error now reads:
```
Internal error: Assertion failed: field.name() == expected.name() (left:
"foo", right: "bar"): Field name mismatch at index 3.
```
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it
is sometimes worth providing a summary of the individual changes in this
PR.
-->
1. Add macros and UTs to test
2. Updated a few internal error patterns that are applicable for this
macro
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
3. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example,
are they covered by existing tests)?
-->
UTs
## Are there any user-facing changes?
No
<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api
change` label.
-->
---------
Co-authored-by: Alex Huang <[email protected]>
---
datafusion/common/src/error.rs | 219 ++++++++++++++++++++++++++++++++
datafusion/core/src/physical_planner.rs | 35 ++---
2 files changed, 237 insertions(+), 17 deletions(-)
diff --git a/datafusion/common/src/error.rs b/datafusion/common/src/error.rs
index fde52944d0..4fa6d28e73 100644
--- a/datafusion/common/src/error.rs
+++ b/datafusion/common/src/error.rs
@@ -758,6 +758,116 @@ macro_rules! unwrap_or_internal_err {
};
}
+/// Assert a condition, returning `DataFusionError::Internal` on failure.
+///
+/// # Examples
+///
+/// ```text
+/// assert_or_internal_err!(predicate);
+/// assert_or_internal_err!(predicate, "human readable message");
+/// assert_or_internal_err!(predicate, format!("details: {}", value));
+/// ```
+#[macro_export]
+macro_rules! assert_or_internal_err {
+ ($cond:expr) => {
+ if !$cond {
+ return Err(DataFusionError::Internal(format!(
+ "Assertion failed: {}",
+ stringify!($cond)
+ )));
+ }
+ };
+ ($cond:expr, $($arg:tt)+) => {
+ if !$cond {
+ return Err(DataFusionError::Internal(format!(
+ "Assertion failed: {}: {}",
+ stringify!($cond),
+ format!($($arg)+)
+ )));
+ }
+ };
+}
+
+/// Assert equality, returning `DataFusionError::Internal` on failure.
+///
+/// # Examples
+///
+/// ```text
+/// assert_eq_or_internal_err!(actual, expected);
+/// assert_eq_or_internal_err!(left_expr, right_expr, "values must match");
+/// assert_eq_or_internal_err!(lhs, rhs, "metadata: {}", extra);
+/// ```
+#[macro_export]
+macro_rules! assert_eq_or_internal_err {
+ ($left:expr, $right:expr $(,)?) => {{
+ let left_val = &$left;
+ let right_val = &$right;
+ if left_val != right_val {
+ return Err(DataFusionError::Internal(format!(
+ "Assertion failed: {} == {} (left: {:?}, right: {:?})",
+ stringify!($left),
+ stringify!($right),
+ left_val,
+ right_val
+ )));
+ }
+ }};
+ ($left:expr, $right:expr, $($arg:tt)+) => {{
+ let left_val = &$left;
+ let right_val = &$right;
+ if left_val != right_val {
+ return Err(DataFusionError::Internal(format!(
+ "Assertion failed: {} == {} (left: {:?}, right: {:?}): {}",
+ stringify!($left),
+ stringify!($right),
+ left_val,
+ right_val,
+ format!($($arg)+)
+ )));
+ }
+ }};
+}
+
+/// Assert inequality, returning `DataFusionError::Internal` on failure.
+///
+/// # Examples
+///
+/// ```text
+/// assert_ne_or_internal_err!(left, right);
+/// assert_ne_or_internal_err!(lhs_expr, rhs_expr, "values must differ");
+/// assert_ne_or_internal_err!(a, b, "context {}", info);
+/// ```
+#[macro_export]
+macro_rules! assert_ne_or_internal_err {
+ ($left:expr, $right:expr $(,)?) => {{
+ let left_val = &$left;
+ let right_val = &$right;
+ if left_val == right_val {
+ return Err(DataFusionError::Internal(format!(
+ "Assertion failed: {} != {} (left: {:?}, right: {:?})",
+ stringify!($left),
+ stringify!($right),
+ left_val,
+ right_val
+ )));
+ }
+ }};
+ ($left:expr, $right:expr, $($arg:tt)+) => {{
+ let left_val = &$left;
+ let right_val = &$right;
+ if left_val == right_val {
+ return Err(DataFusionError::Internal(format!(
+ "Assertion failed: {} != {} (left: {:?}, right: {:?}): {}",
+ stringify!($left),
+ stringify!($right),
+ left_val,
+ right_val,
+ format!($($arg)+)
+ )));
+ }
+ }};
+}
+
/// Add a macros for concise DataFusionError::* errors declaration
/// supports placeholders the same way as `format!`
/// Examples:
@@ -974,6 +1084,115 @@ mod test {
use std::sync::Arc;
use arrow::error::ArrowError;
+ use insta::assert_snapshot;
+
+ fn ok_result() -> Result<()> {
+ Ok(())
+ }
+
+ #[test]
+ fn test_assert_eq_or_internal_err_passes() -> Result<()> {
+ assert_eq_or_internal_err!(1, 1);
+ ok_result()
+ }
+
+ #[test]
+ fn test_assert_eq_or_internal_err_fails() {
+ fn check() -> Result<()> {
+ assert_eq_or_internal_err!(1, 2, "expected equality");
+ ok_result()
+ }
+
+ let err = check().unwrap_err();
+ assert_snapshot!(
+ err.to_string(),
+ @r"
+ Internal error: Assertion failed: 1 == 2 (left: 1, right: 2): expected
equality.
+ This issue was likely caused by a bug in DataFusion's code. Please
help us to resolve this by filing a bug report in our issue tracker:
https://github.com/apache/datafusion/issues
+ "
+ );
+ }
+
+ #[test]
+ fn test_assert_ne_or_internal_err_passes() -> Result<()> {
+ assert_ne_or_internal_err!(1, 2);
+ ok_result()
+ }
+
+ #[test]
+ fn test_assert_ne_or_internal_err_fails() {
+ fn check() -> Result<()> {
+ assert_ne_or_internal_err!(3, 3, "values must differ");
+ ok_result()
+ }
+
+ let err = check().unwrap_err();
+ assert_snapshot!(
+ err.to_string(),
+ @r"
+ Internal error: Assertion failed: 3 != 3 (left: 3, right: 3): values
must differ.
+ This issue was likely caused by a bug in DataFusion's code. Please
help us to resolve this by filing a bug report in our issue tracker:
https://github.com/apache/datafusion/issues
+ "
+ );
+ }
+
+ #[test]
+ fn test_assert_or_internal_err_passes() -> Result<()> {
+ assert_or_internal_err!(true);
+ assert_or_internal_err!(true, "message");
+ ok_result()
+ }
+
+ #[test]
+ fn test_assert_or_internal_err_fails_default() {
+ fn check() -> Result<()> {
+ assert_or_internal_err!(false);
+ ok_result()
+ }
+
+ let err = check().unwrap_err();
+ assert_snapshot!(
+ err.to_string(),
+ @r"
+ Internal error: Assertion failed: false.
+ This issue was likely caused by a bug in DataFusion's code. Please
help us to resolve this by filing a bug report in our issue tracker:
https://github.com/apache/datafusion/issues
+ "
+ );
+ }
+
+ #[test]
+ fn test_assert_or_internal_err_fails_with_message() {
+ fn check() -> Result<()> {
+ assert_or_internal_err!(false, "custom message");
+ ok_result()
+ }
+
+ let err = check().unwrap_err();
+ assert_snapshot!(
+ err.to_string(),
+ @r"
+ Internal error: Assertion failed: false: custom message.
+ This issue was likely caused by a bug in DataFusion's code. Please
help us to resolve this by filing a bug report in our issue tracker:
https://github.com/apache/datafusion/issues
+ "
+ );
+ }
+
+ #[test]
+ fn test_assert_or_internal_err_with_format_arguments() {
+ fn check() -> Result<()> {
+ assert_or_internal_err!(false, "custom {}", 42);
+ ok_result()
+ }
+
+ let err = check().unwrap_err();
+ assert_snapshot!(
+ err.to_string(),
+ @r"
+ Internal error: Assertion failed: false: custom 42.
+ This issue was likely caused by a bug in DataFusion's code. Please
help us to resolve this by filing a bug report in our issue tracker:
https://github.com/apache/datafusion/issues
+ "
+ );
+ }
#[test]
fn test_error_size() {
diff --git a/datafusion/core/src/physical_planner.rs
b/datafusion/core/src/physical_planner.rs
index c280b50a9f..6a75485c62 100644
--- a/datafusion/core/src/physical_planner.rs
+++ b/datafusion/core/src/physical_planner.rs
@@ -64,7 +64,9 @@ use datafusion_catalog::ScanArgs;
use datafusion_common::display::ToStringifiedPlan;
use datafusion_common::format::ExplainAnalyzeLevel;
use datafusion_common::tree_node::{TreeNode, TreeNodeRecursion,
TreeNodeVisitor};
-use datafusion_common::TableReference;
+use datafusion_common::{
+ assert_eq_or_internal_err, assert_or_internal_err, TableReference,
+};
use datafusion_common::{
exec_err, internal_datafusion_err, internal_err, not_impl_err, plan_err,
DFSchema,
ScalarValue,
@@ -347,11 +349,11 @@ impl DefaultPhysicalPlanner {
.flatten()
.collect::<Vec<_>>();
// Ideally this never happens if we have a valid LogicalPlan tree
- if outputs.len() != 1 {
- return internal_err!(
- "Failed to convert LogicalPlan to ExecutionPlan: More than one
root detected"
- );
- }
+ assert_eq_or_internal_err!(
+ outputs.len(),
+ 1,
+ "Failed to convert LogicalPlan to ExecutionPlan: More than one
root detected"
+ );
let plan = outputs.pop().unwrap();
Ok(plan)
}
@@ -588,9 +590,10 @@ impl DefaultPhysicalPlanner {
}
}
LogicalPlan::Window(Window { window_expr, .. }) => {
- if window_expr.is_empty() {
- return internal_err!("Impossibly got empty window
expression");
- }
+ assert_or_internal_err!(
+ !window_expr.is_empty(),
+ "Impossibly got empty window expression"
+ );
let input_exec = children.one()?;
@@ -1764,14 +1767,12 @@ fn qualify_join_schema_sides(
.zip(left_fields.iter().chain(right_fields.iter()))
.enumerate()
{
- if field.name() != expected.name() {
- return internal_err!(
- "Field name mismatch at index {}: expected '{}', found '{}'",
- i,
- expected.name(),
- field.name()
- );
- }
+ assert_eq_or_internal_err!(
+ field.name(),
+ expected.name(),
+ "Field name mismatch at index {}",
+ i
+ );
}
// qualify sides
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
