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(datafusion): add map methods to datafusion compiler #10510

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

venkat-oss
Copy link

@venkat-oss venkat-oss commented Nov 19, 2024

  • Added Datafusion supported map functions to Datafusion compiler.
  • This PR in Datafusion repo adds support for certain map functions.

ops.Map - ✅ (Already supported by ibis)
ops.MapLength - ✅
ops.MapGet - ✅
ops.MapContains - ✅
ops.MapKeys - ✅
ops.MapValues - ✅
ops.MapMerge - 🆘 Need help

@github-actions github-actions bot added the sql Backends that generate SQL label Nov 19, 2024
@venkat-oss venkat-oss changed the title feat: add datafusion map methods to datafusion compiler feat(datafusion): add datafusion map methods to datafusion compiler Nov 19, 2024
@venkat-oss venkat-oss changed the title feat(datafusion): add datafusion map methods to datafusion compiler feat(datafusion): add map methods to datafusion compiler Nov 19, 2024
@venkat-oss
Copy link
Author

venkat-oss commented Nov 19, 2024

@gforsyth and @cpcloud could you please provide some feedback/pointers on how MapMerge could be implemented?

I'm not very sure how MapMerge could be implemented unless supported by datafusion.

@IndexSeek
Copy link
Member

Very nice! Thank you for working on this, @venkat-oss.

Could you remove the "notyet" markers in the tests so that we can ensure these operations run properly with DataFusion? Here is an example of a marker and where they can be found in the case of the tests for map:

@mark_notyet_datafusion
def test_map_length(con):
expr = ibis.literal(dict(a="A", b="B")).length()
assert con.execute(expr) == 2

After this, can you run your code through the linter? If you have just, you can do: just fmt. This command will run the following (in case you don't have just available):

    ruff format .
    ruff check --fix .

@github-actions github-actions bot added the tests Issues or PRs related to tests label Nov 20, 2024
@cpcloud
Copy link
Member

cpcloud commented Dec 17, 2024

@venkat-oss Any chance you can continue work on this PR? It looks like there are a sizable number of map-type test failures related to your new implementations. Let us know if you need some assistance.

@venkat-oss
Copy link
Author

Will try to get this over the finish line soon @cpcloud 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sql Backends that generate SQL tests Issues or PRs related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants