Skip to content

Commit fb765c2

Browse files
authored
minor: refactor with assert_or_internal_err!() in datafusion/sql (#18614)
## 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. --> Part of #18613 ## 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 ## 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. --> ## 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 2. 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)? --> ## Are there any user-facing changes? <!-- 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. -->
1 parent cb1f019 commit fb765c2

File tree

6 files changed

+53
-52
lines changed

6 files changed

+53
-52
lines changed

datafusion/sql/src/expr/identifier.rs

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,9 @@
1717

1818
use arrow::datatypes::Field;
1919
use datafusion_common::{
20-
exec_datafusion_err, internal_err, not_impl_err, plan_datafusion_err, plan_err,
21-
Column, DFSchema, Result, Span, TableReference,
20+
assert_or_internal_err, exec_datafusion_err, internal_err, not_impl_err,
21+
plan_datafusion_err, plan_err, Column, DFSchema, DataFusionError, Result, Span,
22+
TableReference,
2223
};
2324
use datafusion_expr::planner::PlannerResult;
2425
use datafusion_expr::{Case, Expr};
@@ -99,9 +100,7 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
99100
schema: &DFSchema,
100101
planner_context: &mut PlannerContext,
101102
) -> Result<Expr> {
102-
if ids.len() < 2 {
103-
return internal_err!("Not a compound identifier: {ids:?}");
104-
}
103+
assert_or_internal_err!(ids.len() >= 2, "Not a compound identifier: {ids:?}");
105104

106105
let ids_span = Span::union_iter(
107106
ids.iter()

datafusion/sql/src/unparser/expr.rs

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,8 @@ use arrow::datatypes::{
4040
};
4141
use arrow::util::display::array_value_to_string;
4242
use datafusion_common::{
43-
internal_datafusion_err, internal_err, not_impl_err, plan_err, Column, Result,
44-
ScalarValue,
43+
assert_eq_or_internal_err, assert_or_internal_err, internal_datafusion_err,
44+
internal_err, not_impl_err, plan_err, Column, DataFusionError, Result, ScalarValue,
4545
};
4646
use datafusion_expr::{
4747
expr::{Alias, Exists, InList, ScalarFunction, Sort, WindowFunction},
@@ -447,9 +447,7 @@ impl Unparser<'_> {
447447
})
448448
}
449449
Expr::ScalarVariable(_, ids) => {
450-
if ids.is_empty() {
451-
return internal_err!("Not a valid ScalarVariable");
452-
}
450+
assert_or_internal_err!(!ids.is_empty(), "Not a valid ScalarVariable");
453451

454452
Ok(if ids.len() == 1 {
455453
ast::Expr::Identifier(
@@ -593,9 +591,11 @@ impl Unparser<'_> {
593591
}
594592

595593
fn array_element_to_sql(&self, args: &[Expr]) -> Result<ast::Expr> {
596-
if args.len() != 2 {
597-
return internal_err!("array_element must have exactly 2 arguments");
598-
}
594+
assert_eq_or_internal_err!(
595+
args.len(),
596+
2,
597+
"array_element must have exactly 2 arguments"
598+
);
599599
let array = self.expr_to_sql(&args[0])?;
600600
let index = self.expr_to_sql(&args[1])?;
601601
Ok(ast::Expr::CompoundFieldAccess {
@@ -605,9 +605,10 @@ impl Unparser<'_> {
605605
}
606606

607607
fn named_struct_to_sql(&self, args: &[Expr]) -> Result<ast::Expr> {
608-
if !args.len().is_multiple_of(2) {
609-
return internal_err!("named_struct must have an even number of arguments");
610-
}
608+
assert_or_internal_err!(
609+
args.len().is_multiple_of(2),
610+
"named_struct must have an even number of arguments"
611+
);
611612

612613
let args = args
613614
.chunks_exact(2)
@@ -628,9 +629,11 @@ impl Unparser<'_> {
628629
}
629630

630631
fn get_field_to_sql(&self, args: &[Expr]) -> Result<ast::Expr> {
631-
if args.len() != 2 {
632-
return internal_err!("get_field must have exactly 2 arguments");
633-
}
632+
assert_eq_or_internal_err!(
633+
args.len(),
634+
2,
635+
"get_field must have exactly 2 arguments"
636+
);
634637

635638
let field = match &args[1] {
636639
Expr::Literal(lit, _) => self.new_ident_quoted_if_needs(lit.to_string()),
@@ -672,9 +675,7 @@ impl Unparser<'_> {
672675
}
673676

674677
fn map_to_sql(&self, args: &[Expr]) -> Result<ast::Expr> {
675-
if args.len() != 2 {
676-
return internal_err!("map must have exactly 2 arguments");
677-
}
678+
assert_eq_or_internal_err!(args.len(), 2, "map must have exactly 2 arguments");
678679

679680
let ast::Expr::Array(Array { elem: keys, .. }) = self.expr_to_sql(&args[0])?
680681
else {

datafusion/sql/src/unparser/plan.rs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ use crate::unparser::utils::{find_unnest_node_until_relation, unproject_agg_expr
3939
use crate::unparser::{ast::UnnestRelationBuilder, rewrite::rewrite_qualify};
4040
use crate::utils::UNNEST_PLACEHOLDER;
4141
use datafusion_common::{
42-
internal_err, not_impl_err,
42+
assert_or_internal_err, internal_err, not_impl_err,
4343
tree_node::{TransformedResult, TreeNode},
4444
Column, DataFusionError, Result, ScalarValue, TableReference,
4545
};
@@ -887,9 +887,10 @@ impl Unparser<'_> {
887887
.map(|input| self.select_to_sql_expr(input, query))
888888
.collect::<Result<Vec<_>>>()?;
889889

890-
if input_exprs.len() < 2 {
891-
return internal_err!("UNION operator requires at least 2 inputs");
892-
}
890+
assert_or_internal_err!(
891+
input_exprs.len() >= 2,
892+
"UNION operator requires at least 2 inputs"
893+
);
893894

894895
let set_quantifier =
895896
if query.as_ref().is_some_and(|q| q.is_distinct_union()) {
@@ -957,12 +958,11 @@ impl Unparser<'_> {
957958
}
958959
}
959960
LogicalPlan::Unnest(unnest) => {
960-
if !unnest.struct_type_columns.is_empty() {
961-
return internal_err!(
962-
"Struct type columns are not currently supported in UNNEST: {:?}",
963-
unnest.struct_type_columns
964-
);
965-
}
961+
assert_or_internal_err!(
962+
unnest.struct_type_columns.is_empty(),
963+
"Struct type columns are not currently supported in UNNEST: {:?}",
964+
unnest.struct_type_columns
965+
);
966966

967967
// In the case of UNNEST, the Unnest node is followed by a duplicate Projection node that we should skip.
968968
// Otherwise, there will be a duplicate SELECT clause.

datafusion/sql/src/unparser/utils.rs

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ use super::{
2222
rewrite::TableAliasRewriter, Unparser,
2323
};
2424
use datafusion_common::{
25-
internal_err,
25+
assert_eq_or_internal_err, internal_err,
2626
tree_node::{Transformed, TransformedResult, TreeNode},
2727
Column, DataFusionError, Result, ScalarValue,
2828
};
@@ -520,12 +520,12 @@ pub(crate) fn sqlite_from_unixtime_to_sql(
520520
unparser: &Unparser,
521521
from_unixtime_args: &[Expr],
522522
) -> Result<Option<ast::Expr>> {
523-
if from_unixtime_args.len() != 1 {
524-
return internal_err!(
525-
"from_unixtime for SQLite expects 1 argument, found {}",
526-
from_unixtime_args.len()
527-
);
528-
}
523+
assert_eq_or_internal_err!(
524+
from_unixtime_args.len(),
525+
1,
526+
"from_unixtime for SQLite expects 1 argument, found {}",
527+
from_unixtime_args.len()
528+
);
529529

530530
Ok(Some(unparser.scalar_function_to_sql(
531531
"datetime",
@@ -547,12 +547,12 @@ pub(crate) fn sqlite_date_trunc_to_sql(
547547
unparser: &Unparser,
548548
date_trunc_args: &[Expr],
549549
) -> Result<Option<ast::Expr>> {
550-
if date_trunc_args.len() != 2 {
551-
return internal_err!(
552-
"date_trunc for SQLite expects 2 arguments, found {}",
553-
date_trunc_args.len()
554-
);
555-
}
550+
assert_eq_or_internal_err!(
551+
date_trunc_args.len(),
552+
2,
553+
"date_trunc for SQLite expects 2 arguments, found {}",
554+
date_trunc_args.len()
555+
);
556556

557557
if let Expr::Literal(ScalarValue::Utf8(Some(unit)), _) = &date_trunc_args[0] {
558558
let format = match unit.to_lowercase().as_str() {

datafusion/sql/src/utils.rs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,8 @@ use datafusion_common::tree_node::{
2626
Transformed, TransformedResult, TreeNode, TreeNodeRecursion, TreeNodeRewriter,
2727
};
2828
use datafusion_common::{
29-
exec_datafusion_err, exec_err, internal_err, plan_err, Column, DFSchemaRef,
30-
Diagnostic, HashMap, Result, ScalarValue,
29+
assert_or_internal_err, exec_datafusion_err, exec_err, internal_err, plan_err,
30+
Column, DFSchemaRef, DataFusionError, Diagnostic, HashMap, Result, ScalarValue,
3131
};
3232
use datafusion_expr::builder::get_struct_unnested_columns;
3333
use datafusion_expr::expr::{
@@ -422,9 +422,10 @@ impl RecursiveUnnestRewriter<'_> {
422422

423423
match data_type {
424424
DataType::Struct(inner_fields) => {
425-
if !struct_allowed {
426-
return internal_err!("unnest on struct can only be applied at the root level of select expression");
427-
}
425+
assert_or_internal_err!(
426+
struct_allowed,
427+
"unnest on struct can only be applied at the root level of select expression"
428+
);
428429
push_projection_dedupl(
429430
self.inner_projection_exprs,
430431
expr_in_unnest.clone().alias(placeholder_name.clone()),

datafusion/sqllogictest/test_files/unnest.slt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -511,7 +511,7 @@ x y [30, 40, 50]
511511
query error DataFusion error: type_coercion\ncaused by\nThis feature is not implemented: Unnest should be rewritten to LogicalPlan::Unnest before type coercion
512512
select sum(unnest(generate_series(1,10)));
513513

514-
query error DataFusion error: Internal error: unnest on struct can only be applied at the root level of select expression
514+
query error DataFusion error: Internal error: Assertion failed: struct_allowed: unnest on struct can only be applied at the root level of select expression
515515
select arrow_typeof(unnest(column5)) from unnest_table;
516516

517517
query T
@@ -799,7 +799,7 @@ query error DataFusion error: Error during planning: Column in SELECT must be in
799799
select unnest(column1) c1 from nested_unnest_table group by c1.c0;
800800

801801
# TODO: this query should work. see issue: https://github.com/apache/datafusion/issues/12794
802-
query error DataFusion error: Internal error: unnest on struct can only be applied at the root level of select expression
802+
query error DataFusion error: Internal error: Assertion failed: struct_allowed: unnest on struct can only be applied at the root level of select expression
803803
select unnest(column1) c1 from nested_unnest_table
804804

805805
query II??I??

0 commit comments

Comments
 (0)