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

Support shared memory reads #316

Merged
merged 24 commits into from
Nov 11, 2021
Merged

Support shared memory reads #316

merged 24 commits into from
Nov 11, 2021

Conversation

eddelbuettel
Copy link
Contributor

@eddelbuettel eddelbuettel commented Nov 4, 2021

This PR enables accessing shared memory buffers as e.g. provided by the TileDB Cloud backend. This is not of general use for the TileDB R package which will usually call TileDB Embedded in process but supports a usage pattern, and performance enhancement, in our deployment for TileDB Cloud.

The 'list of buffers' proposal is working fine as we can use simple regular expressions thanks to C++17 to go from (required) data buffers to (optional) offsets and validity_map buffers.

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #11819: support shmem buffer reads (as used by the REST server).

@johnkerl
Copy link
Contributor

johnkerl commented Nov 5, 2021

Thanks @eddelbuettel ! I'll pull this locally & build & validate with a local REST-server checkout.

@eddelbuettel eddelbuettel marked this pull request as ready for review November 8, 2021 15:34
@eddelbuettel
Copy link
Contributor Author

I removed the 'draft' label. The earlier reds in CI can be ignored as they come from macOS and Windows which are, effectively, not used in this code -- and earlier changes simply didn't condition as cleanly on Linux-only.

R/TileDBArray.R Outdated Show resolved Hide resolved
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 @eddelbuettel -- this looks great!

Copy link
Member

@ihnorton ihnorton left a comment

Choose a reason for hiding this comment

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

🎉

@@ -158,6 +156,46 @@ tiledb_datatype_t _string_to_tiledb_datatype(std::string typestr) {
}
}

int32_t _tiledb_datatype_to_sizeof(tiledb_datatype_t dtype) {
Copy link
Member

Choose a reason for hiding this comment

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

FYI, this is available in the core C API as tiledb_datatype_size - https://github.com/TileDB-Inc/TileDB/blob/a7e1d7cde0fe07c1dbf8fb18243e91cf3b83fcbf/tiledb/sm/c_api/tiledb.cc#L314

(technically not exposed in the C++ API as far as I can tell, but still usable)

Copy link
Member

Choose a reason for hiding this comment

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

No need to change this now, but just for future reference/refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯

I'll switch to that,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 95fcb52

@ihnorton
Copy link
Member

Thanks @eddelbuettel!

@eddelbuettel eddelbuettel merged commit d3e424a into master Nov 11, 2021
@eddelbuettel eddelbuettel deleted the de/sc-11819/shmem_reads branch November 11, 2021 19:00
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