Rest support for current domain. #5136

robertbindar · 2024-06-21T18:43:28Z

This PR adds REST serialization support for TileDB Current Domain and expansion via schema evolution.

TYPE: FEATURE
DESC: Rest support for current domain.

teo-tsirpanis

As mentioned offline, I think the CurrentDomain::type capnproto field is redundant, given that capnproto already encodes which union field is being used. I made some suggestions that might obviate its need.

teo-tsirpanis · 2024-07-01T16:25:47Z

tiledb/sm/serialization/array_schema_evolution.cc

+  auto crd = array_schema_evolution->current_domain_to_expand();
+  auto current_domain_builder =
+      array_schema_evolution_builder->initCurrentDomainToExpand();
+  current_domain_to_capnp(crd, &current_domain_builder);


What would happen if the schema evolution does not expand the current domain? Should we call initCurrentDomainToExpand only then?

This is a great catch, thanks!

tiledb/sm/serialization/current_domain.h

teo-tsirpanis · 2024-07-01T16:32:43Z

tiledb/sm/serialization/current_domain.cc

+
+  switch (crd->type()) {
+    case CurrentDomainType::NDRECTANGLE: {
+      current_domain_builder->setType(current_domain_type_str(crd->type()));


Suggested change

current_domain_builder->setType(current_domain_type_str(crd->type()));

teo-tsirpanis · 2024-07-01T16:33:49Z

tiledb/sm/serialization/current_domain.cc

+  auto type = current_domain_type_enum(current_domain_reader.getType().cStr());
+  switch (type) {
+    case CurrentDomainType::NDRECTANGLE: {
+      if (!current_domain_reader.isNdRectangle()) {
+        throw std::runtime_error(
+            "The current domain to deserialize has an unexpected type field "
+            "given the union type");
+      }


Suggested change

auto type = current_domain_type_enum(current_domain_reader.getType().cStr());

switch (type) {

case CurrentDomainType::NDRECTANGLE: {

if (!current_domain_reader.isNdRectangle()) {

throw std::runtime_error(

"The current domain to deserialize has an unexpected type field "

"given the union type");

}

switch (current_domain_reader.which()) {

case CurrentDomain::Which::ND_RECTANGLE: {

Let's keep this as is so we can merge this PR today.

teo-tsirpanis · 2024-07-01T16:34:18Z

tiledb/sm/serialization/tiledb-rest.capnp

+  type @1 :Text;
+  # The type of CurrentDomain (e.g. NDRECTANGLE)
+


Suggested change

type @1 :Text;

# The type of CurrentDomain (e.g. NDRECTANGLE)

ypatia · 2024-07-01T08:32:23Z

tiledb/sm/serialization/tiledb-rest.capnp

+  # The type of CurrentDomain (e.g. NDRECTANGLE)
+
+  union {
+    emptyCurrentDomain @2: Void;


tiledb/sm/serialization/current_domain.cc

ypatia · 2024-07-01T13:04:02Z

tiledb/sm/serialization/current_domain.cc

+
+#ifdef TILEDB_SERIALIZATION
+
+void current_domain_to_capnp(


I'd suggest to remove all shared_ptr from this file since we don't need to share ownership so we don't need to pay the performance cost or confuse the reader. We can pass const references or raw pointers in to_capnp functions and return actual objects in from_capnp functions. (make_shared and memory tracker can just move to the caller).

Left a comment below about performance concerns in this serialization code paths also about this being a bit more tedious given copy and move constructors being disabled for these classes. Overall I don't think it's worth doing it, I'll do it only if you insist 😆

I think this is fine here as the resulting object ends up storing the current domain as a shared_ptr in the array schema.

ypatia · 2024-07-01T13:20:47Z

tiledb/sm/serialization/current_domain.cc

+    shared_ptr<NDRectangle> ndr,
+    capnp::NDRectangle::Builder* ndrectangle_builder) {
+  auto& ranges = ndr->get_ndranges();
+  if (!ranges.empty()) {


what if it's empty? is that an error case, should we throw?

Empty NDRectangle set on a current domain is incorrect behavior, this is caught during Array::create. You are correct though, we can catch it early before doing the REST request, I changed the code and added a rest test check for it.

Changed this and treated it like a bug in NDRectangle. The constructors now throw if the ranges vector passed is empty or if the domain has no dimensions. This way we should never get an empty ranges vector here.

ypatia · 2024-07-02T06:56:29Z

tiledb/sm/serialization/current_domain.cc

+  }
+
+  auto type = current_domain_type_enum(current_domain_reader.getType().cStr());
+  switch (type) {


not an expert on Capnp unions but is there a reason to have the type stored ? can't we just tell from the union member that was set?

Left a common comment below for you and @teo-tsirpanis to review.

ypatia · 2024-07-02T06:57:46Z

tiledb/sm/serialization/current_domain.cc

+    shared_ptr<MemoryTracker> memory_tracker) {
+  auto version = current_domain_reader.getVersion();
+  if (current_domain_reader.isEmptyCurrentDomain()) {
+    return make_shared<CurrentDomain>(HERE(), memory_tracker, version);


as suggested in the general comments wrt shared pointers, here we can just return CurrentDomain and construct the pointer on the caller

DISABLE_COPY(CurrentDomain); DISABLE_MOVE(CurrentDomain);

Same as above, we are going to store in the schema in a shared_ptr so doing it here is fine.

ypatia · 2024-07-02T07:02:44Z

tiledb/sm/serialization/array_schema.cc

@@ -1150,6 +1153,13 @@ shared_ptr<ArraySchema> array_schema_from_capnp(
    name = schema_reader.getName().cStr();
  }

+  auto crd = make_shared<CurrentDomain>(


I think you'd like this to be in an else case, otherwise if there's a serialized domain you construct twice and just throw away the first one.
But IMO this "empty"/default object construction should be happening inside the ArraySchema constructor itself, if the caller is passing an uninitialized shared_ptr (aka nullptr). Otherwise we leave the door open to someone not knowing about this and just passing a nullptr for current_domain when constructing an ArraySchema opening the door to segfaults.. (didn't see any checks for null current_domain in ArraySchema class)

This is an easy change, I adjusted the code. In general, I'm shooting for code simplicity in this area, unless I'm doing something with disk access that could be in the same order of magnitude with the wire access, I'm not even thinking of performance.
About the empty current domain construction, there is another constructor for ArraySchema that does just that, this particular one is used [only]? in serialization and assumes we pass the correct things, the argument with passing nullptr by mistake can be extended to all other shared_ptr args we pass to this constructor.

ypatia · 2024-07-02T07:20:54Z

tiledb/sm/array_schema/ndrectangle.h

@@ -138,6 +138,14 @@ class NDRectangle {
    return domain_;
  }

+  /**


Let's make this more robust:

Add checks before every usage of domain_ in ndrectangle.cc that the pointer is not null.

Add comment on domain_ declaration that it can be null

Done, thanks!

ypatia · 2024-07-02T07:25:57Z

test/src/unit-current-domain-rest.cc

+TEST_CASE_METHOD(
+    RestCurrentDomainFx,
+    "C API: Current Domain basic behavior",
+    "[current_domain][rest]") {


Thanks for those tests!!

robertbindar · 2024-07-08T10:36:34Z

@ypatia @teo-tsirpanis thanks a lot for the great reviews! I left comments inline above.
Regarding passing the type in capnp, I chose to do it this way because the switch case on the type will be identical to the other switch cases from sm::CurrentDomain, sm::NDRectangle, etc, and then I can go and add an apply_with_type like function that will keep the switch on shape type in a single place.
Using the union type will just add a different switch that we'll need to maintain. I don't think this is a big deal either way, if any of you has a strong preference on removing the type, I'll just go and do it.

KiterLuc · 2024-07-08T12:12:19Z

test/src/unit-current-domain-rest.cc

+  tiledb_dimension_t* d1;
+  int rc = tiledb_dimension_alloc(
+      ctx_c_, "d1", TILEDB_UINT64, &dim_domain[0], &tile_extents[0], &d1);
+  CHECK(rc == TILEDB_OK);


Could we add a dimension with string type here?

KiterLuc · 2024-07-08T12:14:07Z

tiledb/sm/array_schema/array_schema.cc

@@ -1833,6 +1833,8 @@ void ArraySchema::expand_current_domain(
  new_current_domain->check_schema_sanity(this->shared_domain());

  current_domain_ = new_current_domain;
+
+  current_domain_->ndrectangle()->set_domain(this->shared_domain());


Add a comment as to why we need this, that we don't serialize the domain etc.

KiterLuc · 2024-07-08T12:15:24Z

tiledb/sm/array_schema/ndrectangle.h

+   * Array Schema domain. This can be nullptr during
+   * array schema evolution on REST when we construct with a nullptr domain_
+   * and set it later during ArraySchema::expand_current_domain to avoid
+   * serializing the domain on the evolution object */


Suggested change

* serializing the domain on the evolution object */

* serializing the domain on the evolution object

*/

KiterLuc · 2024-07-08T12:28:07Z

tiledb/sm/serialization/current_domain.cc

+
+#ifdef TILEDB_SERIALIZATION
+
+void current_domain_to_capnp(


I think this is fine here as the resulting object ends up storing the current domain as a shared_ptr in the array schema.

KiterLuc · 2024-07-08T12:33:56Z

tiledb/sm/serialization/current_domain.cc

+    shared_ptr<MemoryTracker> memory_tracker) {
+  auto version = current_domain_reader.getVersion();
+  if (current_domain_reader.isEmptyCurrentDomain()) {
+    return make_shared<CurrentDomain>(HERE(), memory_tracker, version);


Same as above, we are going to store in the schema in a shared_ptr so doing it here is fine.

KiterLuc · 2024-07-08T12:35:21Z

tiledb/sm/serialization/current_domain.cc

+  auto type = current_domain_type_enum(current_domain_reader.getType().cStr());
+  switch (type) {
+    case CurrentDomainType::NDRECTANGLE: {
+      if (!current_domain_reader.isNdRectangle()) {
+        throw std::runtime_error(
+            "The current domain to deserialize has an unexpected type field "
+            "given the union type");
+      }


Let's keep this as is so we can merge this PR today.

teo-tsirpanis · 2024-07-08T12:48:22Z

Re. the explicit type field in capnp, it's a question of whether we want to expose an idiomatic capnp schema or match the Core's internals as closely as possible.

Generally I am a big proponent of having clean data representation boundaries between software layers and would always choose the former, but I can understand the practical considerations (in this case not as much the code variation but the time pressure to have this PR merged) that would lead us to pick the latter option, and I am fine with either option.

teo-tsirpanis

Thanks!

robertbindar requested review from ypatia and KiterLuc June 21, 2024 18:43

teo-tsirpanis reviewed Jul 1, 2024

View reviewed changes

ypatia reviewed Jul 2, 2024

View reviewed changes

robertbindar added 2 commits July 8, 2024 13:37

rest support for current domain

c02afb3

address review comments

fe6b894

robertbindar force-pushed the rbin/ch48253/current_domain_rest branch from 0218ee7 to fe6b894 Compare July 8, 2024 10:37

KiterLuc reviewed Jul 8, 2024

View reviewed changes

robertbindar added 2 commits July 8, 2024 16:35

review comments

641acd9

add string dimension to rest tests

dbef1dd

teo-tsirpanis approved these changes Jul 8, 2024

View reviewed changes

Fix CI.

6193391

KiterLuc closed this Jul 8, 2024

KiterLuc reopened this Jul 8, 2024

Fix CI 2.

175f52e

KiterLuc force-pushed the rbin/ch48253/current_domain_rest branch from 7efc109 to 175f52e Compare July 8, 2024 20:02

KiterLuc changed the title ~~Rest support for current domain~~ Rest support for current domain. Jul 9, 2024

KiterLuc merged commit acca149 into dev Jul 9, 2024
63 checks passed

KiterLuc deleted the rbin/ch48253/current_domain_rest branch July 9, 2024 08:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rest support for current domain. #5136

Rest support for current domain. #5136

robertbindar commented Jun 21, 2024 •

edited by KiterLuc

Loading

teo-tsirpanis left a comment

teo-tsirpanis Jul 1, 2024

robertbindar Jul 8, 2024

teo-tsirpanis Jul 1, 2024

teo-tsirpanis Jul 1, 2024

KiterLuc Jul 8, 2024

teo-tsirpanis Jul 1, 2024

ypatia Jul 1, 2024

ypatia Jul 1, 2024

robertbindar Jul 8, 2024

KiterLuc Jul 8, 2024

ypatia Jul 1, 2024

robertbindar Jul 8, 2024

robertbindar Jul 8, 2024

ypatia Jul 2, 2024

robertbindar Jul 8, 2024

ypatia Jul 2, 2024

robertbindar Jul 8, 2024

KiterLuc Jul 8, 2024

ypatia Jul 2, 2024

robertbindar Jul 8, 2024

ypatia Jul 2, 2024

robertbindar Jul 8, 2024

ypatia Jul 2, 2024

robertbindar commented Jul 8, 2024 •

edited by teo-tsirpanis

Loading

KiterLuc Jul 8, 2024

KiterLuc Jul 8, 2024

KiterLuc Jul 8, 2024

KiterLuc Jul 8, 2024

KiterLuc Jul 8, 2024

KiterLuc Jul 8, 2024

teo-tsirpanis commented Jul 8, 2024

teo-tsirpanis left a comment

		type @1 :Text;
		# The type of CurrentDomain (e.g. NDRECTANGLE)

	* serializing the domain on the evolution object */
	* serializing the domain on the evolution object
	*/

Rest support for current domain. #5136

Rest support for current domain. #5136

Conversation

robertbindar commented Jun 21, 2024 • edited by KiterLuc Loading

teo-tsirpanis 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

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

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

robertbindar commented Jul 8, 2024 • edited by teo-tsirpanis 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

teo-tsirpanis commented Jul 8, 2024

teo-tsirpanis left a comment

Choose a reason for hiding this comment

robertbindar commented Jun 21, 2024 •

edited by KiterLuc

Loading

robertbindar commented Jul 8, 2024 •

edited by teo-tsirpanis

Loading