Skip to content

Commit c728d54

Browse files
2010YOUY01Weijun-H
2010YOUY01
and
Weijun-H
authored
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 #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]>
1 parent c1965b6 commit c728d54

File tree

2 files changed

+237
-17
lines changed

2 files changed

+237
-17
lines changed

‎datafusion/common/src/error.rs‎

Lines changed: 219 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -758,6 +758,116 @@ macro_rules! unwrap_or_internal_err {
758758
};
759759
}
760760

761+
/// Assert a condition, returning `DataFusionError::Internal` on failure.
762+
///
763+
/// # Examples
764+
///
765+
/// ```text
766+
/// assert_or_internal_err!(predicate);
767+
/// assert_or_internal_err!(predicate, "human readable message");
768+
/// assert_or_internal_err!(predicate, format!("details: {}", value));
769+
/// ```
770+
#[macro_export]
771+
macro_rules! assert_or_internal_err {
772+
($cond:expr) => {
773+
if !$cond {
774+
return Err(DataFusionError::Internal(format!(
775+
"Assertion failed: {}",
776+
stringify!($cond)
777+
)));
778+
}
779+
};
780+
($cond:expr, $($arg:tt)+) => {
781+
if !$cond {
782+
return Err(DataFusionError::Internal(format!(
783+
"Assertion failed: {}: {}",
784+
stringify!($cond),
785+
format!($($arg)+)
786+
)));
787+
}
788+
};
789+
}
790+
791+
/// Assert equality, returning `DataFusionError::Internal` on failure.
792+
///
793+
/// # Examples
794+
///
795+
/// ```text
796+
/// assert_eq_or_internal_err!(actual, expected);
797+
/// assert_eq_or_internal_err!(left_expr, right_expr, "values must match");
798+
/// assert_eq_or_internal_err!(lhs, rhs, "metadata: {}", extra);
799+
/// ```
800+
#[macro_export]
801+
macro_rules! assert_eq_or_internal_err {
802+
($left:expr, $right:expr $(,)?) => {{
803+
let left_val = &$left;
804+
let right_val = &$right;
805+
if left_val != right_val {
806+
return Err(DataFusionError::Internal(format!(
807+
"Assertion failed: {} == {} (left: {:?}, right: {:?})",
808+
stringify!($left),
809+
stringify!($right),
810+
left_val,
811+
right_val
812+
)));
813+
}
814+
}};
815+
($left:expr, $right:expr, $($arg:tt)+) => {{
816+
let left_val = &$left;
817+
let right_val = &$right;
818+
if left_val != right_val {
819+
return Err(DataFusionError::Internal(format!(
820+
"Assertion failed: {} == {} (left: {:?}, right: {:?}): {}",
821+
stringify!($left),
822+
stringify!($right),
823+
left_val,
824+
right_val,
825+
format!($($arg)+)
826+
)));
827+
}
828+
}};
829+
}
830+
831+
/// Assert inequality, returning `DataFusionError::Internal` on failure.
832+
///
833+
/// # Examples
834+
///
835+
/// ```text
836+
/// assert_ne_or_internal_err!(left, right);
837+
/// assert_ne_or_internal_err!(lhs_expr, rhs_expr, "values must differ");
838+
/// assert_ne_or_internal_err!(a, b, "context {}", info);
839+
/// ```
840+
#[macro_export]
841+
macro_rules! assert_ne_or_internal_err {
842+
($left:expr, $right:expr $(,)?) => {{
843+
let left_val = &$left;
844+
let right_val = &$right;
845+
if left_val == right_val {
846+
return Err(DataFusionError::Internal(format!(
847+
"Assertion failed: {} != {} (left: {:?}, right: {:?})",
848+
stringify!($left),
849+
stringify!($right),
850+
left_val,
851+
right_val
852+
)));
853+
}
854+
}};
855+
($left:expr, $right:expr, $($arg:tt)+) => {{
856+
let left_val = &$left;
857+
let right_val = &$right;
858+
if left_val == right_val {
859+
return Err(DataFusionError::Internal(format!(
860+
"Assertion failed: {} != {} (left: {:?}, right: {:?}): {}",
861+
stringify!($left),
862+
stringify!($right),
863+
left_val,
864+
right_val,
865+
format!($($arg)+)
866+
)));
867+
}
868+
}};
869+
}
870+
761871
/// Add a macros for concise DataFusionError::* errors declaration
762872
/// supports placeholders the same way as `format!`
763873
/// Examples:
@@ -974,6 +1084,115 @@ mod test {
9741084
use std::sync::Arc;
9751085

9761086
use arrow::error::ArrowError;
1087+
use insta::assert_snapshot;
1088+
1089+
fn ok_result() -> Result<()> {
1090+
Ok(())
1091+
}
1092+
1093+
#[test]
1094+
fn test_assert_eq_or_internal_err_passes() -> Result<()> {
1095+
assert_eq_or_internal_err!(1, 1);
1096+
ok_result()
1097+
}
1098+
1099+
#[test]
1100+
fn test_assert_eq_or_internal_err_fails() {
1101+
fn check() -> Result<()> {
1102+
assert_eq_or_internal_err!(1, 2, "expected equality");
1103+
ok_result()
1104+
}
1105+
1106+
let err = check().unwrap_err();
1107+
assert_snapshot!(
1108+
err.to_string(),
1109+
@r"
1110+
Internal error: Assertion failed: 1 == 2 (left: 1, right: 2): expected equality.
1111+
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
1112+
"
1113+
);
1114+
}
1115+
1116+
#[test]
1117+
fn test_assert_ne_or_internal_err_passes() -> Result<()> {
1118+
assert_ne_or_internal_err!(1, 2);
1119+
ok_result()
1120+
}
1121+
1122+
#[test]
1123+
fn test_assert_ne_or_internal_err_fails() {
1124+
fn check() -> Result<()> {
1125+
assert_ne_or_internal_err!(3, 3, "values must differ");
1126+
ok_result()
1127+
}
1128+
1129+
let err = check().unwrap_err();
1130+
assert_snapshot!(
1131+
err.to_string(),
1132+
@r"
1133+
Internal error: Assertion failed: 3 != 3 (left: 3, right: 3): values must differ.
1134+
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
1135+
"
1136+
);
1137+
}
1138+
1139+
#[test]
1140+
fn test_assert_or_internal_err_passes() -> Result<()> {
1141+
assert_or_internal_err!(true);
1142+
assert_or_internal_err!(true, "message");
1143+
ok_result()
1144+
}
1145+
1146+
#[test]
1147+
fn test_assert_or_internal_err_fails_default() {
1148+
fn check() -> Result<()> {
1149+
assert_or_internal_err!(false);
1150+
ok_result()
1151+
}
1152+
1153+
let err = check().unwrap_err();
1154+
assert_snapshot!(
1155+
err.to_string(),
1156+
@r"
1157+
Internal error: Assertion failed: false.
1158+
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
1159+
"
1160+
);
1161+
}
1162+
1163+
#[test]
1164+
fn test_assert_or_internal_err_fails_with_message() {
1165+
fn check() -> Result<()> {
1166+
assert_or_internal_err!(false, "custom message");
1167+
ok_result()
1168+
}
1169+
1170+
let err = check().unwrap_err();
1171+
assert_snapshot!(
1172+
err.to_string(),
1173+
@r"
1174+
Internal error: Assertion failed: false: custom message.
1175+
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
1176+
"
1177+
);
1178+
}
1179+
1180+
#[test]
1181+
fn test_assert_or_internal_err_with_format_arguments() {
1182+
fn check() -> Result<()> {
1183+
assert_or_internal_err!(false, "custom {}", 42);
1184+
ok_result()
1185+
}
1186+
1187+
let err = check().unwrap_err();
1188+
assert_snapshot!(
1189+
err.to_string(),
1190+
@r"
1191+
Internal error: Assertion failed: false: custom 42.
1192+
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
1193+
"
1194+
);
1195+
}
9771196

9781197
#[test]
9791198
fn test_error_size() {

‎datafusion/core/src/physical_planner.rs‎

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,9 @@ use datafusion_catalog::ScanArgs;
6464
use datafusion_common::display::ToStringifiedPlan;
6565
use datafusion_common::format::ExplainAnalyzeLevel;
6666
use datafusion_common::tree_node::{TreeNode, TreeNodeRecursion, TreeNodeVisitor};
67-
use datafusion_common::TableReference;
67+
use datafusion_common::{
68+
assert_eq_or_internal_err, assert_or_internal_err, TableReference,
69+
};
6870
use datafusion_common::{
6971
exec_err, internal_datafusion_err, internal_err, not_impl_err, plan_err, DFSchema,
7072
ScalarValue,
@@ -347,11 +349,11 @@ impl DefaultPhysicalPlanner {
347349
.flatten()
348350
.collect::<Vec<_>>();
349351
// Ideally this never happens if we have a valid LogicalPlan tree
350-
if outputs.len() != 1 {
351-
return internal_err!(
352-
"Failed to convert LogicalPlan to ExecutionPlan: More than one root detected"
353-
);
354-
}
352+
assert_eq_or_internal_err!(
353+
outputs.len(),
354+
1,
355+
"Failed to convert LogicalPlan to ExecutionPlan: More than one root detected"
356+
);
355357
let plan = outputs.pop().unwrap();
356358
Ok(plan)
357359
}
@@ -588,9 +590,10 @@ impl DefaultPhysicalPlanner {
588590
}
589591
}
590592
LogicalPlan::Window(Window { window_expr, .. }) => {
591-
if window_expr.is_empty() {
592-
return internal_err!("Impossibly got empty window expression");
593-
}
593+
assert_or_internal_err!(
594+
!window_expr.is_empty(),
595+
"Impossibly got empty window expression"
596+
);
594597

595598
let input_exec = children.one()?;
596599

@@ -1764,14 +1767,12 @@ fn qualify_join_schema_sides(
17641767
.zip(left_fields.iter().chain(right_fields.iter()))
17651768
.enumerate()
17661769
{
1767-
if field.name() != expected.name() {
1768-
return internal_err!(
1769-
"Field name mismatch at index {}: expected '{}', found '{}'",
1770-
i,
1771-
expected.name(),
1772-
field.name()
1773-
);
1774-
}
1770+
assert_eq_or_internal_err!(
1771+
field.name(),
1772+
expected.name(),
1773+
"Field name mismatch at index {}",
1774+
i
1775+
);
17751776
}
17761777

17771778
// qualify sides

0 commit comments

Comments
 (0)