Feedback from review

Author: shaunrd0

tiledb/sm/serialization/query.cc

@@ -271,33 +271,35 @@ Subarray subarray_from_capnp(
   auto ranges_reader = reader.getRanges();
 
   uint32_t dim_num = ranges_reader.size();
-  std::vector<RangeSetAndSuperset> range_subset(dim_num);
+  std::vector<RangeSetAndSuperset> range_subset;
+  range_subset.reserve(dim_num);
   std::vector<bool> is_default(dim_num, false);
   for (uint32_t i = 0; i < dim_num; i++) {
     auto range_reader = ranges_reader[i];
-    Datatype type = Datatype::UINT8;
-    throw_if_not_ok(datatype_enum(range_reader.getType(), &type));
+    Datatype type = datatype_enum(range_reader.getType());
     auto dim = array->array_schema_latest().dimension_ptr(i);
 
-    bool implicitly_initialized = range_reader.getHasDefaultRange();
-    range_subset[i] = RangeSetAndSuperset(
-        dim->type(), dim->domain(), implicitly_initialized, coalesce_ranges);
-    is_default[i] = implicitly_initialized;
-    if (range_reader.hasBufferSizes()) {
-      auto ranges = range_buffers_from_capnp(range_reader);
+    is_default[i] = range_reader.getHasDefaultRange();
+    if (is_default[i]) {
       // If the range is implicitly initialized, the RangeSetAndSuperset
       // constructor will initialize the ranges to the domain.
-      if (!implicitly_initialized) {
+      range_subset.emplace_back(
+          type, dim->domain(), is_default[i], coalesce_ranges);
+    } else {
+      std::vector<Range> ranges;
+      if (range_reader.hasBufferSizes()) {
+        ranges = range_buffers_from_capnp(range_reader);
         // Add custom ranges, clearing any implicit ranges previously set.
-        range_subset[i] = RangeSetAndSuperset(
-            dim->type(), dim->domain(), ranges, coalesce_ranges);
+        range_subset.emplace_back(
+            type, dim->domain(), std::move(ranges), coalesce_ranges);
+      } else {
+        // Handle 1.7 style ranges where there is a single range with no sizes
+        auto data = range_reader.getBuffer();
+        auto data_ptr = data.asBytes();
+        ranges.emplace_back(data_ptr.begin(), data.size());
+        range_subset.emplace_back(
+            type, dim->domain(), std::move(ranges), coalesce_ranges);
       }
-    } else {
-      // Handle 1.7 style ranges where there is a single range with no sizes
-      auto data = range_reader.getBuffer();
-      auto data_ptr = data.asBytes();
-      Range range(data_ptr.begin(), data.size());
-      throw_if_not_ok(range_subset[i].add_range_unrestricted(range));
     }
   }
 

tiledb/sm/subarray/range_subset.cc

@@ -135,10 +135,11 @@ RangeSetAndSuperset::RangeSetAndSuperset(
 RangeSetAndSuperset::RangeSetAndSuperset(
     Datatype datatype,
     const Range& superset,
-    std::vector<Range> subset,
+    std::vector<Range>&& subset,
     bool coalesce_ranges)
-    : RangeSetAndSuperset(datatype, superset, false, coalesce_ranges) {
-  ranges_ = std::move(subset);
+    : impl_(range_subset_internals(datatype, superset, coalesce_ranges))
+    , is_implicitly_initialized_(false)
+    , ranges_(subset) {
 }
 
 void RangeSetAndSuperset::sort_and_merge_ranges(
@@ -171,6 +172,13 @@ tuple<Status, optional<std::string>> RangeSetAndSuperset::add_range(
   }
 }
 
+void RangeSetAndSuperset::check_oob() {
+  for (auto& range : ranges_) {
+    impl_->check_range_is_valid(range);
+    throw_if_not_ok(impl_->check_range_is_subset(range));
+  }
+}
+
 Status RangeSetAndSuperset::add_range_unrestricted(const Range& range) {
   if (is_implicitly_initialized_) {
     ranges_.clear();
@@ -179,11 +187,4 @@ Status RangeSetAndSuperset::add_range_unrestricted(const Range& range) {
   return impl_->add_range(ranges_, range);
 }
 
-void RangeSetAndSuperset::check_oob() {
-  for (auto& range : ranges_) {
-    impl_->check_range_is_valid(range);
-    throw_if_not_ok(impl_->check_range_is_subset(range));
-  }
-}
-
 }  // namespace tiledb::sm

tiledb/sm/subarray/range_subset.h

@@ -419,6 +419,9 @@ class TypedRangeSetAndFullsetImpl<std::string, CoalesceAdds>
  */
 class RangeSetAndSuperset {
  public:
+  /** Friend Subarray for calling `add_range_unrestricted` */
+  friend class Subarray;
+
   /* ********************************* */
   /*     CONSTRUCTORS & DESTRUCTORS    */
   /* ********************************* */
@@ -453,7 +456,7 @@ class RangeSetAndSuperset {
   RangeSetAndSuperset(
       Datatype datatype,
       const Range& superset,
-      std::vector<Range> subset,
+      std::vector<Range>&& subset,
       bool coalesce_ranges);
 
   /** Destructor. */
@@ -484,16 +487,6 @@ class RangeSetAndSuperset {
   tuple<Status, optional<std::string>> add_range(
       Range& range, const bool read_range_oob_error = true);
 
-  /**
-   * Adds a range to the range manager without performing any checkes.
-   *
-   * If the ranges are currently implicitly initialized, then they will be
-   * cleared before the new range is added.
-   *
-   * @param range The range to add.
-   */
-  Status add_range_unrestricted(const Range& range);
-
   /**
    * Removes all ranges.
    *
@@ -559,6 +552,10 @@ class RangeSetAndSuperset {
   void sort_and_merge_ranges(ThreadPool* const compute_tp, bool merge = false);
 
  private:
+  /* ********************************* */
+  /*         PRIVATE ATTRIBUTES        */
+  /* ********************************* */
+
   /** Pointer to typed implementation details. */
   shared_ptr<detail::RangeSetAndSupersetImpl> impl_ = nullptr;
 
@@ -571,6 +568,20 @@ class RangeSetAndSuperset {
 
   /** Stored ranges. */
   std::vector<Range> ranges_{};
+
+  /* ********************************* */
+  /*          PRIVATE METHODS          */
+  /* ********************************* */
+
+  /**
+   * Adds a range to the range manager without performing any checkes.
+   *
+   * If the ranges are currently implicitly initialized, then they will be
+   * cleared before the new range is added.
+   *
+   * @param range The range to add.
+   */
+  Status add_range_unrestricted(const Range& range);
 };
 
 }  // namespace tiledb::sm