Changes from review

+ Testing delta UT alone on fork for sanity
TileDB-Inc · Jul 12, 2023 · 9ca7ca7 · 9ca7ca7
1 parent f8268b6
commit 9ca7ca7
Show file tree

Hide file tree

Showing 6 changed files with 28 additions and 45 deletions.
diff --git a/test/src/unit-capi-sparse_array.cc b/test/src/unit-capi-sparse_array.cc
@@ -2486,14 +2486,6 @@ TEST_CASE_METHOD(
         TILEDB_ROW_MAJOR,
         TILEDB_COL_MAJOR);
   }
-
-  SECTION("- delta compression, row/col-major") {
-    // TODO: refactor for each supported FS.
-    std::string temp_dir = fs_vec_[0]->temp_dir();
-    array_name = temp_dir + ARRAY;
-    check_sorted_reads(
-        array_name, TILEDB_FILTER_DELTA, TILEDB_ROW_MAJOR, TILEDB_COL_MAJOR);
-  }
 }
 
 TEST_CASE_METHOD(

diff --git a/tiledb/sm/filter/filter.cc b/tiledb/sm/filter/filter.cc
@@ -58,17 +58,6 @@ Filter* Filter::clone() const {
   return clone;
 }
 
-bool Filter::accepts_input_datatype(Datatype) const {
-  return true;
-};
-
-void Filter::ensure_compatible_output(const Filter& filter) const {
-  if (filter.type() == FilterType::FILTER_NONE)
-    return;
-
-  this->ensure_accepts_datatype(filter.output_datatype());
-}
-
 Datatype Filter::output_datatype(Datatype) const {
   return Datatype::ANY;
 }
@@ -84,6 +73,10 @@ void Filter::ensure_accepts_datatype(Datatype datatype) const {
   };
 }
 
+bool Filter::accepts_input_datatype(Datatype) const {
+  return true;
+};
+
 Status Filter::get_option(FilterOption option, void* value) const {
   if (value == nullptr)
     return LOG_STATUS(

diff --git a/tiledb/sm/filter/filter.h b/tiledb/sm/filter/filter.h
@@ -77,15 +77,6 @@ class Filter {
   /** Dumps the filter details in ASCII format in the selected output. */
   virtual void dump(FILE* out) const = 0;
 
-  /**
-   * Gets an option from this filter.
-   *
-   * @param option Option whose value to get
-   * @param value Buffer to store the value.
-   * @return Status
-   */
-  Status get_option(FilterOption option, void* value) const;
-
   /**
    * @brief Returns the filter output type
    *
@@ -95,17 +86,12 @@ class Filter {
    */
   virtual Datatype output_datatype(Datatype input_type = Datatype::ANY) const;
 
-  /**
-   * @brief Throws if given filter's output *cannot* be handled by this filter.
-   *
-   */
-  virtual void ensure_compatible_output(const Filter& filter) const;
-
   /**
    * @brief Throws if given data type *cannot* be handled by this filter.
    *
+   * @param datatype Input datatype to check filter compatibility.
    */
-  virtual void ensure_accepts_datatype(Datatype datatype) const;
+  void ensure_accepts_datatype(Datatype datatype) const;
 
   /**
    * @brief Checks if the filter is applicable to the input datatype.
@@ -114,6 +100,15 @@ class Filter {
    */
   virtual bool accepts_input_datatype(Datatype type) const;
 
+  /**
+   * Gets an option from this filter.
+   *
+   * @param option Option whose value to get
+   * @param value Buffer to store the value.
+   * @return Status
+   */
+  Status get_option(FilterOption option, void* value) const;
+
   /**
    * Runs this filter in the "forward" direction (i.e. during write queries).
    *

diff --git a/tiledb/sm/filter/filter_pipeline.cc b/tiledb/sm/filter/filter_pipeline.cc
@@ -104,6 +104,10 @@ void FilterPipeline::check_filter_types(
     const FilterPipeline& pipeline,
     const Datatype first_input_type,
     bool is_var) {
+  if (pipeline.filters_.empty()) {
+    return;
+  }
+
   if ((first_input_type == Datatype::STRING_ASCII ||
        first_input_type == Datatype::STRING_UTF8) &&
       is_var && pipeline.size() > 1) {
@@ -122,12 +126,8 @@ void FilterPipeline::check_filter_types(
   }
 
   // ** Modern checks using Filter output type **
-  for (unsigned i = 0; i < pipeline.size(); ++i) {
-    // If the pipeline has filters, check first filter accepts first input type.
-    if (i == 0) {
-      pipeline.get_filter(i)->ensure_accepts_datatype(first_input_type);
-      continue;
-    }
+  pipeline.get_filter(0)->ensure_accepts_datatype(first_input_type);
+  for (unsigned i = 1; i < pipeline.size(); ++i) {
     ensure_compatible(*pipeline.get_filter(i - 1), *pipeline.get_filter(i));
   }
 }
@@ -260,7 +260,10 @@ Status FilterPipeline::filter_chunks_forward(
           &output_data));
 
       // Final tile type will be the output type of last filter in pipeline.
-      tile.set_datatype(f->output_datatype(tile.type()));
+      auto filtered_type = f->output_datatype(tile.type());
+      if (filtered_type != Datatype::ANY && filtered_type != tile.type())  {
+        tile.set_datatype(filtered_type);
+      }
       input_data.set_read_only(false);
       throw_if_not_ok(input_data.swap(output_data));
       input_metadata.set_read_only(false);

diff --git a/tiledb/sm/filter/xor_filter.h b/tiledb/sm/filter/xor_filter.h
@@ -81,7 +81,7 @@ class XORFilter : public Filter {
    * change output type based on input data. e.g. XORFilter output type is
    * based on byte width of input type.
    */
-  virtual Datatype output_datatype(
+  Datatype output_datatype(
       Datatype input_type = Datatype::ANY) const override;
 
   /**

diff --git a/tiledb/sm/tile/tile.h b/tiledb/sm/tile/tile.h
@@ -79,12 +79,12 @@ class TileBase {
 
   /**
    * Set the tile data type. Used in filter pipeline for filters that modify
-   * tile type. If the filter output datatype is ANY, the datatype is unchanged.
+   * tile type.
    *
    * @param datatype The updated Datatype of the tile.
    */
   inline void set_datatype(Datatype datatype) {
-    type_ = datatype == Datatype::ANY ? type_ : datatype;
+    type_ = datatype;
   }
 
   /** Returns the tile data type. */