-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(deps): update sqlparser requirement from 0.44.0 to 0.45.0 #10137
Conversation
.position(|item| *item == pk.value) | ||
.ok_or_else(|| { | ||
DataFusionError::Execution( | ||
"Unique key doesn't exist".to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we include the field name in the error message to make it easier for the user to debug when they hit this error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as well as unique key details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing, done 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much @Jefffrey -- looks great to me 🙏
DataFusionError::Execution( | ||
"Primary key doesn't exist".to_string(), | ||
format!("Column for unique constraint {}not found in schema: {}", name,u.value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format!("Column for unique constraint {}not found in schema: {}", name,u.value) | |
format!("Column for unique constraint {} not found in schema: {}", name,u.value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah the missing whitespace is actually intentional here as the unique constraint may be unnamed 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it -- sorry -- maybe we can add a test or something of the error message in a follow on PR to make it clearer.
.iter() | ||
.position(|item| *item == pk.value) | ||
.ok_or_else(|| { | ||
DataFusionError::Execution(format!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be cool to add the name in this error message too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose not to add it here as you'd only have a single primary key constraint on a table anyway so user's would ideally already know where to look
Thanks @Jefffrey ! |
…he#10137) * chore(deps): update sqlparser requirement from 0.44.0 to 0.45.0 * Bump datafusion-cli Cargo.lock * Enhance error messages
Which issue does this PR close?
Supersedes #10085
Rationale for this change
What changes are included in this PR?
Handle some sqlparser API changes. Have tried to keep changes minimum.
Are these changes tested?
Are there any user-facing changes?