Add subarray range API wrappers #72

davisp · 2024-04-23T22:18:42Z

This is a draft PR to show the current approach to how I'm handling the range APIs. I'm gonna ask for a quick PR review to make sure that I'm not terribly off base.

I did find a few new macro patterns in @rroelke's latest PR that could reduce some of the repetitiveness in range.rs that I'll think harder about doing tomorrow with fresh eyes.

davisp · 2024-04-23T22:20:54Z

@rroelke Can you give this a quick skim to see if anything jumps out at you?

Your Foo: Type macro thing is something I'm going to go back and apply all over in range.rs. I'm also contemplating replacing my Range::as_u8_slices with your enum variant macro approach that is similar to fn_typed!.

davisp · 2024-04-24T22:38:07Z

I've rewritten the range.rs module to now use the better macro rules patterns that I stole from @rroelke and I think its a lot more reasonable. I've also improved the types a bit since we have three distinct cases to worry about:

cell_val_num == 1
cell_val_num > 1 && cell_val_num < u32::MAX
cell_val_num == u32::MAX

The reason 2 is an issue is that its never valid for dimensions ranges. Also, this cleaned up the code quite nicely in a lot of ways where things weren't super awesome, like ranges in case 2 weren't getting checked for equal length because we have to cover case 3 where different length values are valid.

davisp · 2024-04-24T22:40:06Z

I'm currently porting over a couple examples that exercise these APIs. I'll flip to a review when I finish those and add some tests for the ranges module. Hopefully that'll happen before sync tomorrow.

davisp · 2024-04-26T17:56:04Z

@rroelke This is ready for review. I've managed to get fairly decent test coverage on the range.rs module last night.

tiledb/api/src/array/dimension/mod.rs

rroelke

Will come back to this Monday morning...

tiledb/api/src/array/dimension/mod.rs

tiledb/api/src/array/schema/mod.rs

rroelke · 2024-04-27T00:08:51Z

tiledb/api/src/datatype/mod.rs

@@ -381,6 +386,56 @@ impl Datatype {
            || self.is_datetime_type()
            || self.is_time_type()
    }
+
+    #[cfg(test)]


Huh, I don't think I've seen this except for a mod tests item before, but this makes sense. Neat.

I don't remember what crate I was looking through ages ago, but they were using a similar pattern for cross platform code that was so awesome that it just stuck.

#[cfg(unix)] fn do_thing(&self) { .... } #[cfg(windows)] fn do_thing(&self) { ... }

Having just spent a year working on a cross platform C++ library for a living, the simplicity of that made it stick pretty thoroughly.

rroelke · 2024-04-27T00:10:32Z

tiledb/api/src/datatype/mod.rs

-            Datatype::StringUtf32 => unimplemented!(),
-            Datatype::StringUcs2 => unimplemented!(),
-            Datatype::StringUcs4 => unimplemented!(),
+            Datatype::Char => { type $typename = i8; $then },


Seems fine for now but I sorta wonder if in the future we should use newtypes for these so that we can do custom display, and etc.

And also now that I'm playing more with the Arrow datatypes I wonder if we should draw some inspiration from them too.

Just musing. Looks good for now

Yeah. In an ideal world our type system would get split between concrete and logical types though that's apparently contentious.

tiledb/api/src/query/subarray.rs

tiledb/api/examples/multi_range_subarray.rs

tiledb/api/src/query/subarray.rs

tiledb/api/src/range.rs

rroelke · 2024-04-30T12:45:07Z

tiledb/api/src/range.rs

+                type Error = crate::error::Error;
+                fn try_from(value: (u32, Box<[$U]>, Box<[$U]>)) ->
+                        TileDBResult<MultiValueRange> {
+                    if value.0 < 2 || value.0 == u32::MAX {


Checking that I follow - value.0 is meant to be the length? Is the idea with value.0 < 2 that you should (must) use a SingleValueRange instead?

How come you don't just check value.1.len() and value.2.len() against 2 and against each other? That sounds more user-friendly. Is it because the u32 here holds a fixed cell val num?

This would probably be more clear if that were correctly typed as a CellValNum. The logic here is saying "If you have a CellValNum::Fixed(1) or CellValNum::Var, go use the correct range type.

Fun fact, this isn't even used for domains because core doesn't support them, but it felt reasonable to include since it'd be kinda weird to not be able to create them at all.

How come you didn't just use CellValNum? That would be a lot clearer

Totes just saw this in my email so I'm un-resolving this thread. We should probably codify a better protocol for when to resolve an open thread like this rather than relying on timeouts like I did here.

To answer the question though, I basically hadn't internalized the CellValNum type at the time I wrote this. I was relying on my knowledge of internals rather than preventing the edge cases. As in, I was more worried about user experience with syntax than forcing correctness by construction.

I'll go back and change this to something better.

Also, it occurs to me that CellValNum only has the Fixed and Var variants. I'm pretty sure it should have Single Multi and Var variants to correctly express the current constraints. That'll probably be a decent change worthy of a separate PR.

Reasons favoring CellValNum::Single:

clarifies some pattern matching,CellValNum::Fixed(nz) if nz.get() == 1 could be changed to CellValNum::Single

construction of CellValNum::Fixed(1) is less work

Reasons against:

Some code doesn't care whether nz.get() == 1 and this code will get slightly worse

To prevent overlap between CellValNum::Single and CellValNum::Multi(1) we would probably need to invent a new type BoundedInt<2, u32::MAX - 1> or something. I don't hate that idea and of course there is already a crate for it.

At some point in a branch I added fn single() -> Self which addresses the construction problem. So I feel sort of neutrally about this notion.

But regardless I look forward to seeing this changed to CellValNum

tiledb/api/src/range.rs

tiledb/api/src/array/dimension/mod.rs

rroelke · 2024-05-06T12:20:42Z

tiledb/api/src/query/mod.rs

@@ -46,6 +47,20 @@ pub trait Query<'ctx> {
    fn finalize(self) -> TileDBResult<Array<'ctx>>
    where
        Self: Sized;
+
+    fn subarray(&'ctx self) -> TileDBResult<Subarray<'ctx>> {


I don't love the overload of the 'ctx lifetime name, I'd suggest 'query

Actually, it turns out to be not fine. I was pretty sure that 'ctx was more correct here when I started, but who cares about naming. But now I've had rustc tell me "No" enough times that I'm pretty sure I understand specifically why (except one particular detail I need to go find an answer for).

Regardless, I believe the 'ctx is correct here because its referring to the Schema reference in the Subarray struct which flows from the same 'ctx lifetime that parameterizes the Query instance. What you might be thinking of is the lifetime of the Subarray struct instance itself, which is the elided lifetime of the Query instance. This can be confirmed you insert a drop(query) after getting the Subarray and then attempting to use the subarray.

The only thing I couldn't figure out is why a restricted lifetime wouldn't work. I couldn't decide if this was because the compiler tracks enough to realize that the lifetime parameter for the Query must match the lifetime parameter of the Schema (regardless of what they're called) or if there's a weird issue with restricted lifetimes on generic trait methods. But all I could get in the attempt was for the rustc to tell me that 'ctx: 'query is shadowing an already defined lifetime 'ctx. I'm still reading on that one.

rroelke · 2024-05-06T12:24:16Z

Good stuff, I'll approve after the last two tweaks :)

davisp · 2024-05-06T14:33:30Z

I assume one is the 'ctx -> 'query which is fine, but what's the other tweak?

rroelke · 2024-05-06T14:46:52Z

I assume one is the 'ctx -> 'query which is fine, but what's the other tweak?

Using CellValNum

The `&'ctc self` in `Query::subarray` was required because `Array::schema(&self) -> TileDBResult<Schema>` was incorrect. It should have ben `TileDBResult<Schema<'ctx>>`.

@rroelke

@rroelke discovered that it is in fact not safe to keep a reference to a Subarray after a Query is dropped. This ties the lifetime of Subarray to the lifetime of Query to prevent the issue.

rroelke

thanks for all the good discussions!

davisp requested a review from rroelke April 23, 2024 22:18

davisp force-pushed the pd/sc-45895/wrap-range-apis branch 2 times, most recently from d66ed61 to 7a98ae0 Compare April 24, 2024 22:34

davisp force-pushed the pd/sc-45895/wrap-range-apis branch 4 times, most recently from 79b9efd to f782ad6 Compare April 25, 2024 23:47

davisp marked this pull request as ready for review April 26, 2024 17:55

rroelke reviewed Apr 26, 2024

View reviewed changes

tiledb/api/src/array/dimension/mod.rs Show resolved Hide resolved

rroelke reviewed Apr 27, 2024

View reviewed changes

rroelke requested changes Apr 30, 2024

View reviewed changes

ihnorton changed the title ~~Add subarrray range API wrappers~~ Add subarray range API wrappers May 2, 2024

davisp force-pushed the pd/sc-45895/wrap-range-apis branch 2 times, most recently from c09ce32 to 6df6f31 Compare May 3, 2024 19:19

rroelke reviewed May 3, 2024

View reviewed changes

tiledb/api/src/array/dimension/mod.rs Outdated Show resolved Hide resolved

davisp and others added 10 commits May 3, 2024 15:15

Wrap subarray range APIs

8887fcc

Fix Factor implementation for DimensionData

7ff11ae

Fix example description

5e38c88

Check the datatype for point ranges.

0133e70

Add zero copy array VarValueRange helper

663590d

Condense macro definitions

9a71b9d

Cleanup Range tests

251e577

Add tests for Subarray::ranges()

2a308be

SubarrayBuilder just has RawSubarray

0639f8c

I get by with a little help from my friends.

5f52681

davisp force-pushed the pd/sc-45895/wrap-range-apis branch from f4cbf19 to 5f52681 Compare May 3, 2024 20:18

davisp added 3 commits May 3, 2024 15:23

Add note about intended removal

0618d3a

Fix clippy issues

8eabe96

Add extra comments on DimensionBuilder::new_raw

74848e3

rroelke reviewed May 6, 2024

View reviewed changes

davisp added 5 commits May 6, 2024 10:21

Remove unneeded argument

e9170e3

Remove extraneous lifetime restriction

ad9727a

The `&'ctc self` in `Query::subarray` was required because `Array::schema(&self) -> TileDBResult<Schema>` was incorrect. It should have ben `TileDBResult<Schema<'ctx>>`.

Replace u32 with CellValNum

0f4604b

Fix subarray lifetime issue

e8b6329

@rroelke discovered that it is in fact not safe to keep a reference to a Subarray after a Query is dropped. This ties the lifetime of Subarray to the lifetime of Query to prevent the issue.

Add compile_fail test for Subarray lifetime

bc17ddc

rroelke approved these changes May 6, 2024

View reviewed changes

Add compile_fail test for Subarray lifetime

f9a7983

davisp merged commit a7bd02b into main May 6, 2024
2 checks passed

davisp deleted the pd/sc-45895/wrap-range-apis branch May 6, 2024 18:09

rroelke mentioned this pull request Jul 9, 2024

feat: [sc-50382] [tables] or [rs] implement accumulating a subarray from the intersection of predicate ranges #133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add subarray range API wrappers #72

Add subarray range API wrappers #72

davisp commented Apr 23, 2024

davisp commented Apr 23, 2024

davisp commented Apr 24, 2024

davisp commented Apr 24, 2024

davisp commented Apr 26, 2024

rroelke left a comment

rroelke Apr 27, 2024

davisp Apr 30, 2024

rroelke Apr 27, 2024

davisp Apr 30, 2024

rroelke Apr 30, 2024

davisp Apr 30, 2024

rroelke May 3, 2024

davisp May 5, 2024 •

edited

Loading

rroelke May 6, 2024

rroelke May 6, 2024

davisp May 6, 2024

rroelke commented May 6, 2024

davisp commented May 6, 2024

rroelke commented May 6, 2024

rroelke left a comment

Add subarray range API wrappers #72

Add subarray range API wrappers #72

Conversation

davisp commented Apr 23, 2024

davisp commented Apr 23, 2024

davisp commented Apr 24, 2024

davisp commented Apr 24, 2024

davisp commented Apr 26, 2024

rroelke 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davisp May 5, 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

rroelke commented May 6, 2024

davisp commented May 6, 2024

rroelke commented May 6, 2024

rroelke left a comment

Choose a reason for hiding this comment

davisp May 5, 2024 •

edited

Loading