Skip to content
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

Return an error when a column does not exist in window function #9202

Merged
merged 9 commits into from
Feb 14, 2024

Conversation

PhVHoang
Copy link
Contributor

Which issue does this PR close?

Closes #9166.

Rationale for this change

To return an error instead of panicking when columns don't exist in window function.

What changes are included in this PR?

Code, tests

Are these changes tested?

Yes

Are there any user-facing changes?

Signed-off-by: Hoang Pham <a2k40pbc@gmail.com>
Signed-off-by: Hoang Pham <a2k40pbc@gmail.com>
Signed-off-by: Hoang Pham <a2k40pbc@gmail.com>
Signed-off-by: Hoang Pham <a2k40pbc@gmail.com>
Signed-off-by: Hoang Pham <a2k40pbc@gmail.com>
Signed-off-by: Hoang Pham <a2k40pbc@gmail.com>
Signed-off-by: Hoang Pham <a2k40pbc@gmail.com>
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 @PhVHoang -- this is a great first contribution.

I think the code can be made simpler and I left a comment for your consideration

let idx_result = schema.index_of_column(col);

// Propagate the error to the caller and return it immediately
let idx = idx_result
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the ok() function call here discards the error.

I think changing

                        let idx = schema.index_of_column(col).unwrap();

To

                        let idx = schema.index_of_column(col).ok()?;

accomplishes the same thing (and your test still passes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much for your suggestion. Yes it's way simpler than that of mine.

Fixed.

Signed-off-by: Hoang Pham <a2k40pbc@gmail.com>
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 @PhVHoang -- this looks good to me

I think it would be good if @mustafasrepo could also review this PR before we merge, if he has time

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @PhVHoang
its better than crash

@comphead
Copy link
Contributor

@PhVHoang please add the test also for non existent partition by column
and qualified column like t.a, schema.t.a, catalog.schema.t.a

Copy link
Contributor

@mustafasrepo mustafasrepo left a comment

Choose a reason for hiding this comment

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

Thanks @PhVHoang for this PR. LGTM!

@alamb
Copy link
Contributor

alamb commented Feb 14, 2024

@PhVHoang please add the test also for non existent partition by column and qualified column like t.a, schema.t.a, catalog.schema.t.a

I think the only thing remaining before merging this PR is the suggestion from @comphead #9202 (comment) for a few more tests. I can add them hopefully later today if no one else has a chance

Signed-off-by: Hoang Pham <a2k40pbc@gmail.com>
@PhVHoang
Copy link
Contributor Author

@alamb @comphead I added some more tests. Please take a look and let me know if these tests are sufficient for this change.

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @PhVHoang

@alamb alamb merged commit 0444dad into apache:main Feb 14, 2024
23 checks passed
@alamb
Copy link
Contributor

alamb commented Feb 14, 2024

Thanks again everyone

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.

Invalid column reference in window function order by causes panic
4 participants