Towards splitting coordinates in separate buffers and files #1454

stavrospapadopoulos · 2019-12-08T17:38:12Z

Towards addressing #93. Split writer algorithms such that the dimension coordinates are split in different buffers and handled separately.

joe-maley

This is a partial review of the header files and small changes in .cc files.

joe-maley · 2019-12-09T17:55:42Z

tiledb/sm/misc/utils.cc

@@ -675,6 +675,20 @@ void expand_mbr(T* mbr, const T* coords, unsigned int dim_num) {
  }
 }

+template <class T>
+void expand_mbr(T* mbr, const std::vector<T*>& coords, uint64_t pos) {


I'll only leave one of these comments because it's a little pedantic, but we should value-const function parameters in the definitions (but not declarations) whenever possible for const correctness. So in this example, I would do T* const mbr and const uint64_t pos.

mbr is modified by the function, perhaps I should move it to the end of the list (that's the convention we've been following). Good point on pos although every other place in the code must be changed similarly. We can start now :).

joe-maley · 2019-12-09T17:58:15Z

tiledb/sm/query/types.h

+  uint64_t original_buffer_var_size_;
+
+  /** Constructor. */
+  CoordBuffer() {


Maybe it's changed in recent years, but list-initializing these class members has been classically faster.

joe-maley · 2019-12-09T17:59:32Z

tiledb/sm/misc/comparators.h

+   * Constructor.
+   *
+   * @param domain The array domain.
+   * @param buff The coordinate buffers, one per dimension, containing the


rename buff to coord_buffs to match the declaration.

joe-maley · 2019-12-09T18:23:26Z

tiledb/sm/misc/comparators.h

+   */
+  GlobalCmp2(const Domain* domain, const std::vector<const void*>& coord_buffs)
+      : domain_(domain)
+      , coord_buffs_(coord_buffs) {


This is unsafe because coord_buffs is a temporary bound to a class member reference (coord_buffs_). The temporary coord_buffs is only guaranteed to persist for the lifetime of this constructor. In other words: coord_buffs_ will NOT extend the lifetime of coord_buffs. When coord_buffs_ is dereferenced outside of the constructor, for example: within GlobalCmp2::operator(), it may be invalid.

From the C++03 standard:

The second context is when a reference is bound to a temporary. The temporary to which the reference is bound or the temporary that is the complete object to a subobject of which the temporary is bound persists for the lifetime of the reference except as specified below. A temporary bound to a reference member in a constructor’s ctor-initializer (§12.6.2 [class.base.init]) persists until the constructor exits.

So we must ensure that the original coord_buffs outlives this GlobalCmp2.
I think the right thing to do here is to change coord_buffs_ from a const-ref to a const raw ptr. The contract is the same, but if the object is freed before we access it, we'll get a segfault instead of a garbage object.

Or, we could copy/move it, but I don't recommend that.

joe-maley · 2019-12-09T18:26:12Z

tiledb/sm/array_schema/domain.h

+   *    - +1 if the first coordinates succeed the second on the cell order
+   */
+  template <class T>
+  static int cell_order_cmp(


What's the motivation for making this static? Each cpp file that includes this header will get its own text copy of this routine. We should make this non-static to avoid binary bloat.

In order to be able to store it in cell_order_cmp_func_, this function needs to be static, otherwise the compiler complains.

joe-maley · 2019-12-09T18:26:52Z

tiledb/sm/array_schema/domain.h

+   *    - +1 if the first coordinates succeed the second on the tile order
+   */
+  template <class T>
+  static int tile_order_cmp(


remove static

Similar to the above.

joe-maley · 2019-12-09T18:32:40Z

tiledb/sm/array_schema/domain.h

+   * - d: The dimension index to compare on.
+   */
+  std::vector<int (*)(
+      const Dimension* dim,


How about typedef or using = either the vector its element template? Just an easy way to re-use it with the tile_order_cmp_func_ declaration and any other place we may need to reuse it.

joe-maley · 2019-12-09T18:36:35Z

tiledb/sm/query/writer.h

@@ -880,6 +1037,29 @@ class Writer {
      const std::string& attribute,
      FragmentMetadata* frag_meta,
      const std::vector<Tile>& tiles) const;
+
+  // TODO: remove


If this is intended to be checked in, could you expand this comment to explain why it should be removed in a future patch?

This will be removed in my next PR over the next couple of days, but I will add a comment.

joe-maley

LGTM overall, just a few nitpick comments that you can address or not. Thanks!

joe-maley · 2019-12-10T14:47:28Z

tiledb/sm/query/writer.cc

@@ -438,15 +446,16 @@ Status Writer::check_buffer_sizes() const {

  auto cell_num = array_schema_->domain()->cell_num(subarray_);
  uint64_t expected_cell_num = 0;
-  for (const auto& attr : attributes_) {
+  for (const auto& it : attr_buffers_) {
+    auto attr = it.first;


const-ref here to avoid unnecessary string copy

joe-maley · 2019-12-10T14:56:54Z

tiledb/sm/query/writer.cc

+    }
+    coord_buffer_sizes_.clear();
+    coord_buffers_.clear();
+    coord_buffers_alloced_ = false;


Is coord_buffers_alloced_ necessary? Couldn't we determine if we allocated the buffers just by checking if coord_buffers_ is or is not empty?

Does this instead indicate ownership of the buffers? Is there a scenario where coord_buffers_ is non-empty but this path wouldn't be responsible for freeing the buffers?

Yes, it is. In the very next PR, the user will be able to set the coordinate buffers separately for each dimension (whereas currently, they must zip all coordinates in a single buffer). We need to have a way to differentiate, which is accomplished by checking that member.

joe-maley · 2019-12-10T15:07:46Z

tiledb/sm/query/writer.cc

+    case Datatype::UINT16:
+      return compute_coords_metadata<uint16_t>(tiles, meta);
+    case Datatype::INT32:
+      return compute_coords_metadata<int>(tiles, meta);


extreme nitpick: let's use int32_t here instead of int. same for unsigned->uint32 below.

joe-maley · 2019-12-10T15:11:26Z

tiledb/sm/query/writer.cc

+  for (const auto& it : attr_buffers_) {
+    auto attr = it.first;
+    if (global_write_state_->attr_cells_written_[attr] != cell_num) {
+      std::cout << global_write_state_->attr_cells_written_[attr] << "\n";


is this a debug statement? we should remove this

joe-maley · 2019-12-10T15:16:10Z

tiledb/sm/query/writer.cc

+  std::string ret;
+  auto dim_num = array_schema_->dim_num();
+
+  ret = "(";


if this will be called frequently, consider a stringstream here to avoid a bunch of string copies.

No, it isn't, but I will change anyway.

…on coordinates are split in different buffers and handled separately.

stavrospapadopoulos added enhancement internals labels Dec 8, 2019

stavrospapadopoulos added this to the 1.8.0 milestone Dec 8, 2019

stavrospapadopoulos requested review from ihnorton, tdenniston and joe-maley December 8, 2019 17:38

stavrospapadopoulos force-pushed the sp/writer_split_coords branch from 06f4a05 to 534050a Compare December 8, 2019 19:27

joe-maley reviewed Dec 9, 2019

View reviewed changes

stavrospapadopoulos force-pushed the sp/writer_split_coords branch from 534050a to ef02c45 Compare December 9, 2019 21:56

joe-maley approved these changes Dec 10, 2019

View reviewed changes

Towards addressing #93. Split writer algorithms such that the dimensi…

cada687

…on coordinates are split in different buffers and handled separately.

stavrospapadopoulos force-pushed the sp/writer_split_coords branch from ef02c45 to cada687 Compare December 10, 2019 15:50

stavrospapadopoulos merged commit e217587 into dev Dec 10, 2019

stavrospapadopoulos mentioned this pull request Mar 21, 2020

Split coordinates #93

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Towards splitting coordinates in separate buffers and files #1454

Towards splitting coordinates in separate buffers and files #1454

stavrospapadopoulos commented Dec 8, 2019

joe-maley left a comment

joe-maley Dec 9, 2019

stavrospapadopoulos Dec 9, 2019

joe-maley Dec 9, 2019

joe-maley Dec 9, 2019

joe-maley Dec 9, 2019

joe-maley Dec 9, 2019

stavrospapadopoulos Dec 9, 2019

joe-maley Dec 9, 2019

stavrospapadopoulos Dec 9, 2019

joe-maley Dec 9, 2019

joe-maley Dec 9, 2019

stavrospapadopoulos Dec 9, 2019

joe-maley left a comment

joe-maley Dec 10, 2019

joe-maley Dec 10, 2019

stavrospapadopoulos Dec 10, 2019

joe-maley Dec 10, 2019

joe-maley Dec 10, 2019

joe-maley Dec 10, 2019

stavrospapadopoulos Dec 10, 2019

Towards splitting coordinates in separate buffers and files #1454

Towards splitting coordinates in separate buffers and files #1454

Conversation

stavrospapadopoulos commented Dec 8, 2019

joe-maley 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

joe-maley 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