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.

Sign up for GitHub

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

Jump to bottom

fix(duckdb): workaround for duckdb Map NULL bugs #8649

Merged
merged 1 commit into from
Apr 17, 2024
Merged

fix(duckdb): workaround for duckdb Map NULL bugs #8649

merged 1 commit into from
Apr 17, 2024

Conversation

NickCrews
Copy link
Contributor

partially addresses #8632

@github-actions GitHub Actions
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.

@NickCrews NickCrews force-pushed the map_nulls branch 2 times, most recently from aaf1582 to 6925c90 Compare March 13, 2024 20:55
@NickCrews NickCrews changed the title Add workaround for duckdb Map NULL bugs feat: workaround for duckdb Map NULL bugs Mar 13, 2024
@NickCrews
Copy link
Contributor Author

Once that xfail test starts passing, then we could

  • check for the duckdb version, and if we are running a broken version, then do the workaround, otherwise short-circuit to the simple path
  • something else?

@NickCrews
Copy link
Contributor Author

errr, this is probably the wrong way around this, I should be fixing this at the duckdb level, not the global ibis level. Otherwise we miss MapValues that are created via other APIs, like ibis.literal()

@cpcloud cpcloud added bug Incorrect behavior inside of ibis duckdb The DuckDB backend community Issues or PRs requiring help from the community ecosystem External projects or activities requires upstream support Feature or bug requires support from the upstream project labels Mar 14, 2024
cpcloud
cpcloud requested changes Mar 14, 2024
View reviewed changes
ibis/backends/tests/test_map.py Outdated Show resolved Hide resolved
ibis/backends/tests/test_map.py Outdated Show resolved Hide resolved
ibis/expr/types/structs.py Outdated Show resolved Hide resolved
ibis/expr/types/arrays.py Outdated Show resolved Hide resolved
ibis/expr/types/maps.py Outdated Show resolved Hide resolved
@NickCrews NickCrews mentioned this pull request Mar 15, 2024
Closed
@NickCrews NickCrews mentioned this pull request Mar 19, 2024
Closed
1 task
@NickCrews NickCrews force-pushed the map_nulls branch 6 times, most recently from 3d31c13 to ddd398e Compare March 19, 2024 18:08
@NickCrews NickCrews force-pushed the map_nulls branch 3 times, most recently from c0e3761 to 18cc81e Compare March 21, 2024 20:47
@NickCrews NickCrews force-pushed the map_nulls branch 2 times, most recently from e4f99b5 to 7660c2b Compare March 21, 2024 20:48
@NickCrews @cpcloud
fix: add workaround for NULL map bugs
fdeaf98 
partially addresses ibis-project#8632

postgres started failing these new test too,
so had to adjust that visit_Map()
@cpcloud cpcloud added this to the 9.0 milestone Apr 17, 2024
@cpcloud cpcloud changed the title feat: workaround for duckdb Map NULL bugs fix(duckdb): workaround for duckdb Map NULL bugs Apr 17, 2024
@cpcloud cpcloud enabled auto-merge (squash) April 17, 2024 11:27
@cpcloud cpcloud merged commit 75d32e5 into ibis-project:main Apr 17, 2024
82 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpcloud cpcloud cpcloud approved these changes

Assignees
No one assigned
Labels
bug Incorrect behavior inside of ibis community Issues or PRs requiring help from the community duckdb The DuckDB backend ecosystem External projects or activities requires upstream support Feature or bug requires support from the upstream project
Projects
None yet
Milestone
9.0
Development

Successfully merging this pull request may close these issues.
2 participants
@NickCrews @cpcloud