Skip to content

Support some of pipe operators #1759

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 13 commits into from
May 2, 2025
Merged

Conversation

simonvandel
Copy link
Contributor

Part of #1758

Still missing (ran out of time today, can be done in follow-ups perhaps)

  • join
  • union|intersect|except
  • call
  • tablesample
  • pivot
  • unpivot

I'm a first time contributor - please let me know if there are better places to store the PipeOperator in the AST

Part of apache#1758

Still missing
- join
- union|intersect|except
- call
- tablesample
- pivot
- unpivot
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 reasonable to me overall, left some comments

@iffyio iffyio marked this pull request as draft March 18, 2025 06:23
@simonvandel simonvandel marked this pull request as ready for review April 16, 2025 18:09
@simonvandel
Copy link
Contributor Author

Hi @iffyio

I'm sorry for the long response time.
I have now revised the code according to your comments.
Adding more tests found some bugs, so I had to change the implementation a bit

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! left a common re error handling, otherwise the changes look good to me

@simonvandel
Copy link
Contributor Author

simonvandel commented Apr 28, 2025

Thanks @iffyio

I resolved the merge conflict, and fixed your comment on error handling.
Additionally, I added a dialect check before parsing pipe operators. See f01301d

@iffyio
Copy link
Contributor

iffyio commented Apr 29, 2025

@simonvandel could you take a look at the CI issues when you get the time?

@simonvandel
Copy link
Contributor Author

@simonvandel could you take a look at the CI issues when you get the time?

I think CI should be green with 89309c1

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 e5d2215 into apache:main May 2, 2025
9 checks passed
@simonvandel simonvandel mentioned this pull request Mar 9, 2025
18 tasks
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.

2 participants