From 9ca7ca7b9ab08f8a5bbfa9cb2d2683248b80a56a Mon Sep 17 00:00:00 2001 From: Shaun Reed Date: Wed, 12 Jul 2023 17:27:40 -0400 Subject: [PATCH] Changes from review + Testing delta UT alone on fork for sanity --- test/src/unit-capi-sparse_array.cc | 8 -------- tiledb/sm/filter/filter.cc | 15 ++++----------- tiledb/sm/filter/filter.h | 27 +++++++++++---------------- tiledb/sm/filter/filter_pipeline.cc | 17 ++++++++++------- tiledb/sm/filter/xor_filter.h | 2 +- tiledb/sm/tile/tile.h | 4 ++-- 6 files changed, 28 insertions(+), 45 deletions(-) diff --git a/test/src/unit-capi-sparse_array.cc b/test/src/unit-capi-sparse_array.cc index affeb8ac3500..e415dca1d436 100644 --- 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 index 4bfc87908903..5c7fd453a35a 100644 --- 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 index 69b01ab91234..91e1af868a2d 100644 --- 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 index af8adb4b5375..fad973051b3d 100644 --- 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 index aa0ad6d6ea21..878cbd93f0f0 100644 --- 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 index e0096e0e8d6e..1e09ffc66098 100644 --- 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. */