From 02808ba7e4c13f6d828fceec5fcb2fd41d77c006 Mon Sep 17 00:00:00 2001 From: John Kerl Date: Fri, 24 Jan 2025 16:28:31 -0500 Subject: [PATCH 1/3] Show reasons a POSIX file cannot be read (#5432) Twice in the last few weeks, doing operational work, I've seen the error message ``` tiledb.cc.TileDBError: [TileDB::Array] Error: ArrayDirectory: IO Error: Cannot read from file; Read exceeds file size ``` This error happens (rightly) when `offset + nbytes > file_size`. In order to be helpful, the message should say what those numbers are. If `offset` is 10 and `nbytes` is 5 but `file_size` is 0, that's a data problem to debug; if `nbytes` is in the billions, that's more likely a software problem to debug. In any case, the error message should be helpful. As-is, not only does the message hide useful information, it doesn't even show the URI in question. This forces whoever's doing debug work to rebuild core in debug mode, re-run the repro in the debugger, and print things out in the debugger. It's a more effective use of time for the message to show that debug information. See also [[sc-62426]](https://app.shortcut.com/tiledb-inc/story/62426) for Windows CI issue (@teo-tsirpanis) --- TYPE: IMPROVEMENT DESC: Show file size, offset, nbytes, and URI in file-read error message --------- Co-authored-by: Theodore Tsirpanis --- tiledb/sm/filesystem/mem_filesystem.cc | 8 ++++++-- tiledb/sm/filesystem/posix.cc | 11 +++++++++-- tiledb/sm/filesystem/win.cc | 24 +++++++++++++++++++----- 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/tiledb/sm/filesystem/mem_filesystem.cc b/tiledb/sm/filesystem/mem_filesystem.cc index b1de887c305..439b535d69e 100644 --- a/tiledb/sm/filesystem/mem_filesystem.cc +++ b/tiledb/sm/filesystem/mem_filesystem.cc @@ -183,8 +183,12 @@ class MemFilesystem::File : public MemFilesystem::FSNode { assert(buffer); if (offset + nbytes > size_) - return LOG_STATUS( - Status_MemFSError("Cannot read from file; Read exceeds file size")); + return LOG_STATUS(Status_MemFSError(fmt::format( + "Cannot read from file; Read exceeds file size: offset {} nbytes {} " + "size_ {}", + offset, + nbytes, + size_))); memcpy(buffer, (char*)data_ + offset, nbytes); return Status::Ok(); diff --git a/tiledb/sm/filesystem/posix.cc b/tiledb/sm/filesystem/posix.cc index 8b9cd679605..30e62eeeee9 100644 --- a/tiledb/sm/filesystem/posix.cc +++ b/tiledb/sm/filesystem/posix.cc @@ -273,8 +273,15 @@ void Posix::read( auto path = uri.to_path(); uint64_t file_size; this->file_size(URI(path), &file_size); - if (offset + nbytes > file_size) - throw IOError("Cannot read from file; Read exceeds file size"); + if (offset + nbytes > file_size) { + throw IOError(fmt::format( + "Cannot read from file; Read exceeds file size: offset {}, nbytes {}, " + "file_size {}, URI {}", + offset, + nbytes, + file_size, + uri.to_path())); + } // Open file int fd = open(path.c_str(), O_RDONLY); diff --git a/tiledb/sm/filesystem/win.cc b/tiledb/sm/filesystem/win.cc index 684a5c6ea51..6b851a1f6ff 100644 --- a/tiledb/sm/filesystem/win.cc +++ b/tiledb/sm/filesystem/win.cc @@ -58,6 +58,8 @@ #include "uri.h" #include "win.h" +#include + using namespace tiledb::common; using tiledb::common::filesystem::directory_entry; @@ -462,11 +464,23 @@ Status Win::read( 0) { auto gle = GetLastError(); CloseHandle(file_h); - return LOG_STATUS(Status_IOError( - "Cannot read from file '" + path + "'; File read error " + - (gle != 0 ? get_last_error_msg(gle, "ReadFile") : - "num_bytes_read " + std::to_string(num_bytes_read) + - " != nbyes " + std::to_string(nbytes)))); + + std::string err_msg; + if (gle != 0) { + err_msg = get_last_error_msg(gle, "ReadFile"); + } else { + err_msg = std::string("num_bytes_read ") + + std::to_string(num_bytes_read) + " != nbytes " + + std::to_string(nbytes); + } + + return LOG_STATUS(Status_IOError(fmt::format( + "Cannot read from file '{}'; File read error '{}' offset {} nbytes " + "{}", + path, + err_msg, + offset, + nbytes))); } byte_buffer += num_bytes_read; offset += num_bytes_read; From 2d4327264202b6a023958505a3ebe95e80281028 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Du=C5=A1an=20Baran?= Date: Tue, 28 Jan 2025 14:29:23 +0100 Subject: [PATCH 2/3] Do not set CC/CXX/cmake variables (#5434) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Modify `bootstrap` script to not set compiler, since cmake can do the same. --- TYPE: NO_HISTORY DESC: Do not set CC/CXX variables Co-authored-by: DuĊĦan Baran --- bootstrap | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/bootstrap b/bootstrap index 27d93935c50..5f62e7dad0f 100755 --- a/bootstrap +++ b/bootstrap @@ -196,26 +196,9 @@ if [ "${linkage}" = "shared" ] && [ "${enable_static_tiledb}" = "ON" ]; then die "cannot specify both --linkage=shared and --enable-static-tiledb" fi -# Check clang compiler -if [[ x"${CC}" = x"" ]]; then - CC=gcc -fi - -if [[ x"${CXX}" = x"" ]]; then - CXX=g++ -fi - -cmake=`which cmake` - -if [[ ! -x ${cmake} ]]; then - die "cannot find cmake" -fi - # Configure -${cmake} -DCMAKE_BUILD_TYPE=${build_type} \ +cmake -DCMAKE_BUILD_TYPE=${build_type} \ -DCMAKE_INSTALL_PREFIX="${prefix_dirs}" \ - -DCMAKE_C_COMPILER="${CC}" \ - -DCMAKE_CXX_COMPILER="${CXX}" \ -DCMAKE_PREFIX_PATH="${dependency_dir}" \ -DBUILD_SHARED_LIBS=${build_shared_libs} \ -DTILEDB_ASSERTIONS=${tiledb_assertions} \ From aa9b6464679c5e43f03ce55ca3f5b4ff4c08c6ff Mon Sep 17 00:00:00 2001 From: Beka Davis <31743465+bekadavis9@users.noreply.github.com> Date: Wed, 29 Jan 2025 08:22:14 -0500 Subject: [PATCH 3/3] Patch intermittent failure in SchemaEvolution test. (#5435) Patch intermittent failure in `SchemaEvolution` test, which was caused by the `GENERATE` statements in `Catch2`. [sc-61528] --- TYPE: BUG DESC: Patch intermittent `SchemaEvolution` test failure. --- test/src/unit-cppapi-schema-evolution.cc | 36 +++++++++++++++--------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/test/src/unit-cppapi-schema-evolution.cc b/test/src/unit-cppapi-schema-evolution.cc index 1a80b6d4842..23cb1c810a5 100644 --- a/test/src/unit-cppapi-schema-evolution.cc +++ b/test/src/unit-cppapi-schema-evolution.cc @@ -5,7 +5,7 @@ * * The MIT License * - * @copyright Copyright (c) 2023-2024 TileDB Inc. + * @copyright Copyright (c) 2023-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 @@ -806,21 +806,21 @@ TEST_CASE( } } -TEST_CASE( - "C++ API: SchemaEvolution, drop fixed attribute and add back as var-sized", - "[cppapi][schema][evolution][add][drop]") { +/** + * C++ API: SchemaEvolution, drop fixed attribute and add back as var-sized + * + * Wrapper function for the following test case of the same name. + * This logic has been moved into a function to resolve intermittent failures + * when using Catch2's GENERATE statements inline with code under test + * (https://app.shortcut.com/tiledb-inc/story/61528). + * We should re-evaluate when Catch2 is upgraded. + */ +void test_schema_evolution_drop_fixed_add_var( + tiledb_array_type_t array_type, tiledb_layout_t layout) { test::VFSTestSetup vfs_test_setup; Context ctx{vfs_test_setup.ctx()}; auto array_uri{ vfs_test_setup.array_uri("test_schema_evolution_drop_fixed_add_var")}; - tiledb_array_type_t array_type = TILEDB_DENSE; - bool allows_dups = false; - auto layout = GENERATE(TILEDB_UNORDERED, TILEDB_GLOBAL_ORDER); - - SECTION("sparse") { - array_type = TILEDB_SPARSE; - allows_dups = GENERATE(true, false); - } // Create array Domain domain(ctx); @@ -830,8 +830,7 @@ TEST_CASE( auto b = Attribute::create(ctx, "b"); ArraySchema schema(ctx, array_type); schema.set_domain(domain); - schema.set_allows_dups(allows_dups); - CHECK(allows_dups == schema.allows_dups()); + schema.set_allows_dups(false); schema.add_attribute(a); schema.add_attribute(b); schema.set_cell_order(TILEDB_ROW_MAJOR); @@ -908,6 +907,15 @@ TEST_CASE( Catch::Matchers::Equals(std::vector{1, 2, 3, 4, 5, 6, 7, 8, 9, 10})); } +TEST_CASE( + "C++ API: SchemaEvolution, drop fixed attribute and add back as var-sized", + "[cppapi][schema][evolution][add][drop]") { + test_schema_evolution_drop_fixed_add_var(TILEDB_DENSE, TILEDB_UNORDERED); + test_schema_evolution_drop_fixed_add_var(TILEDB_DENSE, TILEDB_GLOBAL_ORDER); + test_schema_evolution_drop_fixed_add_var(TILEDB_SPARSE, TILEDB_UNORDERED); + test_schema_evolution_drop_fixed_add_var(TILEDB_SPARSE, TILEDB_GLOBAL_ORDER); +} + TEST_CASE( "SchemaEvolution Error Handling Tests", "[cppapi][schema][evolution][errors][rest]") {