throw error on dimension drop in schema evolution.

[DimitrisStaratzis]

Adding an error message when trying to drop a non existent attribute or a dimension with the Schema Evolution API. + Test

[sc-30774]

---

TYPE: BUG
DESC: Throw error on dimension drop in schema evolution.

[eric-hughes-tiledb]

The new validation check happens too late. This code has it happening when the operation runs, not when it's first specified.

Doing this correctly means implementing validation checks in the two places that the attributes_to_drop_ can gain new members:

• In drop_attribute, which is connected to the C API.
• In attribute_names_to_drop, which is used only by capnp serialization.

In addition, the validation check itself isn't quite right. See drop_attribute for a check that's not made in the new code. Furthermore, the validations for incremental changes through the API are not identical to those made with serialization, where we can validate everything together more clearly and efficiently than making the same checks incrementally.

I would be satisfied if we only tightened up drop_attribute in this PR. We can leave validation for capnp for later. That means at minimum three things:

• A story to add the requisite validation checks for capnp.
• Documentation of the invariants in the class members. The descriptions in the current docs for the relevant member variables are inadequate. Here are two of them; neither is currently documented explicitly in the code.
  • Each element in attributes_to_drop_ must be an attribute of the array schema
  • attributes_to_drop_ and the attributes to add must be disjoint
• Validation in drop_attribute that the invariants are satisfied. This is where the validation check that this PR seeks to add belongs.

Note the ordering. Getting the code in place is lower priority than the others. If you have the others, the code will eventually get right, but not vice-versa.

[eric-hughes-tiledb]

This is the statement that should fail. Avoiding a failure here only defers detection of the problem until later.

[DimitrisStaratzis]

Thanks for the comments Eric. I agree that the check should happen further up in drop_attribute() however the ArraySchema object (which is required for the use of has_attribute()) is not available in drop_attribute(). It is only passed as a parameter in ArraySchemaEvolution::array_evolve() in the form of the array_uri which is then used to create the schema object

Here is the current order:

Array::create(array_uri, schema);
auto evolution = ArraySchemaEvolution(ctx);
evolution.drop_attribute("d1");
evolution.array_evolve(array_uri));

[eric-hughes-tiledb]

the ArraySchema object [...] is not available in drop_attribute()

Yes, I see that now. I didn't realize the whole design was broken in this particular way.

[eric-hughes-tiledb]

This test is not quite right. The issue is with capnp deserialization. As with all input that comes in from the outside, here from the network, we need to validate inputs. We may not assume that a capnp message was generated by libtiledb necessarily; it might have been formed by other, likely malicious, software. We don't need to care why it was created, only that it's detected and rejected.

There's a check in drop_attribute to see if the attribute is the name of an attribute that was previously added. In this case, there's no check to see if the dropped attribute here also appears in the list of added attributes.

[eric-hughes-tiledb]

Approving for expediency.

This code does not correct the validation issues with capnp.