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

feat(api): support Deferreds in Array.map and .filter #8267

Merged
merged 4 commits into from
Feb 8, 2024

Conversation

NickCrews
Copy link
Contributor

fixes #8180

Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together!

Bit of feedback around tests, and the general approach looks good.

ibis/backends/tests/test_array.py Outdated Show resolved Hide resolved
ibis/backends/tests/test_array.py Outdated Show resolved Hide resolved
ibis/backends/tests/test_array.py Outdated Show resolved Hide resolved
ibis/backends/tests/test_array.py Outdated Show resolved Hide resolved
ibis/backends/tests/test_array.py Outdated Show resolved Hide resolved
ibis/backends/tests/test_array.py Outdated Show resolved Hide resolved
ibis/expr/types/arrays.py Show resolved Hide resolved
ibis/expr/types/arrays.py Show resolved Hide resolved
ibis/expr/types/arrays.py Show resolved Hide resolved
ibis/expr/types/arrays.py Show resolved Hide resolved
@cpcloud cpcloud added this to the 9.0 milestone Feb 7, 2024
@cpcloud cpcloud added ux User experience related issues expressions Issues or PRs related to the expression API labels Feb 7, 2024
@NickCrews
Copy link
Contributor Author

Thanks for the review @cpcloud ! I think I found a fragility with how we generate argument names for the lambdas in SQL. Waiting to here your suggestions on how to fix that, I'm not familiar with the reasoning behind how to generate those arg names. Can we just do __ibis_param__, not templating in the name of the arg?

@kszucs kszucs changed the title feat: support Deferreds in Array.map and .filter feat(api): support Deferreds in Array.map and .filter Feb 7, 2024
@cpcloud
Copy link
Member

cpcloud commented Feb 8, 2024

I think I found a fragility with how we generate argument names for the lambdas in SQL.

Certainly possible!

We're generating names to workaround an issue in DuckDB (opened by you in #7676 😄, fixed in #7741).

I reported this upstream here: duckdb/duckdb#9981

and the fixed was merged 3 weeks ago in duckdb/duckdb#10150.

I think DuckDB is the only backend that had this problem, so when DuckDB releases a format-stable version we can bump its version lower bound and remove the workaround.

This is in prep for adding in Deferreds
@cpcloud cpcloud force-pushed the deferred-filter-map branch from 4b9201e to 6a523bd Compare February 8, 2024 13:01
@cpcloud cpcloud force-pushed the deferred-filter-map branch from 6a523bd to 31bb0dd Compare February 8, 2024 13:06
@cpcloud cpcloud enabled auto-merge (squash) February 8, 2024 13:17
@cpcloud cpcloud disabled auto-merge February 8, 2024 13:17
@cpcloud
Copy link
Member

cpcloud commented Feb 8, 2024

Clouds are passing array tests:

❯ pytest -m 'snowflake or bigquery' ibis/backends/tests/test_array.py -n 8 -q
bringing up nodes...
.......x.....x.............x....x........xs....xx..s...xx.x.....xx........x......x...x...x..s............x.x...x.........x.......xx......s.x.........x....x.....x.x........x..xx...x....x..... [ 71%]
....xs..x...s......x...xx.....x..........x.........x...........x.........x.x                                                                                                                   [100%]
217 passed, 6 skipped, 43 xfailed in 197.49s (0:03:17)

@cpcloud cpcloud merged commit 8289d2c into ibis-project:main Feb 8, 2024
80 checks passed
@cpcloud
Copy link
Member

cpcloud commented Feb 8, 2024

Thanks @NickCrews!

@NickCrews NickCrews deleted the deferred-filter-map branch February 8, 2024 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
expressions Issues or PRs related to the expression API ux User experience related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: UX: Support deferreds in addition to lambdas in Array.map, Array.filter
2 participants