Skip to content

Extend query_object for var char columns#161

Merged
eddelbuettel merged 4 commits intomasterfrom
de/query_var_char
Aug 26, 2020
Merged

Extend query_object for var char columns#161
eddelbuettel merged 4 commits intomasterfrom
de/query_var_char

Conversation

@eddelbuettel
Copy link
Contributor

This PR adds some more polish to the recently added tiledb_query S4 object letting it also operate on variable length character columns -- which in turn makes a number of the examples more concise and somewhat more natural for R users.

@eddelbuettel eddelbuettel changed the title De/query var char Extend query_object for var char columns Aug 25, 2020
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.

Having a look now.

I noticed a few different argument names are used for size offsets and size data:

  • sizeoffsets, szoff, szoffset
  • sizedata, szdat

@eddelbuettel
Copy link
Contributor Author

Thanks. They should all be internal but easy to rename.

The key point of the PR is that a variable

R> var <- c("The", "quick", "brown", "fox")
R> var
[1] "The"   "quick" "brown" "fox"  
R>

can now be used rather than a collated single string and offsets vector which is a little more foreign to R users (and still used in the unit tests etc).

Copy link
Member

@Shelnutt2 Shelnutt2 left a comment

Choose a reason for hiding this comment

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

Very nice usability improvement

@eddelbuettel
Copy link
Contributor Author

Thanks @seth. There may be more to do still on that front but we "got to flip those stones one at a time" so to speak.

I'll look into the suggestion by @aaronwolen and standardize the variable names some more and merge then.

@aaronwolen
Copy link
Member

Everything worked for me. This is indeed a nice convenience R users will appreciate!

@eddelbuettel
Copy link
Contributor Author

@aaronwolen If you have a second glance at the commit 2942e16 I just added which should clean things up a bit more.

@aaronwolen
Copy link
Member

LGTM—I like the longer arg names 👍

@eddelbuettel eddelbuettel merged commit fefd2ca into master Aug 26, 2020
@eddelbuettel eddelbuettel deleted the de/query_var_char branch August 26, 2020 14:18
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