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

ARROW-2226, ARROW-2233: [JS] Dictionary bugfixes #1671

Closed
wants to merge 4 commits into from

Conversation

trxcllnt
Copy link
Contributor

@TheNeuralBit
Copy link
Member

How does this bug present itself? I thought maybe DictionaryVector.slice wouldn't work, but I just wrote some unit tests for that on master and it seems ok

@wesm wesm changed the title ARROW-2226: [JS] use indicies.offset in DictionaryData constructor ARROW-2226: [JS] use indices.offset in DictionaryData constructor Feb 27, 2018
@wesm
Copy link
Member

wesm commented Feb 27, 2018

Would definitely be good to have a unit test for this

@trxcllnt
Copy link
Contributor Author

@wesm @TheNeuralBit yeah, I wanted to get a fix in first. I encountered this trying to read values from a DictionaryVector whose indices have a nullCount > 0. I think this was just a typo/bad merge when I merged my branch with Brian's a few weeks ago. I would imagine the Dictionary integration tests could cover this case?

@trxcllnt
Copy link
Contributor Author

trxcllnt commented Feb 27, 2018

@wesm I might've asked this before, but is it possible (via the spec) for the indices itself to be a DictionaryVector (e.g. for mapping between an original to sorted set of indices)? We don't guard against doing it on-the-fly, but if we want to support it, we should probably go through the public indicies.nullCount instead of using the private _nullCount here.

@wesm
Copy link
Member

wesm commented Feb 27, 2018

The indices are supposed to be a signed integer type

@TheNeuralBit
Copy link
Member

I already started adding some basic unit tests for DictionaryVector, so I'll look at adding a test that covers the indices nullCount > 0 case and maybe PR them into your branch @trxcllnt

@TheNeuralBit
Copy link
Member

Put up a PR with some tests: trxcllnt#5
I think I may have found another bug? Slicing a DictionaryVector with nullCount > 0 is throwing an error

@TheNeuralBit TheNeuralBit changed the title ARROW-2226: [JS] use indices.offset in DictionaryData constructor ARROW-2226, ARROW-2233: [JS] DictionaryData bugfixes Feb 28, 2018
@wesm
Copy link
Member

wesm commented Feb 28, 2018

@TheNeuralBit you should be able to push directly to this branch, too, if that's easier

@TheNeuralBit
Copy link
Member

Oh nice, I'll do that in the future, thanks @wesm

@trxcllnt
Copy link
Contributor Author

@TheNeuralBit lgtm thanks! 👍

@TheNeuralBit TheNeuralBit changed the title ARROW-2226, ARROW-2233: [JS] DictionaryData bugfixes ARROW-2226, ARROW-2233: [JS] Dictionary bugfixes Feb 28, 2018
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.

3 participants