You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "eliaperantoni (via GitHub)" <gi...@apache.org> on 2025/02/03 12:08:31 UTC
[I] Emit warning with attached `Diagnostic` when doing `= NULL` [datafusion]
eliaperantoni opened a new issue, #14434:
URL: https://github.com/apache/datafusion/issues/14434
### Is your feature request related to a problem or challenge?
For a query like:
```sql
SELECT * FROM users WHERE password = NULL
```
DataFusion does not emit any error. Of course this a valid query, but it's very likely that the user intended to do `password IS NULL` instead.
We want to provide a rich message that _references and highlights locations in the original SQL query_, and contextualises and helps the user understand the pitfall. In the end, it would be possible to display a warning in a fashion akin to what was enabled by #13664 for some (fatal) errors:
See #14429 for more information.
### Describe the solution you'd like
Attach a well crafted `Diagnostic` to the `DataFusionError`, building on top of the foundations laid in #13664. See #14429 for more information.
There would probably need to be some plumbing required to make DataFusion collect warnings without halting the overall execution. In this scenario, we want to use `Diagnostic::new_warning`.
### Describe alternatives you've considered
_No response_
### Additional context
_No response_
--
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] Emit warning with attached `Diagnostic` when doing `= NULL` [datafusion]
Posted by "natalievolk (via GitHub)" <gi...@apache.org>.
natalievolk commented on issue #14434:
URL: https://github.com/apache/datafusion/issues/14434#issuecomment-3948700525
Hey, could I take this?
--
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] Emit warning with attached `Diagnostic` when doing `= NULL` [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew closed issue #14434: Emit warning with attached `Diagnostic` when doing `= NULL`
URL: https://github.com/apache/datafusion/issues/14434
--
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] Emit warning with attached `Diagnostic` when doing `= NULL` [datafusion]
Posted by "changsun20 (via GitHub)" <gi...@apache.org>.
changsun20 commented on issue #14434:
URL: https://github.com/apache/datafusion/issues/14434#issuecomment-2731465313
Hi @eliaperantoni,
Thank you for the detailed guidance! Here's my understanding of the next steps:
1. **Warning Scope**
I'll implement the semantic approach to only raise warnings when `= NULL` is used as a condition, which makes the most sense functionally.
2. **Warning Plumbing**
Both approaches you outlined are helpful. I'll start with the interior mutability approach using `warnings: RefCell<Vec<Diagnostic>>` in `SqlToRel` as it seems to be the least disruptive path forward. The `DatafusionResult` type with `WarningAccumulator` pattern offers an interesting, more robust solution that we could consider for the longer term.
3. **Span Highlighting**
I've checked PR [#1738](https://github.com/apache/datafusion-sqlparser-rs/pull/1738) and found that the feature is available in `sqlparser` version 0.55.0. However, the main codebase can't be upgraded to this version directly at the moment. There's a WIP PR(#15183) for this dependency upgrade, so I believe this won't be a problem soon.
Thanks again for your guidance. I'll start working on an implementation based on these suggestions. Appreciate your patience as I navigate the codebase!
--
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] Emit warning with attached `Diagnostic` when doing `= NULL` [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on issue #14434:
URL: https://github.com/apache/datafusion/issues/14434#issuecomment-2728578964
Thank you @changsun20 for the detailed analysis 🙏
#### 1. Warning Scope for = NULL Patterns
I would say that, if's not _too_ complicated, we should go for the semantic approach. i.e. only raise a warning when `= NULL` is being used as a condition.
#### 2. Non-Fatal Warning Plumbing
This is a complicated one! 😃 You hit the nail on the head: the complicated part is bubbling up a warning without interrupting everything. I haven't thought too much about how to solve it.
Perhaps one way would be to make a `type DatafusionResult<R> = Result<(R, Vec<DatafusionWarning>), DatafusionError>` so that a fatal error can still be bubbled up with `?`, and then a `WarningAccumulator` could help with the aggregation of warnings that arise in a function body. i.e.
```rust
fn parse_query(sql: &str) -> DatafusionResult<LogicalPlan> {
let mut warnings = WarningAccumulator::new();
// WarningAccumulator::extend(&mut self) takes a DatafusionResult<R> and
// returns Result<R>, while storing any warnings in itself.
let ctes = warnings.extend(parse_ctes(sql))?;
let mut logical_plan = warnings.extend(parse_select(sql, ctes))?;
warnings.extend(parse_where(&mut logical_plan, sql))?;
// WarningAccumulator::into returns Vec<DatafusionWarning>
DatafusionResult::ok_with_warnings(logical_plan, warnings.into())
}
```
But I believe that's a bit invasive because it requires replacing `Result` with `DatafusionResult` all the way up to the outermost function call. It also pollutes the code with a lot of `warnings.extend` calls. Though perhaps this would be the most robust solution in the long term.
Alternatively, to start with we could add a field to `SqlToRel` where warnings are collected (i.e. just a `warnings: Vec<Diagnostic>` would work imo, no need for a new type), and then the user can get them out if they so wish. The only problem I see with this is that most (all?) methods of `SqlToRel` take an immutable reference. So you'd either have to use interior mutability, or change most receivers to `&mut self`. The latter is definitely a breaking change so I'd not encourage going that route. I think interior mutability with `RefCell` is fine.
#### 3. Span Highlighting Strategy
> Add Span to op: Eq and/or right: Value(Null) - most precise but risks breaking changes elsewhere
This means changing the `sqlparser` dependency, that's the only component that can give you spans. Actually, someone already added spans to `Value` (https://github.com/apache/datafusion-sqlparser-rs/pull/1738). Perhaps you could bump Datafusion's depedency on `sqlparser` to get that?
If that doesn't work, I would suggest perhaps going the easy route of highlighting the expression and saying "this expression is being compared with `NULL` and that's always falsey, perhaps you meant `IS NULL`" or something like that. It's not the best but it's the most we can do without more information from `sqlparser` :) I think trying to find the `IS NULL` by means of character operations is not worth the complexity and risks missing some edges cases.
--
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] Emit warning with attached `Diagnostic` when doing `= NULL` [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on issue #14434:
URL: https://github.com/apache/datafusion/issues/14434#issuecomment-2757036518
> Quick update: I was a bit busy with schoolwork last week, but I’ll try to fix this ticket this week. Thanks for your patience!
No worries at all! <3
--
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] Emit warning with attached `Diagnostic` when doing `= NULL` [datafusion]
Posted by "changsun20 (via GitHub)" <gi...@apache.org>.
changsun20 commented on issue #14434:
URL: https://github.com/apache/datafusion/issues/14434#issuecomment-2718702755
Hi @ugoa, I noticed that this ticket was assigned to you a couple weeks ago. May I ask if any progress has been made? If you are willing to cooperate, I would be happy to submit a PR to help resolve this issue. Please let me know if this works for you. Appreciate your efforts!
--
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] Emit warning with attached `Diagnostic` when doing `= NULL` [datafusion]
Posted by "changsun20 (via GitHub)" <gi...@apache.org>.
changsun20 commented on issue #14434:
URL: https://github.com/apache/datafusion/issues/14434#issuecomment-2726143005
Hi @eliaperantoni,
Thank you raising this issue. As I explore potential implementations, I’d appreciate your insights on a few key points:
**1. Warning Scope for `= NULL` Patterns**
Could you help confirm the appropriate scope for detecting `= NULL` comparisons?
- **Option 1**: Emit warnings universally whenever `= NULL` is parsed.
- _Potential Risk_: Trigger warnings whenever "= NULL" is parsed. However, expressions like `UPDATE users SET password = NULL` would be legitimate usage.
- Note: Since `Dml(Update)` isn't currently supported in DataFusion, this might not be an immediate issue but could become one.
- **Option 2**: Restrict warnings to predicate contexts (WHERE/JOIN/HAVING/CASE WHEN clauses).
- _Tradeoff_: This would be more precise but adds complexity.
**2. Non-Fatal Warning Plumbing**
You mentioned the need to implement plumbing and collect warnings via `Diagnostic::new_warning`. Could you please elaborate more on this? Can you briefly outline (or just give me a hint) how to propagate warnings without interrupting the execution flow? I recognize this may require deeper code context and may to too general to answer—feel free to defer discussion until I share a draft implementation!
**3. Span Highlighting Strategy**
For the query `SELECT * FROM users WHERE password = NULL`, the parsed structure shows:
```json
BinaryOp {
left: Identifier(
Ident {
value: "password",
quote_style: None,
span: Span(Location(1,27)..Location(1,35)),
},
),
op: Eq,
right: Value(
Null,
),
},
```
I have come up with some possible approaches:
- **Option 1**: Add `Span` to `op: Eq` and/or `right: Value(Null)` - most precise but risks breaking changes elsewhere
- **Option 2**: Use the `password` identifier’s span - simplest approach, but highlights the column name rather than the actual issue ("= NULL").
- **Option 3**: Use the identifier's `Span` as a starting point and estimate the entire expression's span - avoids structural changes but may be inaccurate with variable whitespace
What would be the best approach in your opinion?
Thank you for your guidance!
--
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] Emit warning with attached `Diagnostic` when doing `= NULL` [datafusion]
Posted by "ugoa (via GitHub)" <gi...@apache.org>.
ugoa commented on issue #14434:
URL: https://github.com/apache/datafusion/issues/14434#issuecomment-2660809032
Yeah I am working on it this weekend
--
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] Emit warning with attached `Diagnostic` when doing `= NULL` [datafusion]
Posted by "changsun20 (via GitHub)" <gi...@apache.org>.
changsun20 commented on issue #14434:
URL: https://github.com/apache/datafusion/issues/14434#issuecomment-2755932600
Quick update: I was a bit busy with schoolwork last week, but I’ll try to fix this ticket this week. Thanks for your patience!
--
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] Emit warning with attached `Diagnostic` when doing `= NULL` [datafusion]
Posted by "ugoa (via GitHub)" <gi...@apache.org>.
ugoa commented on issue #14434:
URL: https://github.com/apache/datafusion/issues/14434#issuecomment-2646349335
take
--
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] Emit warning with attached `Diagnostic` when doing `= NULL` [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on issue #14434:
URL: https://github.com/apache/datafusion/issues/14434#issuecomment-4708693257
I agree that this behavior can be confusing for users who are very new to SQL, but I don't know of a single other database system that emits a warning in this situation. I wonder if there are any circumstances where emitting a warning would be undesirable (code gen / ORMs maybe?).
--
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] Emit warning with attached `Diagnostic` when doing `= NULL` [datafusion]
Posted by "eliaperantoni (via GitHub)" <gi...@apache.org>.
eliaperantoni commented on issue #14434:
URL: https://github.com/apache/datafusion/issues/14434#issuecomment-2656882060
Hey @ugoa how is it going with this ticket :) Can I help with anything?
--
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] Emit warning with attached `Diagnostic` when doing `= NULL` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #14434:
URL: https://github.com/apache/datafusion/issues/14434#issuecomment-2634527624
I think this is a good first issue as the need is clear and the tests in https://github.com/apache/datafusion/blob/85fbde2661bdb462fc498dc18f055c44f229604c/datafusion/sql/tests/cases/diagnostic.rs are well structured for extension.
--
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