chore: [sc-63841] [core] adding multiple attributes with schema evolution does not add them in a predictable order (#5470)

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

Previously attributes were stored in the `SchemaEvolution` via
`std::unordered_map` which resulted in an arbitrary order that the
attributes would be appended to the target schema.

This pull request uses `std::vector` instead to preserve the order of
added attributes. Lookups by attribute name now require a linear scan of
the added attributes. This will probably be faster than the
`unordered_map` lookup was for small numbers of attributes; when there
are many attributes it may not be, but it should be a rare use case
where that would be noticeable.

I also noticed that it is possible to evolve into an invalid schema
which cannot be loaded. This pull request adds an integrity check before
returning a successful schema evolution to fix this.

---
TYPE: IMPROVEMENT
DESC: Schema evolution consistent attribute order

Author: rroelke

test/src/unit-cppapi-schema-evolution.cc

@@ -32,7 +32,10 @@
 
 #include <test/support/src/vfs_helpers.h>
 #include <test/support/tdb_catch.h>
+#include "test/support/src/array_helpers.h"
+#include "test/support/src/array_schema_helpers.h"
 #include "test/support/src/mem_helpers.h"
+
 #include "tiledb/sm/array_schema/array_schema.h"
 #include "tiledb/sm/array_schema/array_schema_evolution.h"
 #include "tiledb/sm/array_schema/attribute.h"
@@ -48,6 +51,23 @@
 
 using namespace tiledb;
 
+/**
+ * @return a simple schema with dimension d1 and attribute a1
+ */
+static ArraySchema simple_schema(Context& ctx) {
+  Domain domain(ctx);
+  auto d1 = Dimension::create<int>(ctx, "d1", {{-100, 100}}, 10);
+  domain.add_dimension(d1);
+
+  auto a1 = Attribute::create<int>(ctx, "a1");
+
+  ArraySchema schema(ctx, TILEDB_DENSE);
+  schema.set_domain(domain);
+  schema.add_attribute(a1);
+
+  return schema;
+}
+
 TEST_CASE(
     "C++ API: SchemaEvolution, add and drop attributes",
     "[cppapi][schema][evolution][add][drop][rest]") {
@@ -950,3 +970,182 @@ TEST_CASE(
 
   CHECK_NOTHROW(ase->evolve_schema(schema));
 }
+
+TEST_CASE(
+    "C++ API: SchemaEvolution add multiple attributes",
+    "[cppapi][schema][evolution][add]") {
+  test::VFSTestSetup vfs_test_setup;
+  Context ctx{vfs_test_setup.ctx()};
+  auto array_uri{vfs_test_setup.array_uri(
+      "test_schema_evolution_add_multiple_attributes")};
+
+  // create initial array
+  Domain domain(ctx);
+  auto d1 = Dimension::create<int>(ctx, "d1", {{-100, 100}}, 10);
+  domain.add_dimension(d1);
+
+  auto a1 = Attribute::create<int>(ctx, "a1");
+
+  ArraySchema schema(ctx, TILEDB_DENSE);
+  schema.set_domain(domain);
+  schema.add_attribute(a1);
+
+  auto add_attributes = std::vector<Attribute>{
+      Attribute::create<int>(ctx, "a2"),
+      Attribute::create<int>(ctx, "a3"),
+      Attribute::create<int>(ctx, "a4")};
+
+  auto permutation = GENERATE(
+      std::vector<int>{0, 1, 2},
+      std::vector<int>{0, 2, 1},
+      std::vector<int>{1, 0, 2},
+      std::vector<int>{1, 2, 0},
+      std::vector<int>{2, 0, 1},
+      std::vector<int>{2, 1, 0});
+
+  DYNAMIC_SECTION(
+      "Add a1/a2/a3 in permutation: " + std::to_string(permutation[0]) + "/" +
+      std::to_string(permutation[1]) + std::to_string(permutation[2])) {
+    // create array
+    Array::create(array_uri, schema);
+    test::DeleteArrayGuard guard(ctx.ptr().get(), array_uri.c_str());
+
+    // evolve it
+    auto evolution = ArraySchemaEvolution(ctx);
+    for (auto idx : permutation) {
+      evolution.add_attribute(add_attributes[idx]);
+    }
+    evolution.array_evolve(array_uri);
+
+    // check attribute order
+    auto schema = Array::load_schema(ctx, array_uri);
+    std::vector<Attribute> attributes;
+    for (unsigned a = 0; a < schema.attribute_num(); a++) {
+      attributes.push_back(schema.attribute(a));
+    }
+
+    CHECK(attributes.size() == 4);
+    if (attributes.size() >= 1) {
+      CHECK(test::is_equivalent_attribute(attributes[0], a1));
+    }
+    if (attributes.size() >= 2) {
+      CHECK(test::is_equivalent_attribute(
+          attributes[1], add_attributes[permutation[0]]));
+    }
+    if (attributes.size() >= 3) {
+      CHECK(test::is_equivalent_attribute(
+          attributes[2], add_attributes[permutation[1]]));
+    }
+    if (attributes.size() >= 4) {
+      CHECK(test::is_equivalent_attribute(
+          attributes[3], add_attributes[permutation[2]]));
+    }
+  }
+}
+
+TEST_CASE(
+    "C++ API: SchemaEvolution add duplicate attribute",
+    "[cppapi][schema][evolution][add]") {
+  test::VFSTestSetup vfs_test_setup;
+  Context ctx{vfs_test_setup.ctx()};
+  auto array_uri{vfs_test_setup.array_uri(
+      "test_schema_evolution_add_duplicate_attribute")};
+
+  // create initial array
+  Domain domain(ctx);
+  auto d1 = Dimension::create<int>(ctx, "d1", {{-100, 100}}, 10);
+  domain.add_dimension(d1);
+
+  auto a1 = Attribute::create<int>(ctx, "a1");
+
+  ArraySchema schema(ctx, TILEDB_DENSE);
+  schema.set_domain(domain);
+  schema.add_attribute(a1);
+
+  SECTION("Add attribute to evolution twice") {
+    // try evolving
+    auto evolution = ArraySchemaEvolution(ctx);
+    evolution.add_attribute(Attribute::create<int>(ctx, "a2"));
+    CHECK_THROWS(evolution.add_attribute(Attribute::create<int>(ctx, "a2")));
+
+    evolution.drop_attribute("a2");
+    evolution.add_attribute(Attribute::create<int>(ctx, "a2"));
+    CHECK_THROWS(evolution.add_attribute(Attribute::create<int>(ctx, "a2")));
+  }
+
+  SECTION("Add attribute with same name as schema attribute") {
+    // create array
+    Array::create(array_uri, schema);
+    test::DeleteArrayGuard guard(ctx.ptr().get(), array_uri.c_str());
+
+    // try evolving
+    auto evolution = ArraySchemaEvolution(ctx);
+    evolution.add_attribute(Attribute::create<int>(ctx, "a1"));
+
+    // should throw, cannot add an attribute with the same name
+    CHECK_THROWS(evolution.array_evolve(array_uri));
+
+    // load schema back should succeed
+    CHECK_NOTHROW(Array::load_schema(ctx, array_uri));
+  }
+}
+
+TEST_CASE(
+    "C++ API: SchemaEvolution drop last attribute",
+    "[cppapi][schema][evolution][drop]") {
+  test::VFSTestSetup vfs_test_setup;
+  Context ctx{vfs_test_setup.ctx()};
+  auto array_uri{
+      vfs_test_setup.array_uri("test_schema_evolution_drop_last_attribute")};
+
+  // create array
+  ArraySchema schema = simple_schema(ctx);
+  Array::create(array_uri, schema);
+  test::DeleteArrayGuard guard(ctx.ptr().get(), array_uri.c_str());
+
+  // try evolving
+  auto evolution = ArraySchemaEvolution(ctx);
+  evolution.drop_attribute("a1");
+
+  // should throw, schema must have at least one attribute
+  CHECK_THROWS(evolution.array_evolve(array_uri));
+
+  // load schema back should succeed
+  CHECK_NOTHROW(Array::load_schema(ctx, array_uri));
+}
+
+/**
+ * Add an enumeration which is not used by any attribute.
+ *
+ * FIXME: should this be an error?
+ * As far as I can tell the enumeration is undiscoverable if no attribute
+ * references it. It can be looked up by name but there is no
+ * API to list enumeration names.
+ */
+TEST_CASE("C++ API: SchemaEvolution add unused enumeration") {
+  test::VFSTestSetup vfs_test_setup;
+  Context ctx{vfs_test_setup.ctx()};
+  auto array_uri{
+      vfs_test_setup.array_uri("test_schema_evolution_add_unused_enumeration")};
+
+  // create array
+  ArraySchema schema = simple_schema(ctx);
+  Array::create(array_uri, schema);
+  test::DeleteArrayGuard guard(ctx.ptr().get(), array_uri.c_str());
+
+  // evolve
+  auto evolution = ArraySchemaEvolution(ctx);
+  Enumeration enumeration_in = Enumeration::create_empty(
+      ctx, "us_states", TILEDB_STRING_ASCII, tiledb::sm::constants::var_num);
+  evolution.add_enumeration(enumeration_in);
+
+  evolution.array_evolve(array_uri);
+
+  auto schema_out = Array::load_schema(ctx, array_uri);
+
+  Enumeration enumeration_out =
+      ArraySchemaExperimental::get_enumeration_from_name(
+          ctx, schema_out, "us_states");
+
+  CHECK(test::is_equivalent_enumeration(enumeration_in, enumeration_out));
+}

test/src/unit-enumerations.cc

@@ -1639,7 +1639,7 @@ TEST_CASE_METHOD(
 
   auto orig_schema = array->array_schema_latest_ptr();
   auto ase = make_shared<ArraySchemaEvolution>(HERE(), memory_tracker_);
-  auto attr3 = make_shared<Attribute>(HERE(), "attr3", Datatype::UINT32);
+  auto attr3 = make_shared<Attribute>(HERE(), "attr4", Datatype::UINT32);
   ase->add_attribute(attr3);
   CHECK_NOTHROW(ase->evolve_schema(orig_schema));
 }
@@ -1656,7 +1656,7 @@ TEST_CASE_METHOD(
   auto enmr = create_enumeration(values);
   ase->add_enumeration(enmr);
 
-  auto attr3 = make_shared<Attribute>(HERE(), "attr3", Datatype::UINT32);
+  auto attr3 = make_shared<Attribute>(HERE(), "attr4", Datatype::UINT32);
   attr3->set_enumeration_name(default_enmr_name);
   ase->add_attribute(attr3);
 

test/support/src/array_schema_helpers.cc

@@ -31,11 +31,55 @@
  */
 
 #include "test/support/src/array_schema_helpers.h"
+#include "tiledb/api/c_api/enumeration/enumeration_api_internal.h"
 
 using namespace tiledb;
 
 namespace tiledb::test {
 
+bool is_equivalent_filter(
+    const tiledb::Filter& left, const tiledb::Filter& right) {
+  // FIXME if it ever matters: check options
+  return left.filter_type() == right.filter_type();
+}
+
+bool is_equivalent_filter_list(
+    const tiledb::FilterList& left, const tiledb::FilterList& right) {
+  if (left.nfilters() != right.nfilters()) {
+    return false;
+  }
+  for (unsigned i = 0; i < left.nfilters(); i++) {
+    if (!(is_equivalent_filter(left.filter(i), right.filter(i)))) {
+      return false;
+    }
+  }
+  return true;
+}
+
+bool is_equivalent_attribute(
+    const tiledb::Attribute& left, const tiledb::Attribute& right) {
+  if (!(left.name() == right.name() && left.type() == right.type() &&
+        left.cell_val_num() == right.cell_val_num() &&
+        left.nullable() == right.nullable() &&
+        is_equivalent_filter_list(left.filter_list(), right.filter_list()))) {
+    return false;
+  }
+
+  const void *leftfill, *rightfill;
+  uint64_t leftfillsize, rightfillsize;
+  uint8_t leftfillvalid, rightfillvalid;
+  if (left.nullable()) {
+    left.get_fill_value(&leftfill, &leftfillsize, &leftfillvalid);
+    right.get_fill_value(&rightfill, &rightfillsize, &rightfillvalid);
+  } else {
+    left.get_fill_value(&leftfill, &leftfillsize);
+    right.get_fill_value(&rightfill, &rightfillsize);
+    leftfillvalid = rightfillvalid = 1;
+  }
+  return leftfillsize == rightfillsize && leftfillvalid == rightfillvalid &&
+         (memcmp(leftfill, rightfill, leftfillsize) == 0);
+}
+
 bool is_equivalent_enumeration(
     const sm::Enumeration& left, const sm::Enumeration& right) {
   return left.name() == right.name() && left.type() == right.type() &&
@@ -48,4 +92,10 @@ bool is_equivalent_enumeration(
              right.data().end());
 }
 
+bool is_equivalent_enumeration(
+    const Enumeration& left, const Enumeration& right) {
+  return is_equivalent_enumeration(
+      *left.ptr()->enumeration().get(), *right.ptr()->enumeration().get());
+}
+
 }  // namespace tiledb::test

test/support/src/array_schema_helpers.h

@@ -33,17 +33,49 @@
 #ifndef TILEDB_TEST_ARRAY_SCHEMA_HELPERS_H
 #define TILEDB_TEST_ARRAY_SCHEMA_HELPERS_H
 
+#include "tiledb/sm/array_schema/array_schema.h"
+#include "tiledb/sm/array_schema/attribute.h"
 #include "tiledb/sm/array_schema/enumeration.h"
+#include "tiledb/sm/cpp_api/tiledb"
+
+#include "tiledb/sm/cpp_api/tiledb"
+#include "tiledb/sm/cpp_api/tiledb_experimental"
 
 namespace tiledb::test {
 
+/**
+ * @return if two filters `left` and `right` represent the same transformations
+ */
+bool is_equivalent_filter(
+    const tiledb::Filter& left, const tiledb::Filter& right);
+
+/**
+ * @return if two filter lists `left` and `right` have the same filters in the
+ * same order
+ */
+bool is_equivalent_filter_list(
+    const tiledb::FilterList& left, const tiledb::FilterList& right);
+
+/**
+ * @return if two attributes `left` and `right` are equivalent
+ */
+bool is_equivalent_attribute(
+    const tiledb::Attribute& left, const tiledb::Attribute& right);
+
 /**
  * @return if two enumerations `left` and `right` are equivalent,
  *         i.e. have the same name, datatype, variants, etc
  */
 bool is_equivalent_enumeration(
     const sm::Enumeration& left, const sm::Enumeration& right);
 
+/**
+ * @return if two enumerations `left` and `right` are equivalent,
+ *         i.e. have the same name, datatype, variants, etc
+ */
+bool is_equivalent_enumeration(
+    const Enumeration& left, const Enumeration& right);
+
 }  // namespace tiledb::test
 
 #endif