chore: [sc-57786] [rs] minimize set of functions which need to link with core tiledb to run #179

rroelke · 2024-10-15T14:20:54Z

Story details: https://app.shortcut.com/tiledb-inc/story/57786

This pull request is a rather large "Part 1" of the linked story, whose goal is to restructure our crates in tiledb-rs so that the only code which requires linking with core tiledb is the code which actually calls the C APIs. Doing this will simplify the build process for users of the tiledb-rs crates which do not need to use the C APIs directly, such as a client which sends RPCs to a server in tiledb-cloud.

We have written handles to tiledb schema objects: struct Attribute, struct Dimension, struct Domain, struct Schema, and so on. When we turned our eye towards property-based testing, we concluded that we needed to write structures whose fields duplicate the fields of the objects inside of the core library (whereas the handles just provide methods to get or set them). But over time we have come to use these secondary structures more and more, as they are often more user-friendly. The goal of this "Part 1" pull request is to move these secondary structures to their own crate, which is tentatively named tiledb-serde in the diff.

Unfortunately this goal requires moving lots and lots of other pieces.

The new tiledb crates:

tiledb-common: contains enums and other utility pieces, most notably including Datatype, FilterData, and Range.
tiledb-serde: contains the secondary structures described above. The name comes from the idea that these structures can be used to derive Desesrialize and Serialize for the API handles. I think the name should be changed but I wanted to get things compiling and running again before taking that step.
tiledb-api: renamed from the previous monolithic tiledb.

Each of the above crates has features serde and proptest-strategies. The serde feature enables derived Deserialize and Serialize impls, but for tiledb-api this also adds tiledb-serde as a dependency. The proptest-strategies feature similarly defines Arbitrary impls (for the most part) and is used as a dev-depedency by the tiledb-api crate.

There is a bit of thiserror-ization as entities which previously used TileDBError were moved into a new crate which does not have TileDBError. But the tiledb-api crate itself still uses the large TileDBError enum.

Additionally, this pull request adds a new top-level directory to the workspace, test-utils. This moves the various disjoint items from the previous tiledb-test-utils crate into their own crates. All of the Cells testing code is also moved into a new crate cells in this directory. This was mostly done to split out strategy-ext but the rest followed from there.

For the most part, code has just moved around, without any changes, but there are some exceptions:

One difficulty with the crate splitting is our use of the PhysicalType trait. Because this trait is now defined in a different crate than tiledb-api, we cannot provide generic impls on PhysicalType for some of the query stuff. A new trait CellValue is introduced to work around this.
Similarly the cells crate cannot have its attach_read and attach_write methods anymore since those use structures from tiledb-api, which uses on cells as a test dependency. To resolve this the tiledb-api crate declares traits ToReadQuery and ToWriteQuery and provides impls for the items from cells.
We previously used FilterListData to represent a filter list handle for proptests; this has been replaced with Vec<FilterData>.

…ll features

…-split

rroelke · 2024-10-15T14:37:25Z

test-utils/cells/Cargo.toml

The cells crate contains the structure definitions and trait impls of code which used to live in api/src/query/strategy.rs and api/src/query/write/strategy.rs. This code has been exported and is used in tables tests. This crate depends on tiledb-serde because it uses the schema to make some decisions about values and etc.

I don't think there is any new code in the cells crate itself, but splitting this out is what has led to the ToReadQuery and ToWriteQuery traits in tiledb-api.

rroelke · 2024-10-15T14:39:43Z

test-utils/proptest-config/Cargo.toml

This new crate proptest-config just contains the configuration settings for proptest that we previously had. I moved it to its own crate because cells and tiledb-serde both use it.

rroelke · 2024-10-15T14:40:16Z

test-utils/signal/Cargo.toml

We had some signal stuff in tiledb-test-utils, this crate contains that code.

rroelke · 2024-10-15T14:41:37Z

test-utils/strategy-ext/Cargo.toml

tiledb-test-utils previously had a bunch of generic strategy extensions which now live in this new crate.

rroelke · 2024-10-15T14:43:26Z

test-utils/uri/Cargo.toml

This crate is for the URI generator stuff which previously lived in tiledb-test-utils.

Initially I had split all of the tiledb-test-utils modules into separate features rather than separate crates; but this was sort of gnarly for dependencies, which is when I moved them all to their own crates.

No new code in this crate, just moved around.

rroelke · 2024-10-15T14:48:23Z

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

@@ -473,330 +476,18 @@ impl Builder {
    }
 }

-/// Trait for data which can be used as a fill value for an attribute.
-pub trait IntoFillValue {


This was moved into tiledb-common because of the impls which build upon PhysicalType. I did this before adding the CellValue trait; that would probably be more appropriate to use and we could move this trait back into tiledb-api.

rroelke · 2024-10-15T14:49:56Z

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

-    }
-}
-
-impl TryFrom<&Attribute> for AttributeData {


The conversions between AttributeData and Attribute now live in the serde module, i.e. tiledb/api/src/array/attribute/serde.rs.

This is the case for all the other ThingData structs as well

rroelke · 2024-10-15T14:52:47Z

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

@@ -170,275 +158,16 @@ impl PartialEq<Dimension> for Dimension {
    }
 }

-#[derive(Clone, Debug, Deserialize, OptionSubset, PartialEq, Serialize)]
-pub enum DimensionConstraints {


This is now in tiledb-common though perhaps it should go into tiledb-serde.

rroelke · 2024-10-15T14:53:48Z

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

        let c_name = cstring!(name);
-        let c_domain = constraints.domain_ptr();
-        let c_extent = constraints.extent_ptr();
+        let c_domain = dimension_constraints_go!(


This was the body of DimensionConstraints::domain_ptr.
It didn't really feel right to have an otherwise Rust-only crate provide methods which deal with raw pointers.

rroelke · 2024-10-15T14:55:13Z

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

    #[test]
-    fn subarray_strategy_dense() {


This test doesn't actually use any tiledb APIs so it now lives in serde/src/array/dimension/strategy.rs

rroelke · 2024-10-15T14:56:28Z

tiledb/api/src/array/mod.rs

 };
-
-#[derive(Clone, Debug, PartialEq)]
-pub enum Mode {


All the enums have been moved to tiledb-common.

Except for Encryption which is not used by anything else yet. Though it does seem like something we might want to put in an RPC, so it probably should be moved there.

rroelke · 2024-10-15T15:02:23Z

tiledb/api/src/datatype/arrow.rs

-
-/// Represents tiledb (`Datatype`, `CellValNum`) compatibility for an arrow `DataType`.
-#[derive(Clone, Debug, Eq, PartialEq)]
-pub enum DatatypeToArrowResult {


All of this is moved to tiledb-common/src/datatype/arrow.rs

rroelke · 2024-10-15T15:04:26Z

tiledb/api/src/datatype/mod.rs


-use crate::error::DatatypeErrorKind;
-use crate::Result as TileDBResult;
+impl ToStringCore for Datatype {


The enum is moved to tiledb-common/src/datatype/mod.rs.

What's here now is just trait impls for new traits ToStringCore and FromStringCore which call the tiledb core string functions.

Since these require linking with libtiledb, tiledb-common specifically does not want them. Hence Debug is derived and Display just uses that.

rroelke · 2024-10-15T15:05:35Z

tiledb/api/src/error.rs

@@ -26,107 +29,6 @@ impl Drop for RawError {
    }
 }

-#[derive(Clone, Debug)]
-pub enum DatatypeErrorKind {


Some variants are moved to tiledb_common::datatype::Error and others are now just Error variants.

rroelke · 2024-10-15T15:09:06Z

tiledb/api/src/filter/list.rs

-#[derive(
-    Clone, Debug, Default, Deserialize, OptionSubset, PartialEq, Serialize,
-)]
-pub struct FilterListData(Vec<FilterData>);


In contrast to other items which are moved somewhere else, this one I just deleted and replaced uses of it with Vec<FilterData>.

rroelke · 2024-10-15T15:09:34Z

tiledb/api/src/filter/mod.rs

-#[derive(
-    Copy, Clone, Debug, Deserialize, Eq, OptionSubset, PartialEq, Serialize,
-)]
-pub enum CompressionType {


Moved to tiledb_common::filter

rroelke · 2024-10-15T15:11:06Z

tiledb/api/src/metadata.rs

-use util::option::OptionSubset;
-
-#[derive(Clone, Debug, Deserialize, OptionSubset, PartialEq, Serialize)]
-pub enum Value {


This is moved to tiledb_common. This module now just has functions for FFI.

davisp

Whoops. I totes forgot that I've got a dinner thing tonight so I didn't get as far into this as I'd like.

After my skimming so far, about the only thing I wish were different is the proptest strategies in the pod crate being pushed into their own. Granted I'm well aware of why that didn't and might not ever happen so its not really important. And its not like this isn't an improvement anyway.

Also heads up that there's a bunch of modules in the API crate all called serde that look like they should have been renamed to match the s/serde/pod/ rename you did earlier.

Other than that, this all looks pretty good. But given how large it is I am gonna give it another look over tomorrow before I approve. Today I mostly just looked at the new structure and spot checked that things pretty much ended up where I expected and so far. Tomorrow I'll give a closer skim on the actual diff before +1'ing.

Good work though! This will be a nice clean up of some of our initial decisions made while earlier in our Rust learning adventure.

davisp · 2024-10-17T22:15:23Z

tiledb/pod/src/array/attribute/mod.rs

+    pub nullability: Option<bool>,
+}
+
+#[cfg(any(test, feature = "proptest-strategies"))]


Is there any reason why you don't just move this impl to the strategy module?

Just a stylistic choice to have the impl in the same file that the struct is defined in.
I have a weak preference for this because it means only searching one file for the impls. Though admittendly one should probably build rustdocs for exploring the impls.

rroelke · 2024-10-18T12:30:40Z

Also heads up that there's a bunch of modules in the API crate all called serde that look like they should have been renamed to match the s/serde/pod/ rename you did earlier.

Good catch, fixed!

davisp

Big change but everything looks good to me. The only thing I spotted that had me wondering are the extra features that don't feel very feature level. But those are easy enough to undo later that I'm not gonna request any changes there.

Other than that, all looked good. The error handling cleanup was quite nice to see throughout. I left a few notes, but I think more than half were basically just notes to myself.

+1

davisp · 2024-10-21T15:59:07Z

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

@@ -303,6 +291,19 @@ impl PartialEq<Attribute> for Attribute {
    }
 }

+#[cfg(any(test, feature = "pod"))]


Seems slightly odd to have the pod crate as an optional dependency here.

Well... it definiitely makes use of the API way easier but it's not necessary to use it. If it's a feature than there is a leaner option to use tiledb as a dependency.

To me it was just that it seems like the pod definitions are. the way to interact with the bindings, so making it a feature that requires opting into didn't feel quite right. And this isn't super duper well thought out. I was just thinking "How often are we going to not want those pod APIs?" and I'm reasonably sure "Almost never, and the cost of having them and not using them would likely be miniscule."

Also its fine for now. We can always go back and change things up if we decide that it really is unnecessary for some particular use case where it matters.

Indeed we could - and everyone loves PRs with lots of red

davisp · 2024-10-21T16:04:04Z

tiledb/api/src/array/attribute/pod.rs

+    }
+}
+
+impl TryFrom<Attribute> for AttributeData {


I assume this was just moved from wherever it was before, but seeing a try_from for both a ref and consuming seems odd. I can't quite convince myself this version would never be needed, but it sure seems unlikely.

Not asking for a change here. I'm just noting something I saw for future me writing trait implementations.

I don't really remember why I added this the first time, yeah. Probably trying to do something mildly clever with iterators.

davisp · 2024-10-21T16:07:12Z

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

-#[cfg(any(test, feature = "proptest-strategies"))]
-pub mod strategy;
+#[cfg(any(test, feature = "pod"))]
+pub mod pod;


Still not sure on this pod as feature thing. Also, we should probably standardize that all module definitions (that aren't tests) should go at the top of the file. Finding the code/test transition isn't the easiest thing to navigate to just to check if there's anything interesting going on. Don't worry about that as part of this PR. Just another thing I've noticed while reading through.

Yeah I think we have some stylization to do

modules at the top

use declaration un-grouping

tiledb/api/src/filter/ftype.rs

davisp · 2024-10-21T16:27:35Z

tiledb/common/Cargo.toml

+serde_json = { workspace = true, optional = true }
+thiserror = { workspace = true }
+tiledb-proc-macro = { workspace = true, optional = true }
+tiledb-sys = { workspace = true }


I'm reading this PR pretty quickly, so I may have missed an actual ffi call, but I would only expect (and have only seen) references to various constants. We could pretty easily split those off into a tiledb-sys-defs crate to avoid this crate from requiring a link to libtiledb.so. Not needed for this PR, just a thought I had keeping in mind part of the motivation for this PR.

Each time I add a new crate I have to add a new build.rs file for that crate. Isn't that what sets the linker path, not the presence of the sys crate?

That said I do think it might be nice to split out the typedefs and constants for sys

There's two halves: The dependency on tiledb-sys introduces the requirement that libtiledb.so is linked to the final executable. The build.rs in non-tiledb-sys crates tells the linker where to find it on the current system (assuming its not in a system standard path).

So basically, if we split out tiledb-sys-defs that contains just the constant definitions, then anything that doesn't depend on tiledb-sys (directly or indirectly) will no longer need to provide the build.rs.

Or in other words, the build.rs requirement is a symptom of what's actually linking to libtiledb.so, when we remove that need we should be able to remove a lot of the build.rs scripts from the crates that no longer need it.

Ok I see that more clearly now from looking at sys/build.rs. Definitely will follow up with that in a subsequent PR. Thanks!

davisp · 2024-10-21T16:30:53Z

tiledb/common/src/filter/webp.rs

-    Copy, Clone, Debug, Deserialize, Eq, OptionSubset, PartialEq, Serialize,
-)]
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+#[cfg_attr(feature = "option-subset", derive(OptionSubset))]


Another feature that I'm not 100% sold on. Easy to change down the line, so not requesting a change. Just seems a bit "feature happy" is all.

tiledb/common/src/query/read.rs

davisp · 2024-10-21T16:34:19Z

tiledb/pod/Cargo.toml

+serde_json = { workspace = true, optional = true }
+strategy-ext = { workspace = true, optional = true }
+thiserror = { workspace = true }
+tiledb-common = { workspace = true }


Just a note to self that this hasn't removed the libtiledb.so linkage since we've got this transitive dependency. Though it looks pretty straightforward to cut that tie in a follow up PR that I assume is probably already half written waiting for me to approve this one. So on with the show!

tiledb/pod/src/lib.rs

- move cfg impls into strategy.rs - remove ToStringCore and FromStringCore - fix 'cargo check --tests' from API crate - Fix comment typo - Remove empty pod mod query

…-split

rroelke added 23 commits October 10, 2024 12:25

First step splitting crates from api; tiledb-common passes tests on a…

29fcaa2

…ll features

cargo check no errors on api crate

53ecb06

Update examples

3d15d9e

cargo check with serde feature

46c1afa

api crate compiles with serde feature

74da759

serde crate compiles cargo test

139e56b

tiledb-common does not depend on tiledb-test-utils

8296c30

Split out test-utils generic strategies into their own crate

c5fe283

Split out Cells into its own test-utils crate

cdad8ba

Split other test utils into their own test-utils crates

d02e261

Down to one error in 'api' crate 'cargo test'

690326d

test-utils/proptest-config crate

a88220d

Add back cells write inputs

27249f6

api crate cargo test runs

69f69b7

api arrow feature compiles with cargo test

f6ae514

Forgot some test-utils files

167d3d3

Forgot serde/src/query/mod.rs

42a8dee

Fix queries crate and example

4d1f1bb

Fix cargo check --workspace

4ead776

Fix FilterData::transform_datatype for Xor case

0bc6e44

clippy

20652f4

Fix doc tests

78aa248

Merge remote-tracking branch 'origin/main' into rr/sc-57786-api-crate…

0e44103

…-split