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

implement - nil datatype - polar null type column #805

Merged
merged 4 commits into from Jan 9, 2024

Conversation

lkarthee
Copy link
Member

@lkarthee lkarthee commented Jan 9, 2024

For addressing issues relating to null datatype.

Its not ready for merging - need code review and some help.

@lkarthee
Copy link
Member Author

lkarthee commented Jan 9, 2024

@josevalim @philss There are a lot of test failures - i will fix them after review ..

lib/explorer/series.ex Outdated Show resolved Hide resolved
@josevalim
Copy link
Member

Thank you!

Let's add some tests that to_list works and returns a list of nils. I assume to_binary and to_iovec both fail? Do they fail with a decent error message at least?

@lkarthee
Copy link
Member Author

lkarthee commented Jan 9, 2024

Sure I will add test cases for to_list.

Error message for to_iovec and to_binary.

** (ArgumentError) cannot convert series of dtype :null into iovec
    (explorer 0.8.0-dev) lib/explorer/series.ex:769: Explorer.Series.to_iovec/1
    #cell:2wexfollu25fyu43:14: (file)

For [1, 2, nil] to_iovec fails with :

** (RuntimeError) Generic Error: cannot invoke to_iovec on series with nils

How to fix iovec error any pointers ?

@josevalim
Copy link
Member

Those are all expected and good. So we only need tests for to_list and we are good to go. :)

@josevalim josevalim merged commit 00258f5 into elixir-explorer:main Jan 9, 2024
4 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants