Skip to content

Support remaining pipe operators #1879

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

Merged
merged 30 commits into from
Jun 30, 2025

Conversation

simonvandel
Copy link
Contributor

Fixes #1758

This PR adds support for the remaining SQL pipe operators as defined by BigQuery.

@simonvandel simonvandel mentioned this pull request Jun 10, 2025
18 tasks
Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

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

Thanks @simonvandel! The changes look good to me overall! left some comments

@simonvandel
Copy link
Contributor Author

Hi @iffyio and thank you for the review.
I believe I have addressed all your comments in the latest commits

Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

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

Thanks @simonvandel! Just one comment re vec index and I think this should be good!

Comment on lines 11365 to 11366
// Take first
let join = joins.swap_remove(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

here can we add a condition to guard the vec indexing, so that we don't panic if our assumption or code is wrong for any reason? i.e.

if joins.len() != 1 {
   return Err(...)
}
let join = joins.swap_remove(0);

And maybe for the comment we can elaborate on if/why we expect exactly one entry in the returned join response? thinking since that might not be obvious for folks that stumble on this part of the code later on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! 552714a

Copy link
Contributor

@iffyio iffyio 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 @simonvandel!
cc @alamb

@iffyio iffyio merged commit abd80f9 into apache:main Jun 30, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support SQL pipe operator
2 participants