[c++] Geometry Dataframe #3212

XanthosXanthopoulos · 2024-10-21T15:30:58Z

This PR adds the new SOMAGeometryDataFrame object. This new object requires spatial axes to be determined and uses them internally to index geometry objects stored as a WKB encoded attribute. To differentiate between binary attributes and WKB binary attributes the Arrow schema requires a dtype metadata key with the value WKB.

codecov · 2024-10-21T15:46:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.97%. Comparing base (99ba3a3) to head (ead7104).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3212      +/-   ##
==========================================
+ Coverage   83.86%   83.97%   +0.10%     
==========================================
  Files          51       51              
  Lines        5505     5505              
==========================================
+ Hits         4617     4623       +6     
+ Misses        888      882       -6

Flag	Coverage Δ
python	`83.97% <ø> (+0.10%)`	⬆️

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

Components	Coverage Δ
python_api	`83.97% <ø> (+0.10%)`	⬆️
libtiledbsoma	`∅ <ø> (∅)`

jp-dark

That parts of this I understand look good to me, but I'm not familiar enough with the arrow adapter code to have a good sense of what impact the change might have there.

johnkerl · 2024-10-23T16:28:51Z

libtiledbsoma/src/utils/arrow_adapter.cc

+        if (type_metadata.compare("WKB") == 0) {
+            type = TILEDB_GEOM_WKB;
+        } else {
+            throw std::runtime_error(


please use fmt::format with {} as elsewhere in this file

johnkerl · 2024-10-23T16:29:39Z

libtiledbsoma/src/utils/arrow_adapter.cc

+    return {attr, enmr};
+}
+
+std::pair<Dimension, bool> ArrowAdapter::tiledb_dimension_from_arrow_schema(


i'd prefer you move the body of this dimension function above the body of the corresponding attribute funtion

johnkerl · 2024-10-23T16:30:22Z

libtiledbsoma/src/utils/arrow_adapter.h

+     * @return std::pair<Dimension, bool> The TileDB dimension with a boolean
+     * flag indicating whether or not the dimension uses `current domain`.
+     */
+    static std::pair<Dimension, bool> tiledb_dimension_from_arrow_schema(


i'd prefer you move the declaration of this dimension function above the corresponding attribute function

johnkerl · 2024-10-23T16:32:19Z

libtiledbsoma/src/soma/soma_geometry_dataframe.h

+    static void create(
+        std::string_view uri,
+        std::unique_ptr<ArrowSchema> schema,
+        ArrowTable index_columns,


i see no reason not to use const references here

Agrre on const reference here but will require refactoring all other classes which tiledb_schema_from_arrow_schema. Should be done to a follow-up PR maybe?

it doesn't need to be done everywhere -- it can be done in places you add

we already have const references in some places not others -- you can have this PR be part of the work that doesn't need to be redone later

it can also be done all at once in a follow-on, of course

johnkerl · 2024-10-23T16:33:01Z

libtiledbsoma/src/soma/soma_geometry_dataframe.h

+     * @param mode read or write
+     * @param ctx SOMAContext
+     * @param column_names A list of column names to use as user-defined index
+     * columns (e.g., ``['cell_type', 'tissue_type']``). All named columns must


are these example column names legit for geometry dataframes?

johnkerl · 2024-10-23T16:35:14Z

libtiledbsoma/src/soma/soma_geometry_dataframe.cc

+        ctx->tiledb_ctx(),
+        std::move(schema),
+        ArrowTable(
+            std::move(index_columns.first), std::move(index_columns.second)),


i believe you can avoid this by having this constructor take the ArrowTable parameters as const references

libtiledbsoma/test/unit_soma_geometry_dataframe.cc

johnkerl · 2024-10-23T16:37:41Z

libtiledbsoma/test/unit_soma_geometry_dataframe.cc

+        SOMAGeometryDataFrame::create(
+            uri,
+            std::move(schema),
+            ArrowTable(


please use const references to avoid the moves

johnkerl · 2024-10-23T16:38:33Z

libtiledbsoma/test/unit_soma_geometry_dataframe.cc

+        REQUIRE(soma_object->type() == "SOMAGeometryDataFrame");
+        soma_object->close();
+    }
+}


seems like we could use some tests which write & read data -- is this planned for a future PR?

Yes, there is #3222 which adds tests for read/write.

nguyenv · 2024-10-23T18:01:36Z

libtiledbsoma/src/utils/arrow_adapter.cc

+                    if (type_metadata.compare("WKB") != 0) {
+                        throw std::runtime_error(
+                            std::string(
+                                "Unkwown type metadata for `soma_geometry`. "
+                                "Expected 'WKB', found ") +
+                            std::string(type_metadata.data()));
+                    }

-                FilterList filter_list = ArrowAdapter::_create_dim_filter_list(
-                    child->name, platform_config, soma_type, ctx);
+                    for (int64_t j = 0;
+                         j < spatial_column_info.second->n_children;
+                         ++j) {
+                        auto min_dim = tiledb_dimension_from_arrow_schema(
+                            ctx,
+                            spatial_column_info.second->children[j],
+                            spatial_column_info.first->children[j],
+                            soma_type,
+                            type_metadata,
+                            "tiledb__internal__",
+                            "__min",
+                            platform_config);
+
+                        auto max_dim = tiledb_dimension_from_arrow_schema(
+                            ctx,
+                            spatial_column_info.second->children[j],
+                            spatial_column_info.first->children[j],
+                            soma_type,
+                            type_metadata,
+                            "tiledb__internal__",
+                            "__max",
+                            platform_config);
+
+                        dims.insert({min_dim.first.name(), min_dim.first});
+                        dims.insert({max_dim.first.name(), max_dim.first});
+
+                        use_current_domain &= min_dim.second;
+                        use_current_domain &= max_dim.second;


For readability, I think this can be pulled out into a separate _set_spatial_dimension (or better name) helper function.

nguyenv · 2024-10-23T18:05:59Z

libtiledbsoma/src/utils/arrow_adapter.cc

-                }
+            if (strcmp(child->name, index_column_schema->children[i]->name) ==
+                0) {
+                if (strcmp(child->name, "soma_geometry") == 0 &&


Maybe we should const std::string some of these in utils/common.h like "soma_geometry", "tiledb__internal__", "__min", and "__max"

…en creating schema

XanthosXanthopoulos requested review from nguyenv, jp-dark and johnkerl and removed request for nguyenv and jp-dark October 21, 2024 21:11

XanthosXanthopoulos marked this pull request as ready for review October 21, 2024 21:12

jp-dark reviewed Oct 23, 2024

View reviewed changes

johnkerl requested changes Oct 23, 2024

View reviewed changes

nguyenv reviewed Oct 23, 2024

View reviewed changes

XanthosXanthopoulos added 2 commits October 24, 2024 15:03

Geometry dataframe implementation, use spatial axes when available wh…

fa66ba2

…en creating schema

Address PR review comments

ead7104

XanthosXanthopoulos force-pushed the xan/geometry_dataframe_cpp branch from 02bfa69 to ead7104 Compare October 24, 2024 12:03

XanthosXanthopoulos requested review from johnkerl and nguyenv October 24, 2024 12:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[c++] Geometry Dataframe #3212

[c++] Geometry Dataframe #3212

XanthosXanthopoulos commented Oct 21, 2024

codecov bot commented Oct 21, 2024 •

edited

Loading

jp-dark left a comment

johnkerl Oct 23, 2024

johnkerl Oct 23, 2024

johnkerl Oct 23, 2024

johnkerl Oct 23, 2024

XanthosXanthopoulos Oct 23, 2024

johnkerl Oct 24, 2024 •

edited

Loading

johnkerl Oct 23, 2024

johnkerl Oct 23, 2024

johnkerl Oct 23, 2024

johnkerl Oct 23, 2024

XanthosXanthopoulos Oct 23, 2024

nguyenv Oct 23, 2024

nguyenv Oct 23, 2024

[c++] Geometry Dataframe #3212

Are you sure you want to change the base?

[c++] Geometry Dataframe #3212

Conversation

XanthosXanthopoulos commented Oct 21, 2024

codecov bot commented Oct 21, 2024 • edited Loading

Codecov Report

jp-dark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnkerl Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 21, 2024 •

edited

Loading

johnkerl Oct 24, 2024 •

edited

Loading