feat: add hint for missing fields #14521

Lordworms · 2025-02-06T04:34:30Z

Which issue does this PR close?

Closes Add hint to errors for missing fields based on Levenshtein distance #14466

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb

Thank you @Lordworms -- this looks really nice ❤️

I had one question, but otherwise this looks ready to me

cc @adriangb

alamb · 2025-02-06T19:36:48Z

datafusion/sqllogictest/test_files/errors.slt

+statement ok
+create table a(timestamp int, birthday int);
+
+query error DataFusion error: Schema error: No field named timetamp\. Did you mean 'a\.timestamp'\?\.


alamb · 2025-02-06T19:37:24Z

datafusion/sqllogictest/test_files/identifiers.slt

@@ -90,16 +90,16 @@ drop table  case_insensitive_test
 statement ok
 CREATE TABLE test("Column1" string) AS VALUES ('content1');

-statement error DataFusion error: Schema error: No field named column1. Valid fields are test\."Column1"\.
+statement error DataFusion error: Schema error: No field named column1\. Valid fields are test\."Column1"\.


Should't these errors also contain the nice new message?

Seems like we have to use full name to do match and the threshold is 0.5 now but in this case it is 'test.Column1' and column1 so the score is happen to be 0.5

adriangb · 2025-02-06T19:43:50Z

Amazing work!

It seems like these will just be part of the existing error message? Wouldn't it make sense to integrate with the new APIs in #13664 while we're at it?

I'd also suggest a test showing that a threshold of 0.5 is reasonable. Maybe a table with multiple columns that are increasingly further apart by eyeball (timesamp, timeamp, timp, ts, tokens, amp, foo?) and see which ones get returned and which don't.

comphead · 2025-02-06T20:27:21Z

datafusion/sqllogictest/test_files/errors.slt

+query error DataFusion error: Schema error: No field named timetamp\. Did you mean 'a\.timestamp'\?\.
+select timetamp from a;
+
+query error DataFusion error: Schema error: No field named dadsada\. Valid fields are a\.timestamp, a\.birthday\.


still old message 🤔

currently we would only do 'Did you mean' with a match over 0.5

right so in this case there were no matches over 0.5 and thus all of the fields were shown

alamb · 2025-02-06T21:21:47Z

It seems like these will just be part of the existing error message? Wouldn't it make sense to integrate with the new APIs in #13664 while we're at it?

FYI @eliaperantoni for your comments / suggestions

Lordworms · 2025-02-06T23:18:36Z

threshold of 0.5 is reasonable. Maybe a table with multiple columns that are increasingly further apart by eyeball (timesamp, timeamp, timp, ts, tokens, amp, foo?) and see which ones get returned and which don't.

Amazing work!

It seems like these will just be part of the existing error message? Wouldn't it make sense to integrate with the new APIs in #13664 while we're at it?

I can refactor that

I'd also suggest a test showing that a threshold of 0.5 is reasonable. Maybe a table with multiple columns that are increasingly further apart by eyeball (timesamp, timeamp, timp, ts, tokens, amp, foo?) and see which ones get returned and which don't.

eliaperantoni

Amazing! I love that Diagnostic is gaining so much traction ❤️. I have one comment, but otherwise looks perfect to me.

datafusion/common/src/column.rs

datafusion/common/src/error.rs

datafusion/common/src/column.rs

eliaperantoni · 2025-02-07T09:13:18Z

datafusion/sql/tests/cases/diagnostic.rs

-    assert_eq!(
-        diag.notes[0].message,
-        "possible reference to 'first_name' in table 'a'"
-    );
-    assert_eq!(
-        diag.notes[1].message,
-        "possible reference to 'first_name' in table 'b'"
-    );
+    assert_eq!(diag.notes[0].message, "possible column a.first_name");
+    assert_eq!(diag.notes[1].message, "possible column b.first_name");


What would happened if a very similar column (e.g. first_neme) was in scope. I guess they would see "possible column a.first_neme" even though there wasn't really ambiguity there because the planner didn't match that column.

I don't think this is desirable. What do you think @comphead @alamb

My feeling ambiguity might be okay, the final choice is on the user but what is definitely needs to avoid ig leventshein gets false positives and get 0.5 match. in this case the closest column name will be proposed(which can be totally different from what user typed) but others also hidden. If that happen user will have to do more action like EXPLAIN to find the schema. Not sure how to test such scenario though

Ok if you say non-exact matches should be shown too, I'm on board.

eliaperantoni · 2025-02-07T09:14:11Z

datafusion/common/src/column.rs

-        .map_err(|e| match &e {
-            DataFusionError::SchemaError(
-                SchemaError::FieldNotFound {
-                    field,
-                    valid_fields,
-                },
-                _,
-            ) => {
-                let mut diagnostic = Diagnostic::new_error(
-                    format!("column '{}' not found", &field.name()),
-                    field.spans().first(),
-                );
-                add_possible_columns_to_diag(&mut diagnostic, field, valid_fields);
-                e.with_diagnostic(diagnostic)
-            }
-            _ => e,
-        })


Thank you ❤️

fix bugs

alamb · 2025-02-09T10:48:00Z

I merged up from main to resolve a conflict with this branch

alamb · 2025-02-10T13:35:22Z

Thanks everyone! This is epic

github-actions bot added sqllogictest SQL Logic Tests (.slt) common Related to common crate labels Feb 6, 2025

alamb approved these changes Feb 6, 2025

View reviewed changes

alamb mentioned this pull request Feb 6, 2025

Improve error messages to include the function name. #14511

Merged

comphead reviewed Feb 6, 2025

View reviewed changes

Lordworms force-pushed the issue_14466 branch from 527fbd1 to 1f4a95b Compare February 6, 2025 21:03

github-actions bot added the sql SQL Planner label Feb 7, 2025

eliaperantoni reviewed Feb 7, 2025

View reviewed changes

datafusion/common/src/column.rs Outdated Show resolved Hide resolved

datafusion/common/src/error.rs Outdated Show resolved Hide resolved

Lordworms force-pushed the issue_14466 branch from 4032d58 to adfee55 Compare February 7, 2025 07:56

eliaperantoni reviewed Feb 7, 2025

View reviewed changes

datafusion/common/src/column.rs Outdated Show resolved Hide resolved

eliaperantoni reviewed Feb 7, 2025

View reviewed changes

Lordworms added 9 commits February 7, 2025 09:43

feat: add hint for missing fields

4f93b85

set threshold to include 0.5

7d8a83e

fix failed test

84f11ee

add diagnose

b4b12fe

fix clippy

10226c3

fix bugs

da5cbdf

fix bugs

add test

9e10bc9

fix test

46d7f53

fix clippy

a6a7d70

Lordworms force-pushed the issue_14466 branch from 2a052a6 to a6a7d70 Compare February 7, 2025 17:43

Merge remote-tracking branch 'apache/main' into issue_14466

09b1923

eliaperantoni approved these changes Feb 10, 2025

View reviewed changes

alamb merged commit 17396a5 into apache:main Feb 10, 2025
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add hint for missing fields #14521

feat: add hint for missing fields #14521

Lordworms commented Feb 6, 2025

alamb left a comment

alamb Feb 6, 2025

alamb Feb 6, 2025

Lordworms Feb 6, 2025

adriangb commented Feb 6, 2025

comphead Feb 6, 2025

Lordworms Feb 6, 2025

adriangb Feb 6, 2025

alamb commented Feb 6, 2025

Lordworms commented Feb 6, 2025

eliaperantoni left a comment

eliaperantoni Feb 7, 2025

comphead Feb 7, 2025

eliaperantoni Feb 10, 2025

eliaperantoni Feb 7, 2025

alamb commented Feb 9, 2025

alamb commented Feb 10, 2025

feat: add hint for missing fields #14521

feat: add hint for missing fields #14521

Conversation

Lordworms commented Feb 6, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriangb commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Feb 6, 2025

Lordworms commented Feb 6, 2025

eliaperantoni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Feb 9, 2025

alamb commented Feb 10, 2025