Fix dense fragment domains during global order write with maximum fragment size

[rroelke]

Fixes CORE-290.

Report

In CORE-290 a customer reported issues with corrupt arrays after running consolidation. The symptom was memory allocation errors when opening an array. The root cause turned out to be the consolidation itself was writing new fragments where the fragment domain did not match the number of tiles in the fragment.

Root cause analysis

Stepping through a minimal reproducer revealed that consolidation is not at fault at all - instead it is the global order writer max_fragment_size_ field. This field, presumably meant to be used for consolidation only but valid for other writes, instructs the writer to write multiple fragments each under the requested size as necessary. When a write splits its data into multiple fragments this way, nothing need be done for sparse arrays. But dense fragment metadata relies on the domain to determine the number of tiles, and the global order writer did not update the fragment metadata domain when splitting its write into multiple fragments.

This pull request fixes that issue by ensuring that the global order writer updates the fragment metadata domain to reflect what was actually written into that fragment.

This is easier said than done. In CORE-290 I offered four ways we can fix this. The chosen solution:

Design

The fragment metadata domain is a bounding rectangle. This means that the global order writer must split the tiles of its input into fragments at tile boundaries which bisect the bounding rectangle into two smaller bounding rectangles.

To do so, we add a first pass identify_fragment_tile_boundaries which returns a list of tile offsets where new fragments will begin. Upon finishing a fragment, we use that tile offset to determine which rectangle within the target subarray the fragment actually represents, and update the fragment metadata accordingly. We use new functions is_rectangular_domain to determine whether a (start_tile, num_tiles) pair identifies a rectangle, and domain_tile_offset to compute that rectangle.

Much of the complexity comes from the usage of the global order writer which does happen in consolidation: multi-part writes. A user (or a consolidation operation) can set a domain D which it intends to write into, and then actually fill in all of the cells over multiple submit calls which stream in the cells to write. It is not required for these cells to be tile aligned. Because of that, and the need to write rectangle fragments, a single submit cannot always determine whether a tail of tiles belongs to its current fragment or must be deferred to the next. To get around this we keep those tiles in memory in the global_write_state_ and prepend them to the user input in the next submit.

Testing

We add unit tests for is_rectangular_domain and domain_tile_offset, including both example tests as well as rapidcheck properties to assert general claims.

We add tests for the global order writer to make broad claims about what the writer is supposed to do with respect to the max_fragment_size parameter. We add examples and a rapidcheck test to exercise these claims. In particular:

• no fragment should be created larger than this size
• two adjacent fragments must exceed that size (otherwise they could be one fragment)
• fragments correctly populate the domain of the write query

Since the original report originated from consolidation, we also add some consolidation tests (TODO) mimicking the customer input.

TODO

• consolidation test which mimics the customer report
• test the scenario in the FIXME comment in global_order_writer.cc
• test with variable-length attributes
• more careful test of last_tiles_ buffering which observes the amount of data buffered for a 3D write

---

TYPE: BUG
DESC: Fix dense global order writer use of max_fragment_size_

[rroelke]

This is the assert which is tripped without the fix. When the global order writer doesn't update the fragment domain, each fragment gets the whole domain but a fraction of the tiles. tile_num() is computed using the fragment domain, so they don't agree.

[rroelke]

I'm seeing a weird "possible use of uninitialized value" error in release builds, I changed this to try and rule something out. No dice but having a constructor feels better.

[rroelke]

Non-const accessors.

[kounelisagis]

You can perhaps use/modify utils::math::safe_mul.

[rroelke]

The scope of this is broader than it looks at first glance. safe_mul handles floats, this new mul doesn't. I do agree that it would be better to consolidate the functions; I am reluctant to do so in this already very large pull request.

[rroelke]

I moved the definitions used in the file from test/support/src/array_templates.h into test/support/src/array_schema_templates.h.
I did this because the first header had some API stuff which required including tiledb.h which was not compatible with using these structures in a unit test.
The changes to this file follow up on that, applying similar reason to our rapidcheck generators.\

[rroelke]

There is some actual new code in this file, such as the "bound" arguments to make_extent, make_domain, and make_dimension.

[rroelke]

I moved the definitions used in the file from test/support/src/array_templates.h into test/support/src/array_schema_templates.h.
I did this because the first header had some API stuff which required including tiledb.h which was not compatible with using these structures in a unit test.

The changes to this file follow up on that, splitting out some show definitions so we can pull in definitions for tiledb::test::templates::Dimension without anything to do with queries.

[rroelke]

I moved the definitions used in the file from test/support/src/array_templates.h into test/support/src/array_schema_templates.h.
I did this because the first header had some API stuff which required including tiledb.h which was not compatible with using these structures in a unit test.

As such there are only trivial changes to this file, except for the relaxation of the AttributeType concept which previously referred to definitions which are not available in this file. Since it's test code only hopefully we can live with that.

[ypatia]

A first pass

[rroelke]

I have completed all of my TODOs one way or another, as long as CI passes I believe this is ready to go!

[ypatia]

The changes look reasonable to me. I won't say I was able to do a thorough review of 4.5k lines of code in one shot but the effort put in testing is reassuring and the code quality is good so I am not too concerned.
One small worry is about Sparse global order writes, in case we don't have enough test coverage to catch a potential accidental regression there. I would appreciate if you could double check that.
Thanks for solving this complex issue!

[ypatia]

the changes here, in global_write function affect also sparse global order writes, is my understanding correct?
If yes, do we have enough coverage in test to be sure no regression was introduced for sparse arrays?

[rroelke]

I ran tiledb_unit with the following code in the GlobalOrderWriter constructor:

diff --git a/tiledb/sm/query/writers/global_order_writer.cc b/tiledb/sm/query/writers/global_order_writer.cc
index 85abf2281..474fe015a 100644
--- a/tiledb/sm/query/writers/global_order_writer.cc
+++ b/tiledb/sm/query/writers/global_order_writer.cc
@@ -165,6 +165,8 @@ class GlobalOrderWriterException : public StatusException {
 /*   CONSTRUCTORS & DESTRUCTORS   */
 /* ****************************** */
 
+static std::atomic<uint64_t> num_sparse = 0;
+
 GlobalOrderWriter::GlobalOrderWriter(
     stats::Stats* stats,
     shared_ptr<Logger> logger,
(1/2) Discard this hunk from worktree [y,n,q,a,d,j,J,g,/,e,?]? n
@@ -201,6 +203,13 @@ GlobalOrderWriter::GlobalOrderWriter(
         "Failed to initialize global order writer. Global order writes to "
         "ordered attributes are not yet supported.");
   }
+
+  if (!dense()) {
+    num_sparse++;
+    std::cout << "New sparse writer, max_fragment_size = "
+              << max_fragment_size_.value_or(0) << ", " << num_sparse
+              << " writers created" << std::endl;
+  }
 }
 
 GlobalOrderWriter::~GlobalOrderWriter() {

It looks like we create 1000+ sparse global order writers. The cases I saw mostly do not use max fragment size, except consolidation which uses std::numeric_limits<uint64_t>::max(), but I did catch one using a smaller value.

So I believe the test coverage we have should be good enough.