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

refactor(trino): port to sqlglot #7871

Merged
merged 3 commits into from
Jan 6, 2024

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Dec 31, 2023

Port the trino backend to sqlglot. I need to create some follow-up issues.

@cpcloud cpcloud changed the base branch from master to the-epic-split December 31, 2023 22:24
Copy link
Contributor

ACTION NEEDED

Ibis follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message.

Please update your PR title and description to match the specification.

@cpcloud cpcloud changed the title tes trino refactor(trino): port to sqlglot Dec 31, 2023
@cpcloud cpcloud marked this pull request as ready for review December 31, 2023 22:34
@cpcloud cpcloud marked this pull request as draft December 31, 2023 22:35
@cpcloud cpcloud force-pushed the tes-trino branch 2 times, most recently from 595ad37 to c625487 Compare January 1, 2024 16:45
@cpcloud cpcloud added refactor Issues or PRs related to refactoring the codebase trino The Trino backend labels Jan 1, 2024
@cpcloud cpcloud marked this pull request as ready for review January 3, 2024 11:56
@cpcloud cpcloud force-pushed the tes-trino branch 5 times, most recently from a90625f to 2636060 Compare January 3, 2024 17:24
@cpcloud
Copy link
Member Author

cpcloud commented Jan 3, 2024

Snowflake tests are passing:

cloud in 🌐 falcon in …/ibis on  tes-trino is 📦 v7.2.0 via 🐍 v3.10.13 via ❄️  impure (ibis-3.10.13-env)
❯ pytest -m snowflake -n 8 --dist loadgroup --snapshot-update -q
bringing up nodes...
......x..x............x....x.............x.xx..............x..x.........x......................x....xx........x...x.......x..xx..x.............x.........x.xx.x.xx..........x...........x....x [ 13%]
.xxx..x.x.......xx...x.xx..x......x.............x....xx..........xx.....xx..........xx..x...x.x.xxx..x.x.xx......x........xx....x...x...x.x......x.......x..x...............x...x..x.x.......x [ 26%]
.............................x......s.x..x.....x............x.........x.......x.....x.x.x........xxxx....x.x.....xx.x............xx................x........x....x.....xx...x..xx....xxxxx.xxx [ 39%]
xx.x..x.x......x.............................x.........xx.x....x...............................x.x........xx...x................x..................x...........xx..............xx......x...... [ 52%]
.....x.x....................x........x.....x.x.......xx.x..x.....................x.......................x..........................................x.....x...xx......x....x..x.x.....x........ [ 65%]
.xx....x.......x.....x..xx.x............................xx........x............x..x..x..........x.x..x.......x.....x.....x........x...s......xx.........................x.......x......x.x.... [ 78%]
...........xx...x..x..x.x....x.........x.xx.x................................................................................................................................................. [ 91%]
......................................s...x.xx.....x..............................................................................                                                             [100%]
1260 passed, 3 skipped, 198 xfailed in 131.07s (0:02:11)

@cpcloud cpcloud force-pushed the tes-trino branch 2 times, most recently from 0f76d65 to 04eaf7f Compare January 3, 2024 18:27
ibis/formats/pandas.py Outdated Show resolved Hide resolved
@@ -91,6 +92,48 @@ def rewrite_dropna(_):
return ops.Filter(_.parent, tuple(preds))


@replace(p.WindowFunction(p.First(x, y)))
Copy link
Member

Choose a reason for hiding this comment

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

Are these specific to trino?

Copy link
Member Author

@cpcloud cpcloud Jan 4, 2024

Choose a reason for hiding this comment

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

No, other backends that lack a proper First/Last aggregate (but implement it for scalar aggs in Ibis) can reuse this.

Copy link
Member

Choose a reason for hiding this comment

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

Can you please move these rules to sqlglot/rewrites.py?

Later we can do a consolidation pass over all the different rewrites.py

@kszucs kszucs enabled auto-merge (rebase) January 6, 2024 09:35
@cpcloud
Copy link
Member Author

cpcloud commented Jan 6, 2024

Snowflake tests look good:

cloud in 🌐 falcon in …/ibis on  tes-trino is 📦 v7.2.0 via 🐍 v3.10.13 via ❄️  impure (ibis-3.10.13-env)
❯ pytest -m snowflake -n 8 --dist loadgroup --snapshot-update -q
bringing up nodes...
.......x.x.........xxx...xx.x...xx.......x...xx.x..x........s......x.x..xx........x......x..x.x.......x...............x......x..x.....x..x.....x......xxx..x.x.x.........x.............x...... [ 13%]
...............x...x.....x.x.x...............x.....x....xx............xx.....x..x.x..xx.x...x..x........x........x.............x...x......x..............x....x......x.xxx...x.x..x.x......... [ 26%]
.xxx...xx......xx.xx..x.....x..x...x...x..x........x......x....x..x.x...x....x..........x..x......xx......x.....x..x....x..........x.......................x...........x.......x...........xx. [ 39%]
...xxxx......x...x............................x............................x...........................................x................x.............x..................x.....x.....x.x...... [ 52%]
.x.x.x...x..x.x....xx...xx.........x..x..x....xx.x.........x...x.........xx.......x............x................x.x............x.x...........x..............x...x.....x..x......xx...........x [ 65%]
.......x...x.x..xx........x.x..x.xxx..x.xxxxxxxxx......x..x....x.x.....x.x...x....x.x....................x..x........x....x..........x......x.........................................x....... [ 78%]
.........................................x......................................................x.s...............................xx.......................................................... [ 91%]
.....................................................s.............................................................................                                                            [100%]
1260 passed, 3 skipped, 198 xfailed in 125.41s (0:02:05)

@kszucs kszucs merged commit 605a873 into ibis-project:the-epic-split Jan 6, 2024
32 checks passed
@cpcloud cpcloud deleted the tes-trino branch January 6, 2024 09:43
cpcloud added a commit that referenced this pull request Jan 12, 2024
)

This PR pulls out some changes from #7871, including some removal of
redundant translation rules and fixing some xpassing tests
cpcloud added a commit that referenced this pull request Jan 13, 2024
)

This PR pulls out some changes from #7871, including some removal of
redundant translation rules and fixing some xpassing tests
cpcloud added a commit that referenced this pull request Jan 17, 2024
)

This PR pulls out some changes from #7871, including some removal of
redundant translation rules and fixing some xpassing tests
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 1, 2024
…is-project#7904)

This PR pulls out some changes from ibis-project#7871, including some removal of
redundant translation rules and fixing some xpassing tests
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 1, 2024
…is-project#7904)

This PR pulls out some changes from ibis-project#7871, including some removal of
redundant translation rules and fixing some xpassing tests
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 1, 2024
…is-project#7904)

This PR pulls out some changes from ibis-project#7871, including some removal of
redundant translation rules and fixing some xpassing tests
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 2, 2024
…is-project#7904)

This PR pulls out some changes from ibis-project#7871, including some removal of
redundant translation rules and fixing some xpassing tests
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 2, 2024
…is-project#7904)

This PR pulls out some changes from ibis-project#7871, including some removal of
redundant translation rules and fixing some xpassing tests
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 2, 2024
…is-project#7904)

This PR pulls out some changes from ibis-project#7871, including some removal of
redundant translation rules and fixing some xpassing tests
cpcloud added a commit to cpcloud/ibis that referenced this pull request Feb 4, 2024
…is-project#7904)

This PR pulls out some changes from ibis-project#7871, including some removal of
redundant translation rules and fixing some xpassing tests
cpcloud added a commit to cpcloud/ibis that referenced this pull request Feb 5, 2024
…is-project#7904)

This PR pulls out some changes from ibis-project#7871, including some removal of
redundant translation rules and fixing some xpassing tests
kszucs pushed a commit that referenced this pull request Feb 5, 2024
)

This PR pulls out some changes from #7871, including some removal of
redundant translation rules and fixing some xpassing tests
kszucs pushed a commit that referenced this pull request Feb 6, 2024
)

This PR pulls out some changes from #7871, including some removal of
redundant translation rules and fixing some xpassing tests
kszucs pushed a commit that referenced this pull request Feb 6, 2024
)

This PR pulls out some changes from #7871, including some removal of
redundant translation rules and fixing some xpassing tests
cpcloud added a commit to cpcloud/ibis that referenced this pull request Feb 12, 2024
…is-project#7904)

This PR pulls out some changes from ibis-project#7871, including some removal of
redundant translation rules and fixing some xpassing tests
cpcloud added a commit that referenced this pull request Feb 12, 2024
)

This PR pulls out some changes from #7871, including some removal of
redundant translation rules and fixing some xpassing tests
cpcloud added a commit to cpcloud/ibis that referenced this pull request Feb 12, 2024
…is-project#7904)

This PR pulls out some changes from ibis-project#7871, including some removal of
redundant translation rules and fixing some xpassing tests
cpcloud added a commit that referenced this pull request Feb 12, 2024
)

This PR pulls out some changes from #7871, including some removal of
redundant translation rules and fixing some xpassing tests
kszucs pushed a commit that referenced this pull request Feb 12, 2024
)

This PR pulls out some changes from #7871, including some removal of
redundant translation rules and fixing some xpassing tests
ncclementi pushed a commit to ncclementi/ibis that referenced this pull request Feb 21, 2024
…is-project#7904)

This PR pulls out some changes from ibis-project#7871, including some removal of
redundant translation rules and fixing some xpassing tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Issues or PRs related to refactoring the codebase trino The Trino backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants