Add support for getting a dtype of ndrectangle. by robertbindar · Pull Request #5229 · TileDB-Inc/TileDB

robertbindar · 2024-08-06T13:45:23Z

Users wrapping the current domain APIs are requesting:
tiledb_datatype_t NDRectangle::range_dtype(const std::string& dim_name) and
tiledb_datatype_t NDRectangle::range_dtype(uint32_t dim_idx) (and the CAPI counterparts)
so that the underlying TileDB datatype of a range/dimension can be queried closer to the
call site of functions like e.g. range where a conversion from tiledb_datatype_t to static c++ type is most of the time needed.

[sc-52012]

TYPE: FEATURE
DESC: Add support for getting a dtype of ndrectangle.

eddelbuettel · 2024-08-06T14:16:58Z

+   * @param dim_idx The dimension index.
+   * @return The datatype of the range.
+   */
+  tiledb_datatype_t range_dtype(unsigned dim_idx) {


There are other accessors that double up everything in terms of dimension index and name. Could we have an accessor that uses name too as I wrote the rest of my implementation in terms of the dimension name. (Yes, I know, I could wrap this but while we are at making this complete may as well make it fully complete.)

johnkerl

LGTM, and I second @eddelbuettel 's request for a by-name accessor (either on this PR, or a subsequent PR, makes no difference to me)

johnkerl

As bonus it wouldn't hurt to drop examples into

examples/c_api/current_domain.c
examples/cpp_api/current_domain.cc

such as (for example in the C++ case)

diff --git a/examples/cpp_api/current_domain.cc b/examples/cpp_api/current_domain.cc
index 0ba8f3373..a1626478b 100644
--- a/examples/cpp_api/current_domain.cc
+++ b/examples/cpp_api/current_domain.cc
@@ -107,6 +107,15 @@ void print_current_domain(Context& ctx) {
   // Get the current domain's NDRectangle
   NDRectangle ndrect = current_domain.ndrectangle();

+  tiledb_datatype_t dtype = ndrect.range_dtype(0);
+  const char* name;
+  auto st = tiledb_datatype_to_str(dtype, &name);
+  if (st == TILEDB_OK) {
+    std::cout << "Current domain dtype: " << name << std::endl;
+  } else {
+    std::cout << "Could not retrieve dtype name for" << dtype << std::endl;
+  }
+
   // Get the range of the rectangle's first dimension
   std::array<int32_t, 2> range = ndrect.range<int32_t>("d1");

eddelbuettel · 2024-08-06T14:44:06Z

+   * @param dim_name The dimension name.
+   * @return The datatype of the range.
+   */
+  tiledb_datatype_t range_dtype_from_name(const std::string& dim_name) {


eddelbuettel

Thanks!

teo-tsirpanis

This is very likely sufficient for the C# API, thanks!

The term dtype is nowhere else being used in the C or C++ API, we should better call it just get_type for consistency.

eddelbuettel · 2024-08-06T21:42:47Z

I think datatype is used in a few places and tiledb_datatype_t also cements it a little.

teo-tsirpanis · 2024-08-06T21:51:58Z

Indeed, but function names use type.

TileDB/tiledb/sm/cpp_api/attribute.h

Lines 151 to 158 in 523f84f

    
           /** Returns the attribute datatype. */ 
        
           tiledb_datatype_t type() const { 
        
             auto& ctx = ctx_.get(); 
        
             tiledb_datatype_t type; 
        
             ctx.handle_error( 
        
                 tiledb_attribute_get_type(ctx.ptr().get(), attr_.get(), &type)); 
        
             return type; 
        
           }

TileDB/tiledb/sm/cpp_api/dimension.h

Lines 143 to 150 in 523f84f

    
           /** Returns the dimension datatype. */ 
        
           tiledb_datatype_t type() const { 
        
             tiledb_datatype_t type; 
        
             auto& ctx = ctx_.get(); 
        
             ctx.handle_error( 
        
                 tiledb_dimension_get_type(ctx.ptr().get(), dim_.get(), &type)); 
        
             return type; 
        
           }

TileDB/tiledb/sm/cpp_api/domain.h

Lines 171 to 178 in 523f84f

    
           /** Returns the domain type. */ 
        
           tiledb_datatype_t type() const { 
        
             auto& ctx = ctx_.get(); 
        
             tiledb_datatype_t type; 
        
             ctx.handle_error( 
        
                 tiledb_domain_get_type(ctx.ptr().get(), domain_.get(), &type)); 
        
             return type; 
        
           }

teo-tsirpanis · 2024-08-06T22:12:13Z

+   * @param dim_idx The dimension index.
+   * @return The datatype of the range.
+   */
+  tiledb_datatype_t range_dtype(unsigned dim_idx) {


Should we actually make the C++ APIs public? Are there use cases for them outside of helping with type validation in the range getters and setters? I think those should be better served by an API to get the whole domain.

@teo-tsirpanis in Slack/Shortcut our discussion was to not expose the entire domain, but rather, subaccessors such as range_dtype and ndim individually.

teo-tsirpanis · 2024-08-06T22:12:45Z

You should also update range and set_range to use the new APIs.

robertbindar · 2024-08-07T05:17:13Z

get_type

@teo-tsirpanis get_type is used in the current domain API and returns NDRECTANGLE, it's an unfortunate overload of the word type, I think get_dtype makes things a little less confusing.

eddelbuettel · 2024-08-07T11:17:29Z

But dtype is used only as a variable not in a signature:

$ grep dtype tiledb/sm/cpp_api/*
tiledb/sm/cpp_api/enumeration_experimental.h:    tiledb_datatype_t dtype = type.value_or(DataT::tiledb_type);
tiledb/sm/cpp_api/enumeration_experimental.h:          dtype,
tiledb/sm/cpp_api/enumeration_experimental.h:          dtype,
tiledb/sm/cpp_api/enumeration_experimental.h:    tiledb_datatype_t dtype = type.value_or(DataT::tiledb_type);
tiledb/sm/cpp_api/enumeration_experimental.h:        dtype,
$

I would find get_datatype cleaner.

KiterLuc · 2024-08-08T08:32:07Z

@robertbindar could we beef up the description of the PR here? More precisely, I'd like to include the motivations that lead to us needing the extra API.

eddelbuettel · 2024-08-08T12:31:59Z

I cannot change the feeling that naming it range_dtype() is a bit of an anti-pattern as the token 'dtype' is not used in any other interface so far. But hey it's merged so we'll use it.

This PR adds capi and cppapi support for querying the number of slots/dimensions/axes of a current domain ndrectangle. Depends on #5229 to be merged first. --- TYPE: FEATURE DESC: Add dim num support for ndrectangle.

KiterLuc · 2024-08-08T13:50:18Z

@eddelbuettel If it's just a naming change, I'm happy to take it before we release. @robertbindar let's open a discussion in the core team channel to choose a name.

robertbindar requested review from DimitrisStaratzis, eddelbuettel and johnkerl August 6, 2024 13:45

robertbindar force-pushed the rbin/ch52012/ndrectangle_dtype branch 2 times, most recently from 89d6a98 to d91f05d Compare August 6, 2024 14:06

eddelbuettel reviewed Aug 6, 2024

View reviewed changes

johnkerl approved these changes Aug 6, 2024

View reviewed changes

johnkerl reviewed Aug 6, 2024

View reviewed changes

eddelbuettel reviewed Aug 6, 2024

View reviewed changes

eddelbuettel approved these changes Aug 6, 2024

View reviewed changes

robertbindar force-pushed the rbin/ch52012/ndrectangle_dtype branch 3 times, most recently from fcc96b4 to 29de8cb Compare August 6, 2024 15:56

Add support for getting dtype of ndrectangle

6c0b6f5

robertbindar force-pushed the rbin/ch52012/ndrectangle_dtype branch from 29de8cb to 6c0b6f5 Compare August 6, 2024 15:59

teo-tsirpanis reviewed Aug 6, 2024

View reviewed changes

robertbindar mentioned this pull request Aug 7, 2024

Add dim num support for ndrectangle. #5230

Merged

KiterLuc changed the title ~~Add support for getting a dtype of ndrectangle~~ Add support for getting a dtype of ndrectangle, Aug 8, 2024

KiterLuc merged commit 58dbc08 into dev Aug 8, 2024

KiterLuc deleted the rbin/ch52012/ndrectangle_dtype branch August 8, 2024 10:24

KiterLuc changed the title ~~Add support for getting a dtype of ndrectangle,~~ Add support for getting a dtype of ndrectangle. Aug 8, 2024

kounelisagis mentioned this pull request Nov 18, 2024

Remove workaround from CurrentDomain/NDRectangle TileDB-Inc/TileDB-Py#2111

Merged

Conversation

robertbindar commented Aug 6, 2024 • edited by KiterLuc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eddelbuettel Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

johnkerl left a comment

Choose a reason for hiding this comment

Uh oh!

johnkerl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddelbuettel Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

eddelbuettel left a comment

Choose a reason for hiding this comment

Uh oh!

teo-tsirpanis left a comment

Choose a reason for hiding this comment

Uh oh!

eddelbuettel commented Aug 6, 2024

Uh oh!

teo-tsirpanis commented Aug 6, 2024

Uh oh!

teo-tsirpanis Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

johnkerl Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

teo-tsirpanis Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

robertbindar commented Aug 7, 2024

Uh oh!

eddelbuettel commented Aug 7, 2024

Uh oh!

KiterLuc commented Aug 8, 2024

Uh oh!

eddelbuettel commented Aug 8, 2024

Uh oh!

KiterLuc commented Aug 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

robertbindar commented Aug 6, 2024 •

edited by KiterLuc

Loading

johnkerl left a comment •

edited

Loading