Skip to content

Conversation

@Jefffrey
Copy link
Contributor

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?

@github-actions github-actions bot added the sql SQL Planner label Apr 18, 2024
.position(|item| *item == pk.value)
.ok_or_else(|| {
DataFusionError::Execution(
"Unique key doesn't exist".to_string(),
Copy link
Member

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?

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing, done 👍

Copy link
Contributor

@alamb alamb left a 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 🙏

.unwrap_or("".to_string());
DataFusionError::Execution(
"Primary key doesn't exist".to_string(),
format!("Column for unique constraint {}not found in schema: {}", name,u.value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
format!("Column for unique constraint {}not found in schema: {}", name,u.value)
format!("Column for unique constraint {} not found in schema: {}", name,u.value)

Copy link
Contributor Author

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 🙂

Copy link
Contributor

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!(
Copy link
Contributor

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

Copy link
Contributor Author

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

@alamb alamb merged commit fc34dac into apache:main Apr 21, 2024
@alamb
Copy link
Contributor

alamb commented Apr 21, 2024

Thanks @Jefffrey !

ccciudatu pushed a commit to hstack/datafusion that referenced this pull request Apr 26, 2024
…he#10137)

* chore(deps): update sqlparser requirement from 0.44.0 to 0.45.0

* Bump datafusion-cli Cargo.lock

* Enhance error messages
@Jefffrey Jefffrey deleted the sqlparser-45.0.0 branch September 22, 2025 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sql SQL Planner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants