-
Notifications
You must be signed in to change notification settings - Fork 25
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++] SOMAColumn
part 1
#3329
[c++] SOMAColumn
part 1
#3329
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3329 +/- ##
==========================================
+ Coverage 85.99% 86.09% +0.09%
==========================================
Files 55 55
Lines 6221 6221
==========================================
+ Hits 5350 5356 +6
+ Misses 871 865 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
libtiledbsoma/src/soma/soma_column.h
Outdated
* @tparam T | ||
* @param dim | ||
* @param ranges |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These don't match the function template parameter/parameter names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like interesting work but it's not clear why it's being done.
You need a GitHub issue, or Shortcut link, in the description field.
From your GitHub branch name I can discover a Shortcut story number -- since this is an open-source project, please include within a GitHub issue the context that is necessary for anyone who is not a TileDB employee to understand what changes you're making here, and why.
This PR introduces new code without any unit tests, and no callsite from existing code that would exercise existing unit tests. It could be stamped now with guarantee of unit-test cases to follow, but, I'd be happier if you could offer at least a minimal set of tests.
libtiledbsoma/src/soma/soma_column.h
Outdated
* | ||
* @section DESCRIPTION | ||
* | ||
* This file defines the SOMAColumn class. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please say what this class is for, what problem it solves. This comment, as is, doesn't tell anything that class SOMAColuimn { ... }
doesn't.
In particular, up until there's been a 1-1 mapping of one soma column -> one tiledb dim or one tiledb attr. Here it appears (I'm still reading but it appears so far) that you are allowing for one-to-many mappings of one soma column -> more than one tiledb dim and/or more than one tiledb attr. A top-of-file comment here would need to explain this in at least several sentences' worth of detail.
|
||
/** | ||
* @brief Select columns names to query (dim and attr). If the | ||
* `if_not_empty` parameter is `true`, the column will be selected iff the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"iff" is specialist mathematical jargon that non-mathematicians are unlikely to recognize. Please don't use it.
* from changing an empty list (all columns) to a subset of columns. | ||
* | ||
* @param query the ManagedQuery object to modify | ||
* @param if_not_empty Prevent changing an "empty" selection of all columns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param if_not_empty Prevent changing an "empty" selection of all columns | |
* @param if_not_empty Prevent changing an empty selection of all columns |
libtiledbsoma/src/soma/soma_column.h
Outdated
* @param ctx TileDB context | ||
*/ | ||
SOMAColumn(const Context& ctx) | ||
: ctx(ctx){}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we take in a tiledb::Context
or SOMAContext
?
01d39fc
to
792dc96
Compare
5c6c484
to
549a5fe
Compare
549a5fe
to
804c87e
Compare
…nt domain checks, replace vector with span when selecting points
804c87e
to
a58361a
Compare
SOMAColumn
provides an abstraction over TileDB attributes and dimensions and exposes a common interface for all columns regardless of type. Subclasses ofSOMAColumn
can implement complex indexing mechanism through additional dimensions and encapsulate all that logic in one place and make it modular.Subsequent PRs will add implementation for dimension and attributes.
Throughout this PR there is extensive use of
std::any
to enable polymorphism with the differentSOMAColumn
types while maintaining a templated interface at the abstractSOMAColumn
.Currently there are 3 types of
SOMAColumn
:SOMAAttribute
wraps a TileDB Attribute and an optional enumerationSOMADimension
wraps a TileDB Dimension and provides methods to access and manipulate the current domain and set the extend for queriesSOMAGeometryColumn
wraps a set of TileDB Dimensions as well as an Attribute and provides methods to access and manipulate the current domain and set the extend for queries