From e7021c4394cd7735a84cca19453eadd78e1737b7 Mon Sep 17 00:00:00 2001 From: Agisilaos Kounelis <36283973+kounelisagis@users.noreply.github.com> Date: Wed, 8 Jan 2025 20:52:47 +0700 Subject: [PATCH] Bug fix: Ensure correct data retrieval for global cell order in dense reader, avoiding fill values (#5413) `slab_dim` is being calculated incorrectly in `DenseReader::cell_slab_overlaps_range`, causing global cell order reads to return some fill values. After this fix, `slab_dim` remains unchanged for row-major or column-major requests, but for global order requests, cell order is used to determine `slab_dim`. The initial issue was also verified with TileDB-Py, and after applying this fix, the results are as expected. [sc-60301] --- TYPE: NO_HISTORY | BUG DESC: Fix incorrect `slab_dim` calculation in `DenseReader::cell_slab_overlaps_range` to prevent fill values in global cell order reads. --------- Co-authored-by: Ypatia Tsavliri --- test/CMakeLists.txt | 1 + test/src/unit-dense-global-order-reader.cc | 107 +++++++++++++++++++++ tiledb/sm/query/readers/dense_reader.cc | 7 +- 3 files changed, 114 insertions(+), 1 deletion(-) create mode 100644 test/src/unit-dense-global-order-reader.cc diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index a56574c273d..5ca5f4d0d31 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -112,6 +112,7 @@ set(TILEDB_UNIT_TEST_SOURCES src/unit-ctx.cc src/unit-current-domain-rest.cc src/unit-dense-reader.cc + src/unit-dense-global-order-reader.cc src/unit-DenseTiler.cc src/unit-dimension.cc src/unit-duplicates.cc diff --git a/test/src/unit-dense-global-order-reader.cc b/test/src/unit-dense-global-order-reader.cc new file mode 100644 index 00000000000..a49ee02d538 --- /dev/null +++ b/test/src/unit-dense-global-order-reader.cc @@ -0,0 +1,107 @@ +/** + * @file unit-dense-global-order-reader.cc + * + * @section LICENSE + * + * The MIT License + * + * @copyright Copyright (c) 2025 TileDB, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#include +#include "test/support/src/vfs_helpers.h" +#include "tiledb/sm/cpp_api/tiledb" + +using namespace tiledb; + +static void create_array(Context& ctx, const std::string& array_uri); +static void write_array( + Context& ctx, const std::string& array_uri, tiledb_layout_t layout); + +TEST_CASE( + "SC-60301: Read data with global cell order returns fill values", + "[dense-reader][bug][global-cell-order][fixed][sc60301]") { + test::VFSTestSetup vfs_test_setup; + const std::string array_uri = + vfs_test_setup.array_uri("dense_global_cell_order"); + Context ctx{vfs_test_setup.ctx()}; + + // Test setup + create_array(ctx, array_uri); + auto write_layout = + GENERATE(TILEDB_ROW_MAJOR, TILEDB_COL_MAJOR, TILEDB_GLOBAL_ORDER); + write_array(ctx, array_uri, write_layout); + + Array array(ctx, array_uri, TILEDB_READ); + Subarray subarray(ctx, array); + subarray.set_subarray({1, 2, 1, 2}); + std::vector a_read; + a_read.resize(4); + Query query(ctx, array); + query.set_subarray(subarray) + .set_layout(TILEDB_GLOBAL_ORDER) + .set_data_buffer("a", a_read); + + REQUIRE(query.submit() == Query::Status::COMPLETE); + + if (write_layout == TILEDB_ROW_MAJOR) { + REQUIRE(a_read[0] == 1); + REQUIRE(a_read[1] == 3); + REQUIRE(a_read[2] == 2); + REQUIRE(a_read[3] == 4); + } else { + REQUIRE(a_read[0] == 1); + REQUIRE(a_read[1] == 2); + REQUIRE(a_read[2] == 3); + REQUIRE(a_read[3] == 4); + } + + array.close(); +} + +void create_array(Context& ctx, const std::string& array_uri) { + auto obj = Object::object(ctx, array_uri); + if (obj.type() != Object::Type::Invalid) { + Object::remove(ctx, array_uri); + } + + Domain domain(ctx); + domain.add_dimension(Dimension::create(ctx, "d1", {{1, 2}}, 2)) + .add_dimension(Dimension::create(ctx, "d2", {{1, 2}}, 2)); + + // Create the array schema with col-major cell order and tile order + ArraySchema schema(ctx, TILEDB_DENSE); + schema.set_domain(domain) + .set_order({{TILEDB_COL_MAJOR, TILEDB_COL_MAJOR}}) + .add_attribute(Attribute::create(ctx, "a")); + Array::create(ctx, array_uri, schema); +} + +void write_array( + Context& ctx, const std::string& array_uri, tiledb_layout_t layout) { + std::vector data = {1, 2, 3, 4}; + Array array(ctx, array_uri, TILEDB_WRITE); + Query query(ctx, array); + query.set_layout(layout).set_data_buffer("a", data); + REQUIRE(query.submit() == Query::Status::COMPLETE); + query.finalize(); + array.close(); +} diff --git a/tiledb/sm/query/readers/dense_reader.cc b/tiledb/sm/query/readers/dense_reader.cc index f59c6dbe4d9..42c068b0e8d 100644 --- a/tiledb/sm/query/readers/dense_reader.cc +++ b/tiledb/sm/query/readers/dense_reader.cc @@ -1539,7 +1539,12 @@ tuple DenseReader::cell_slab_overlaps_range( const NDRange& ndrange, const std::vector& coords, const uint64_t length) { - const unsigned slab_dim = (layout_ == Layout::COL_MAJOR) ? 0 : dim_num - 1; + const auto cell_order = array_schema_.cell_order(); + const unsigned slab_dim = + (layout_ == Layout::ROW_MAJOR || + (layout_ == Layout::GLOBAL_ORDER && cell_order == Layout::ROW_MAJOR)) ? + dim_num - 1 : + 0; const DimType slab_start = coords[slab_dim]; const DimType slab_end = slab_start + length - 1;