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

Generalize query condition parsing to non-INT32 index columns #614

Merged
merged 3 commits into from Nov 6, 2023

Conversation

eddelbuettel
Copy link
Member

This PR generalized support for parsing of query condition from user supplied text to arrays not created from R factors and using indices with types different from int32_t.

Copy link

This pull request has been linked to Shortcut Story #36509: [r] Fix query conditions on enumerated columns.

Copy link
Contributor

@johnkerl johnkerl 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 the bugfix!

Please add unit-test coverage

@eddelbuettel
Copy link
Member Author

@johnkerl Please supply the unit test. The package has no 'from-arrow' writer.

@eddelbuettel
Copy link
Member Author

Thinko on my part. Can likely do it 'the long way' wrapping the core API. Will attempt later.

Copy link
Member

@aaronwolen aaronwolen left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@johnkerl johnkerl left a comment

Choose a reason for hiding this comment

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

🚢
Thank you!

@eddelbuettel eddelbuettel merged commit c0bb18a into master Nov 6, 2023
1 check passed
@eddelbuettel eddelbuettel deleted the de/sc-36509/qc_on_enum_with_non_int8_indices branch November 6, 2023 21:17
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

4 participants