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(intervals): conslidate interval conversion under _make_interval base compiler implementation #9800

Merged

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Aug 8, 2024

Follow-up to #9799 to consolidate some repetitive interval-handling code.

@cpcloud cpcloud added refactor Issues or PRs related to refactoring the codebase timestamps Issues related to the timestamp API labels Aug 8, 2024
@cpcloud cpcloud requested a review from gforsyth August 8, 2024 16:50
@cpcloud cpcloud force-pushed the consolidate-interval-implementations branch from fbc22a4 to 7777fb8 Compare August 8, 2024 16:53
@gforsyth
Copy link
Member

gforsyth commented Aug 8, 2024

Exasol is an XPASS -- Postgres and Risingwave are throwing syntax errors -- I wouldn't think that op.arg and arg are different, but maybe they are?

@cpcloud
Copy link
Member Author

cpcloud commented Aug 8, 2024

Yep, just working through sqlite now, it's quite ... unique.

@cpcloud
Copy link
Member Author

cpcloud commented Aug 8, 2024

Hell yeah, it's just a sea of unfailure!

image

@cpcloud
Copy link
Member Author

cpcloud commented Aug 8, 2024

Ah, for postgres we're not calling visit_Cast ... fixing that now.

@cpcloud cpcloud added the sqlite The SQLite backend label Aug 8, 2024
@cpcloud cpcloud force-pushed the consolidate-interval-implementations branch 2 times, most recently from a4705d8 to 169b258 Compare August 8, 2024 18:20
@cpcloud
Copy link
Member Author

cpcloud commented Aug 8, 2024

Gonna do a rebase merge on this one so the sqlite feat gets picked up in the release notes.

@cpcloud cpcloud added the feature Features or general enhancements label Aug 8, 2024
@cpcloud cpcloud force-pushed the consolidate-interval-implementations branch from 169b258 to 9f9cd8c Compare August 8, 2024 18:21
Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

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

nice!

@gforsyth gforsyth added the ci-run-cloud Run BigQuery, Snowflake, Databricks, and Athena backend tests label Aug 8, 2024
@cpcloud
Copy link
Member Author

cpcloud commented Aug 8, 2024

Looks like I might be hitting a sqlite version issue.

@ibis-docs-bot ibis-docs-bot bot removed the ci-run-cloud Run BigQuery, Snowflake, Databricks, and Athena backend tests label Aug 8, 2024
@cpcloud
Copy link
Member Author

cpcloud commented Aug 8, 2024

sad

image

@cpcloud
Copy link
Member Author

cpcloud commented Aug 8, 2024

I love that if you pass a bogus argument to sqlite, it returns NULL. <-- Sarcasm

@cpcloud
Copy link
Member Author

cpcloud commented Aug 8, 2024

I think I might just raise an exception for sqlite < 3.46. Otherwise we're looking at implementing this function

https://github.com/sqlite/sqlite/blob/a0789fcc4917af0cdf343c073152045a990a7313/src/date.c#L299

@cpcloud cpcloud force-pushed the consolidate-interval-implementations branch 4 times, most recently from 3004229 to 354d7e6 Compare August 8, 2024 19:28
@cpcloud
Copy link
Member Author

cpcloud commented Aug 8, 2024

yyyyy

image

@cpcloud
Copy link
Member Author

cpcloud commented Aug 8, 2024

GHA is running on 3.40.x 😢

@cpcloud
Copy link
Member Author

cpcloud commented Aug 8, 2024

At this point I'm ready to declare versioning bankruptcy and xfail with strict=False since we're running these tests on the nix jobs which have the latest version of sqlite.

@cpcloud
Copy link
Member Author

cpcloud commented Aug 8, 2024

Oh wait I forgot we allow condition in backend markers. Trying that now.

@cpcloud cpcloud force-pushed the consolidate-interval-implementations branch 2 times, most recently from 5b22b2b to 8eca897 Compare August 9, 2024 09:49
@cpcloud cpcloud force-pushed the consolidate-interval-implementations branch 3 times, most recently from 5fea4ef to 1e0dddf Compare August 9, 2024 11:38
@cpcloud cpcloud added the ci-run-cloud Run BigQuery, Snowflake, Databricks, and Athena backend tests label Aug 9, 2024
@cpcloud cpcloud force-pushed the consolidate-interval-implementations branch from 1e0dddf to d267be7 Compare August 9, 2024 11:40
@ibis-docs-bot ibis-docs-bot bot removed the ci-run-cloud Run BigQuery, Snowflake, Databricks, and Athena backend tests label Aug 9, 2024
@cpcloud cpcloud force-pushed the consolidate-interval-implementations branch from d267be7 to e96c3e2 Compare August 9, 2024 11:46
@cpcloud
Copy link
Member Author

cpcloud commented Aug 9, 2024

Clouds look good:

…/ibis on  consolidate-interval-implementations is 📦 v9.3.0 via 🐍 v3.10.14 via ❄️   impure (ibis-3.10.14-env)
❯ pytest -m snowflake -n 8 --dist loadgroup --snapshot-update -q && pytest -m bigquery -n auto --dist loadgroup --snapshot-update -q
bringing up nodes...
................................................xx.........................................x................................................................................x......... [  9%]
.........x......................x........s................x.....x.x..................x................x.....x..............x...xx..x.....x......xxx.x..x.....................x..xxx.xx [ 19%]
.......x.......x.s..............x.x.xx.xxx.x..xxx.x.xxx..x.x..xx.x.x...x..xx..xx..x.x.....x.xxxxxx.x..xx..x...xx.x..x.x.xx..x..x..xx.xx.xxx.x.xxxx.x.xx.....x.....xx..x............... [ 29%]
...x.x.....x......x...........................x.................xx.............x.....x...........xx..........x.............................x..xx..................x..........xx....... [ 39%]
...........x.......x..............x....................................................x....x..........x.x............................................................................ [ 49%]
........................................xx.....x...........x.x......................xx......x..x...........x....x.........x...x.............x...........x.x....x.......x.............. [ 59%]
....x...x..x.x............x......x.x.x.x..x.x..x..xxxxx.xxxxx.....xx..x...x................x.....................s......................s..s.........................xx.xx.xxxx.x.x.xx [ 68%]
....x.....x...........x........x..............................x.............x......x...........x.x...x...x....x......x...................x..........x..........................x...... [ 78%]
...xx...x......x..............x..........x.x.........xx......................x......................x..........x.x.................x.x...x....x.x...x...s.................x........... [ 88%]
.x........s.....................................................................s..................................................................................................... [ 98%]
......s....s..................                                                                                                                                                         [100%]
1624 passed, 10 skipped, 216 xfailed in 210.48s (0:03:30)
bringing up nodes...
.....................................................................................................................................................................x...sssssssssssss [  8%]
ssssssss...xx.................sssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssx....x.........x...........x.xx...........ss.......xx.........x..xx........x......x...x.. [ 17%]
..x.s.................x.x......x.xxx....x..xx..xx.x.xxxxxxxxxxxxxxxxxxxx.xx.xx.xxxxxxxx.xxxxxxxxx.xx.xxxxxxx.xxxxxxxxxxxxx.xxxxxxxxxxxxxxxx.xxxxxxxxx.xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx [ 26%]
xxxxxxxxx.xxx..x....x......x.xx...x.....s...........x...x.............x..x.............................x..x............xxx...................................x.x....x..x........xx.... [ 35%]
.xxx.x.....x..x.xx..........xx..xx.x....x..x.....x.x....x.........x...x........x..x....xx....x..........x.....x.x..x.xx.......x..xx....xx.....x.................x......x...x...x...... [ 43%]
x...x....xx..x.xxx......x...........x..xxx.....x....s..................x..........x...................x.....X.x...x.x...xx.xx.xx..xXx.....x.....x.x.x..........x...........x.....x.... [ 52%]
.x.....x.x.x.................x...x..........x.....x.x..x.x.X.x.......XxX..x...x..........xx..............x.....................x.x......x.x....x...xx...x....x....x..xx...........x... [ 61%]
x.........x...................................x............x....x......x..........x..x.......s.......x.............x............s..................................................... [ 70%]
......x..........xs...................x......x......x...x.....x.........x........x.x..x..x.x......x..x.................x......xx...x.....................x....x...........x....xxxxxxx [ 79%]
xx.....x............xx.x....x............................................................................x.........................................................x......x........... [ 87%]
......................................................................................................................x......x..........................................x............. [ 96%]
.....................................................................                                                                                                                                              [100%]
1632 passed, 92 skipped, 342 xfailed, 5 xpassed in 309.60s (0:05:09)

@cpcloud cpcloud force-pushed the consolidate-interval-implementations branch from e96c3e2 to 3065cb8 Compare August 9, 2024 11:56
@cpcloud cpcloud enabled auto-merge (rebase) August 9, 2024 11:57
@cpcloud cpcloud added sql Backends that generate SQL trino The Trino backend labels Aug 9, 2024
@cpcloud cpcloud force-pushed the consolidate-interval-implementations branch from 3065cb8 to f132536 Compare August 9, 2024 12:02
@cpcloud cpcloud merged commit 1133973 into ibis-project:main Aug 9, 2024
82 checks passed
@cpcloud cpcloud deleted the consolidate-interval-implementations branch August 9, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Features or general enhancements refactor Issues or PRs related to refactoring the codebase sql Backends that generate SQL sqlite The SQLite backend timestamps Issues related to the timestamp API trino The Trino backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants