[c++] SOMAColumn [WIP] #3329
[c++] SOMAColumn [WIP] #3329
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3329 +/- ##
==========================================
+ Coverage 85.79% 85.89% +0.09%
==========================================
Files 55 55
Lines 6176 6176
==========================================
+ Hits 5299 5305 +6
+ Misses 877 871 -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.
These don't match the function template parameter/parameter names.
There was a problem hiding this comment.
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.
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.
"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.
| * @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.
Should we take in a tiledb::Context or SOMAContext?
XanthosXanthopoulos
force-pushed
the
xan/sc-59427/soma-column
branch
2 times, most recently
from
01d39fc
to
792dc96
Compare
XanthosXanthopoulos
force-pushed
the
xan/sc-59427/soma-column
branch
8 times, most recently
from
7de2ced
to
5c6c484
Compare
XanthosXanthopoulos
force-pushed
the
xan/sc-59427/soma-column
branch
from
5c6c484
to
549a5fe
Compare
SOMAColumnprovides an abstraction over TileDB attributes and dimensions and exposes a common interface for all columns regardless of type. Subclasses ofSOMAColumncan 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.