[c++] Integrate `SOMAColumn`: Arrow adapter methods, part 1 #3405

XanthosXanthopoulos · 2024-12-06T15:21:47Z

This PR replaces the Arrow schema to TileDB schema transformation to use the SOMAColumn create methods.
Also there are a set of new data converters from arrow arrays to std::array for simplification.

This migration also enforces a current domain restriction for string dimensions to libtiledbsoma in addition to the restriction being present only on the R and Python APIs.

nguyenv

Can you explain what the Skip and Take are for and document it? It looks like Take is the index of the column to retrieve and Skip is relevant only for geometry columns (where it's always 2)?

Also is there a way to use std::variant or a templated type instead of std::any or would that make things too complicated?

nguyenv · 2024-12-09T19:04:33Z

libtiledbsoma/src/utils/arrow_adapter.h

+    /**
+     * Return a copy of the data in a specified column of an arrow table.
+     * Complex column types are supported. The for each sub column are an
+     * std::array<T, 2> casted as an std::any object.
+     */


Suggested change

/**

* Return a copy of the data in a specified column of an arrow table.

* Complex column types are supported. The for each sub column are an

* std::array<T, 2> casted as an std::any object.

*/

/**

* Return a copy of the data in a specified column of an arrow table.

* Complex column types are supported. The type for each sub column is

* an std::array<T, 2> casted as an std::any object.

*/

Skip and Take are used in 2 places with 2 specific sets on values (either Skip=3 and Take=2 or Skip=0 and Take=2) and are independent of the geometry column. Their usage is to extract specific subranges of ArrowArray data and they come in handy during ArrowSchema -> TileDBSchema where the arrow array provided has 5 values per dimension and we only need the last 2 to set the current domain.

As to using std::variant, adding more SOMAColumn types would require changing multiple variants. The use of std::any here is to enable runtime polymorphism and indirectly introduces a runtime type check (via any_cast, make_any) between the templated function and the actual dimension type. std::variant can provide all the above it is just a different style I am open to discuss further.

@XanthosXanthopoulos to be clear, I'm not asking you to use a different solution. I'm asking to be clear to the reader the (very good!) reasons you chose the solution you're using.

Please take both paragraphs you just wrote, and put them in the code as comments as well. They are highly informative. :)

Yeah sorry if I was unclear too. This explaination is good, and I was just looking for it to also be documented in the code.

codecov · 2025-01-14T14:22:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.32%. Comparing base (7616147) to head (01c3608).
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3405      +/-   ##
==========================================
+ Coverage   86.22%   86.32%   +0.09%     
==========================================
  Files          55       55              
  Lines        6410     6375      -35     
==========================================
- Hits         5527     5503      -24     
+ Misses        883      872      -11

Flag	Coverage Δ
python	`86.32% <ø> (+0.09%)`	⬆️

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

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

…nt domain checks, replace vector with span when selecting points

libtiledbsoma/src/utils/arrow_adapter.h

jp-dark

I didn't review the get_table_column_by_name function, but everything else looks good to me. However, someone with more familiarity with the arrow adapter code should look over this as well.

johnkerl

Thanks for working on this @XanthosXanthopoulos !

libtiledbsoma/src/utils/arrow_adapter.cc

johnkerl · 2025-01-15T20:32:01Z

libtiledbsoma/src/utils/arrow_adapter.cc

+                index_column_schema->children[i]->name));
+        }
+
+        if ((*column)->tiledb_dimensions().has_value()) {


This is a bit awkward.

column needs to be column_it or some such -- it is not a column, it is an iterator

Then, const auto column = *column_it after you check that column _it != columns.end()

Then the rest of these (*column)->foo become column->foo as they should be

libtiledbsoma/src/utils/arrow_adapter.cc

libtiledbsoma/src/utils/arrow_adapter.h

johnkerl · 2025-01-17T18:42:25Z

libtiledbsoma/src/utils/arrow_adapter.h

-        std::string name);
+    /**
+     * Return a copy of the data in a specified column of an arrow table.
+     * Complex column types are supported. The type for each sub column is an


Suggested change

* Complex column types are supported. The type for each sub column is an

* Complex column types are supported. The type for each subcolumn is an

johnkerl · 2025-01-17T18:43:21Z

libtiledbsoma/src/utils/arrow_adapter.h

+    }
+
+    /**
+     * Read a part of am Arrow array to an std::array and cast in to and


Suggested change

* Read a part of am Arrow array to an std::array and cast in to and

* Read a part of an Arrow array to an std::array and cast in to an

johnkerl · 2025-01-17T18:43:50Z

libtiledbsoma/src/utils/arrow_adapter.h

+     *
+     * @example get_table_any_column<3, 2>(array, schema) will return an
+     * std::array<T, 3> where T is the appropriate type based on the Arrow array
+     * format casted as an std::any object. The std::array will skip 2 element


Suggested change

* format casted as an std::any object. The std::array will skip 2 element

* format casted as an std::any object. The std::array will skip 2 elements

johnkerl · 2025-01-17T18:51:11Z

libtiledbsoma/test/unit_soma_column.cc

+        REQUIRE(str_range[0] < " ");
+        REQUIRE(str_external.first <= " ");
+        REQUIRE(str_range[1] > "~");
+        // REQUIRE(str_external.second >= "~");


Please remove

johnkerl · 2025-01-17T18:51:30Z

libtiledbsoma/src/utils/util.cc

@@ -98,4 +98,22 @@ std::vector<uint8_t> cast_bit_to_uint8(ArrowSchema* schema, ArrowArray* array) {
    return casted;
 }

+std::shared_ptr<SOMAColumn> find_column_by_name(


johnkerl · 2025-01-17T18:56:00Z

libtiledbsoma/src/utils/arrow_adapter.h

+     * @tparam Take The number of elements to read.
+     * @tparam Skip The number of elements to skip.


I was wondering why Take and Skip are template parameters and not simply function arguments. (The latter seems simpler.)

After having read through your code here, the answer seems to be: because you are constructing std::array objects (which are fixed-size) in various places, rather than std::vector (which are not fixed-size).

It's a good reason to have Take and Skip be implemented as template parameters, but I found it non-obvious.

Please come out and state this reasoning, right here, as a comment.

jp-dark

LGTM once the remaining change requests are addressed.

XanthosXanthopoulos requested review from nguyenv and johnkerl December 8, 2024 16:25

XanthosXanthopoulos changed the title ~~Integrate SOMAColumn in Arrow adapter methods [WIP]~~ Integrate SOMAColumn in Arrow adapter methods Part 2 Dec 8, 2024

XanthosXanthopoulos changed the title ~~Integrate SOMAColumn in Arrow adapter methods Part 2~~ [c++] Integrate SOMAColumn in Arrow adapter methods Part 2 Dec 8, 2024

XanthosXanthopoulos marked this pull request as ready for review December 8, 2024 16:25

XanthosXanthopoulos linked an issue Dec 9, 2024 that may be closed by this pull request

[c++] Add an abstraction layer between SOMA columns and TileDB dimensions and attributes #3337

Open

johnkerl changed the title ~~[c++] Integrate SOMAColumn in Arrow adapter methods Part 2~~ [c++] Integrate SOMAColumn in Arrow adapter methods, part 2 Dec 9, 2024

nguyenv reviewed Dec 9, 2024

View reviewed changes

XanthosXanthopoulos force-pushed the xan/sc-59427/soma-column branch from 804c87e to a58361a Compare December 11, 2024 11:38

XanthosXanthopoulos force-pushed the xan/sc-59427/soma-column-arrow-integration branch 2 times, most recently from 5485141 to 0e69ed7 Compare December 13, 2024 16:07

XanthosXanthopoulos changed the base branch from xan/sc-59427/soma-column to xan/sc-59427/soma-geometry-column December 13, 2024 16:08

XanthosXanthopoulos changed the title ~~[c++] Integrate SOMAColumn in Arrow adapter methods, part 2~~ [c++] Integrate SOMAColumn: Arrow adapter methods, part 1 Dec 13, 2024

XanthosXanthopoulos force-pushed the xan/sc-59427/soma-column-arrow-integration branch from 0e69ed7 to d6d6187 Compare December 13, 2024 16:10

XanthosXanthopoulos force-pushed the xan/sc-59427/soma-geometry-column branch 4 times, most recently from 8daf17e to a426c7a Compare January 8, 2025 19:04

Base automatically changed from xan/sc-59427/soma-geometry-column to main January 8, 2025 19:38

XanthosXanthopoulos marked this pull request as draft January 14, 2025 14:04

XanthosXanthopoulos force-pushed the xan/sc-59427/soma-column-arrow-integration branch from d6d6187 to af1f010 Compare January 14, 2025 14:05

XanthosXanthopoulos force-pushed the xan/sc-59427/soma-column-arrow-integration branch from af1f010 to 2401416 Compare January 14, 2025 18:38

XanthosXanthopoulos requested a review from jp-dark January 14, 2025 18:39

XanthosXanthopoulos marked this pull request as ready for review January 14, 2025 18:39

XanthosXanthopoulos added 5 commits January 15, 2025 14:01

SOMAColumn abstract class definition

2022f23

Remove fmt::format

81007d0

Remove unneeded methods and member variables

457d5d4

Add concrete class wrapper for TileDB dimension

d6d91a3

Add minimal testing for dimensions

1cf44f1

XanthosXanthopoulos added 17 commits January 15, 2025 14:01

Replace string_view with string when returning column name, add curre…

e7265fe

…nt domain checks, replace vector with span when selecting points

Add concrete class wrapper for TileDB attribute

1baf795

Remove current_domain flag

df9fcab

Replace string_view with string when returning column name, add curre…

775e756

…nt domain checks, replace vector with span when selecting points

Update CMake files

3511a32

Add minimal testing for dimensions

264512a

Misc fixes

34ed856

Add read test case

bebaddd

Remove current_domain flag

176b576

Do not export soma column [skip ci]

d06bfb7

Replace string_view with string when returning column name, add curre…

b8e8292

…nt domain checks, replace vector with span when selecting points

Add function to extract data from ArrowTable into std::array

bd4edb4

Migrate array creation to SOMAColumn

ab46848

Fix string current domain in unit dataframe tests

6e0f7d1

Fix current domain unit test on string dimension

ba58538

Remove unused methods

1ceef07

Misc fixes

77a01e1

XanthosXanthopoulos force-pushed the xan/sc-59427/soma-column-arrow-integration branch from 2401416 to 77a01e1 Compare January 15, 2025 12:02

jp-dark requested a review from nguyenv January 15, 2025 14:51

jp-dark reviewed Jan 15, 2025

View reviewed changes

libtiledbsoma/src/utils/arrow_adapter.h Outdated Show resolved Hide resolved

jp-dark reviewed Jan 15, 2025

View reviewed changes

johnkerl requested changes Jan 15, 2025

View reviewed changes

Address review comments

01c3608

XanthosXanthopoulos requested review from jp-dark and johnkerl January 17, 2025 14:15

johnkerl requested changes Jan 17, 2025

View reviewed changes

jp-dark approved these changes Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[c++] Integrate `SOMAColumn`: Arrow adapter methods, part 1 #3405

[c++] Integrate `SOMAColumn`: Arrow adapter methods, part 1 #3405

XanthosXanthopoulos commented Dec 6, 2024 •

edited

Loading

nguyenv left a comment

nguyenv Dec 9, 2024

XanthosXanthopoulos Dec 10, 2024

johnkerl Jan 17, 2025

nguyenv Jan 17, 2025 •

edited

Loading

codecov bot commented Jan 14, 2025 •

edited

Loading

jp-dark left a comment

johnkerl left a comment

johnkerl Jan 15, 2025 •

edited

Loading

johnkerl Jan 17, 2025

johnkerl Jan 17, 2025

johnkerl Jan 17, 2025

johnkerl Jan 17, 2025

johnkerl Jan 17, 2025

johnkerl Jan 17, 2025

jp-dark left a comment

	* Complex column types are supported. The type for each sub column is an
	* Complex column types are supported. The type for each subcolumn is an

	* Read a part of am Arrow array to an std::array and cast in to and
	* Read a part of an Arrow array to an std::array and cast in to an

	* format casted as an std::any object. The std::array will skip 2 element
	* format casted as an std::any object. The std::array will skip 2 elements

		* @tparam Take The number of elements to read.
		* @tparam Skip The number of elements to skip.

[c++] Integrate SOMAColumn: Arrow adapter methods, part 1 #3405

Are you sure you want to change the base?

[c++] Integrate SOMAColumn: Arrow adapter methods, part 1 #3405

Conversation

XanthosXanthopoulos commented Dec 6, 2024 • edited Loading

nguyenv 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

nguyenv Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jan 14, 2025 • edited Loading

Codecov Report

jp-dark left a comment

Choose a reason for hiding this comment

johnkerl left a comment

Choose a reason for hiding this comment

johnkerl Jan 15, 2025 • 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

jp-dark left a comment

Choose a reason for hiding this comment

[c++] Integrate `SOMAColumn`: Arrow adapter methods, part 1 #3405

[c++] Integrate `SOMAColumn`: Arrow adapter methods, part 1 #3405

XanthosXanthopoulos commented Dec 6, 2024 •

edited

Loading

nguyenv Jan 17, 2025 •

edited

Loading

codecov bot commented Jan 14, 2025 •

edited

Loading

johnkerl Jan 15, 2025 •

edited

Loading