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

[c++] Addition of ArrowSchema to TileDB ArraySchema Converter #2418

Merged
merged 10 commits into from
Apr 17, 2024

Conversation

nguyenv
Copy link
Member

@nguyenv nguyenv commented Apr 9, 2024

Issue and/or context: #2228

Part of larger branch #2367

Changes:

  • Addition of tiledb_schema_from_arrow_schema converter that takes in an ArrowSchema with ColumnIndexInfo, which contains info about the dimension's extent and tiles, and PlatformConfig, which contains info for other TileDB ArraySchema settings such as filters, etc.
  • All SOMAObject create functions now take in ArrowSchema, ColumnIndexInfo, and PlatformConfig arguments; return signature is void to avoid opening the TileDB object until necessary
  • Add unit test helper functions in common.cc and common.h

Notes for Reviewer:

@nguyenv nguyenv changed the title [c++] Addition of ArrowSchema to TileDB ArraySchema Converter [c++] Addition of ArrowSchema to TileDB ArraySchema Converter Apr 9, 2024
@eddelbuettel
Copy link
Contributor

Very timely! Just what I may need for tiledb-r too ✨ If you can maybe try to exclude nanoarrow.h from the enforced linting as it only gets us fairly pointless whitespace diffs to an already vendored external file. Working with @dudoslav to see if we can make it a more formal external project to avoid those woes.

Copy link

codecov bot commented Apr 9, 2024

Codecov Report

Merging #2418 (ae8dac4) into main (4f6b9ae) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2418   +/-   ##
=======================================
  Coverage   90.43%   90.43%           
=======================================
  Files          37       37           
  Lines        3940     3940           
=======================================
  Hits         3563     3563           
  Misses        377      377           
Flag Coverage Δ
python 90.43% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
python_api 90.43% <ø> (ø)
libtiledbsoma ∅ <ø> (∅)

@nguyenv nguyenv marked this pull request as ready for review April 9, 2024 19:22
Copy link
Contributor

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

This looks really good now, and easier for me to use than in an earlier form. I just spotted some small documentation entries. Worth another pass before merging?

@nguyenv nguyenv force-pushed the viviannguyen/arrow-schema-to-tiledb-schema-cpp branch from 8184070 to 2ab5c8e Compare April 17, 2024 17:05
@nguyenv
Copy link
Member Author

nguyenv commented Apr 17, 2024

@eddelbuettel I have taken a closer scan of all the SOMAArray and derived classes' docstrings and made necessarily corrections.

Copy link
Contributor

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

Nice work! I think this is ready to move along and have its tires kicked from my side too!

@nguyenv nguyenv merged commit 8c07c6f into main Apr 17, 2024
15 checks passed
@nguyenv nguyenv deleted the viviannguyen/arrow-schema-to-tiledb-schema-cpp branch April 17, 2024 18:19
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