niebayes opened a new issue, #21431:
URL: https://github.com/apache/datafusion/issues/21431
## Summary
Planning an invalid SQL expression such as:
```sql
time 17542368000000000
```
causes `datafusion-sql` to panic instead of returning a planner error.
The panic happens in the `SQLExpr::TypedString` branch inside
`SqlToRel::sql_expr_to_logical_expr_internal`, where DataFusion assumes the
typed literal payload is always a string and calls:
```rust
value.into_string().unwrap()
```
For this input, the parser produces a typed-string-like AST path with a
non-string value, so `into_string()` returns `None` and the planner aborts with
`Option::unwrap()`.
## Impact
- Invalid user SQL can crash the planning path.
- Embedders of DataFusion may see process-level panics instead of a
recoverable SQL/planner error.
- This is especially confusing when the original SQL is just malformed and
should be rejected normally.
## Reproduction
``` rust
#[test]
fn test_sql_to_expr_panics_on_time_typed_string_without_string_literal()
{
let schema = DFSchema::empty();
let mut planner_context = PlannerContext::default();
let expr_str = "time 17542368000000000";
let dialect = GenericDialect {};
let mut parser =
Parser::new(&dialect).try_with_sql(expr_str).unwrap();
let sql_expr = parser.parse_expr().unwrap();
let context_provider = TestContextProvider::new();
let sql_to_rel = SqlToRel::new(&context_provider);
let panic = catch_unwind(AssertUnwindSafe(|| {
sql_to_rel
.sql_expr_to_logical_expr(sql_expr, &schema, &mut
planner_context)
.unwrap();
}))
.expect_err("planning invalid TIME typed string syntax should panic
today");
let panic_message = if let Some(message) =
panic.downcast_ref::<String>() {
message.as_str()
} else if let Some(message) = panic.downcast_ref::<&str>() {
message
} else {
""
};
assert!(
panic_message.contains("called `Option::unwrap()` on a `None`
value"),
"unexpected panic payload: {panic_message}"
);
}
```
## Expected behavior
DataFusion should return a normal `Result::Err(...)` describing invalid SQL
or an unsupported typed literal payload.
Examples of acceptable outcomes:
- parser error
- planner error
- `not_impl_err!` / `plan_err!`
Any of these would be preferable to a panic.
## Why this happens
The problematic code path is effectively:
```rust
SQLExpr::TypedString(TypedString {
data_type,
value,
uses_odbc_syntax: _,
}) => Ok(Expr::Cast(Cast::new(
Box::new(lit(value.into_string().unwrap())),
self.convert_data_type_to_field(&data_type)?
.data_type()
.clone(),
))),
```
This assumes `value` is always string-backed. For malformed input such as
`time 17542368000000000`, that assumption does not hold.
## Suggested fix direction
Replace the `unwrap()` with explicit validation and return a planner error
when the typed literal payload is not a string.
Pseudo-shape:
```rust
let value = value.into_string().ok_or_else(|| {
plan_datafusion_err!("TIME/typed string literal requires a string
payload")
})?;
```
The exact error text can be adjusted to match DataFusion conventions.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
