Skip to content

Improve unsupported compound identifier message - #13605

Merged
alamb merged 1 commit into
apache:mainfrom
alamb:alamb/better_error
Nov 30, 2024
Merged

Improve unsupported compound identifier message#13605
alamb merged 1 commit into
apache:mainfrom
alamb:alamb/better_error

Conversation

@alamb

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

While working on the upgrade of sqlparser, the structure has changed and thus the debug formatting has changed (so I needed to change this error message anyways)

The error message was hard to understand anyways, so I spent a few moments improving it to show the actual indentifers rather than a debug version

What changes are included in this PR?

  1. Improve error message and test

Are these changes tested?

Yes

Are there any user-facing changes?

@github-actionsgithub-actionsBot added sql SQL Planner sqllogictest SQL Logic Tests (.slt) labels Nov 29, 2024
SELECT COUNT(*) FROM nonexistentcatalog.public.aggregate_test_100

statement error Error during planning: Unsupported compound identifier '\[Ident \{ value: "way", quote_style: None \}, Ident \{ value: "too", quote_style: None \}, Ident \{ value: "many", quote_style: None \}, Ident \{ value: "namespaces", quote_style: None \}, Ident \{ value: "as", quote_style: None \}, Ident \{ value: "ident", quote_style: None \}, Ident \{ value: "prefixes", quote_style: None \}, Ident \{ value: "aggregate_test_100", quote_style: None \}\]'
statement error DataFusion error: Error during planning: Unsupported compound identifier 'way\.too\.many\.namespaces\.as\.ident\.prefixes\.aggregate_test_100'\. Expected 1, 2 or 3 parts, got 8

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This line has to change anyways with the sqlparser update, and I think this PR has a nicer message

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@jonahgaojonahgao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

impl IdentTaker {
fn take(&mut self, enable_normalization: bool) -> String {
let ident = self.0.pop().expect("no more identifiers");
IdentNormalizer::new(enable_normalization).normalize(ident)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not related to the current PR, maybe we can move IdentNormalizer into IdentTaker to simplify take() and avoid creating normalizer every time.

@alambalambNov 30, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

will do in a follow on PR

Update: in #13609

SELECT COUNT(*) FROM nonexistentcatalog.public.aggregate_test_100

statement error Error during planning: Unsupported compound identifier '\[Ident \{ value: "way", quote_style: None \}, Ident \{ value: "too", quote_style: None \}, Ident \{ value: "many", quote_style: None \}, Ident \{ value: "namespaces", quote_style: None \}, Ident \{ value: "as", quote_style: None \}, Ident \{ value: "ident", quote_style: None \}, Ident \{ value: "prefixes", quote_style: None \}, Ident \{ value: "aggregate_test_100", quote_style: None \}\]'
statement error DataFusion error: Error during planning: Unsupported compound identifier 'way\.too\.many\.namespaces\.as\.ident\.prefixes\.aggregate_test_100'\. Expected 1, 2 or 3 parts, got 8

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@Weijun-HWeijun-H left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thanks @alamb

@alamb
alamb merged commit 68c92c0 into apache:mainNov 30, 2024
@alamb

Copy link
Copy Markdown
ContributorAuthor

Thanks @Weijun-H and @jonahgao

@alamb
alamb deleted the alamb/better_error branch November 30, 2024 12:51
@alambalamb mentioned this pull request Nov 30, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sqlSQL PlannersqllogictestSQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@alamb@Weijun-H@jonahgao