From 3e3ded8d88d22672e7dc03c70639b6e508b06dd8 Mon Sep 17 00:00:00 2001 From: Shivam Dabas Date: Fri, 24 Jul 2026 11:17:10 +0530 Subject: [PATCH 01/13] improve(bq_driver): SQLtables improve --- .../odbc/bq_client_interface/datasets.cc | 3 ++ .../odbc/bq_client_interface/projects.cc | 4 ++ .../cloud/odbc/bq_client_interface/tables.cc | 1 + google/cloud/odbc/bq_client_interface/utils.h | 6 +++ .../bq_driver/internal/odbc_sql_tables.cc | 47 ++++++++++++++----- 5 files changed, 48 insertions(+), 13 deletions(-) diff --git a/google/cloud/odbc/bq_client_interface/datasets.cc b/google/cloud/odbc/bq_client_interface/datasets.cc index 834403ef66..7111f39dea 100644 --- a/google/cloud/odbc/bq_client_interface/datasets.cc +++ b/google/cloud/odbc/bq_client_interface/datasets.cc @@ -13,6 +13,7 @@ // limitations under the License. #include "google/cloud/odbc/bq_client_interface/datasets.h" +#include "google/cloud/odbc/bq_client_interface/utils.h" #include "google/cloud/odbc/internal/status_record_or.h" #include "google/cloud/bigquery/v2/minimal/internal/dataset_client.h" #include "absl/log/log.h" @@ -58,6 +59,7 @@ StatusRecordOr> ListAllDatasets( ListDatasetsRequest request; request.set_project_id(project_id); request.set_all_datasets(true); + request.set_max_results(kMetadataPageSize); LOG(INFO) << "ListAllDatasets:: Request body: " << request.DebugString(""); StreamRange datasets_response = dataset_client.ListDatasets(request, options); @@ -86,6 +88,7 @@ StatusRecordOr> FilterDatasets( request.set_project_id(project_id); request.set_all_datasets(dataset_filter.all); request.set_filter(dataset_filter.filter); + request.set_max_results(kMetadataPageSize); LOG(INFO) << "FilterDatasets:: Request body: " << request.DebugString(""); StreamRange datasets_response = diff --git a/google/cloud/odbc/bq_client_interface/projects.cc b/google/cloud/odbc/bq_client_interface/projects.cc index d21cdc2422..9a11afb6f6 100644 --- a/google/cloud/odbc/bq_client_interface/projects.cc +++ b/google/cloud/odbc/bq_client_interface/projects.cc @@ -14,6 +14,7 @@ #include "google/cloud/odbc/bq_client_interface/projects.h" #include "google/cloud/odbc/bq_client_interface/datasets.h" +#include "google/cloud/odbc/bq_client_interface/utils.h" #include "google/cloud/odbc/internal/sql_state_constants.h" #include "google/cloud/odbc/internal/status_record_or.h" #include "google/cloud/resourcemanager/v3/projects.pb.h" @@ -92,6 +93,7 @@ bool IsProjectBQEnabled(std::string const& bq_project_id, StatusRecordOr> ListAllProjects( ProjectClient& project_client, Options const& options) { ListProjectsRequest request; + request.set_max_results(kMetadataPageSize); StreamRange projects_response = project_client.ListProjects(request, options); @@ -112,6 +114,7 @@ StatusRecordOr GetProject(ProjectClient& project_client, std::string const& project_id, Options const& options) { ListProjectsRequest request; + request.set_max_results(kMetadataPageSize); StreamRange projects_response = project_client.ListProjects(request, options); @@ -252,6 +255,7 @@ StatusRecordOr> FilterProjects( ProjectClient& project_client, std::vector const& project_ids, Options const& options) { ListProjectsRequest request; + request.set_max_results(kMetadataPageSize); StreamRange projects_response = project_client.ListProjects(request, options); diff --git a/google/cloud/odbc/bq_client_interface/tables.cc b/google/cloud/odbc/bq_client_interface/tables.cc index d51903cb0c..654881b8f9 100644 --- a/google/cloud/odbc/bq_client_interface/tables.cc +++ b/google/cloud/odbc/bq_client_interface/tables.cc @@ -69,6 +69,7 @@ StatusRecordOr> ListAllTables( ListTablesRequest request; request.set_project_id(project_id); request.set_dataset_id(dataset_id); + request.set_max_results(kMetadataPageSize); LOG(INFO) << "ListAllTables:: Request body: " << request.DebugString(""); StreamRange tables_response = diff --git a/google/cloud/odbc/bq_client_interface/utils.h b/google/cloud/odbc/bq_client_interface/utils.h index 60ae235e9c..a7d30483e5 100644 --- a/google/cloud/odbc/bq_client_interface/utils.h +++ b/google/cloud/odbc/bq_client_interface/utils.h @@ -17,6 +17,7 @@ #include "absl/log/log.h" #include +#include #include #include #include @@ -27,6 +28,11 @@ struct MaxRetriesOption { using Type = int; }; +// Page size requested from paginated metadata list APIs (projects.list, +// datasets.list, tables.list). When maxResults is unset the BigQuery REST API +// returns small pages (50), which multiplies sequential HTTP round trips +constexpr std::int32_t kMetadataPageSize = 1000; + // URL-encodes a value for safe use as a single path segment. inline std::string UrlEncodeSegment(std::string const& value) { std::ostringstream os; diff --git a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc index 88e7f6db84..05614540d2 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc +++ b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc @@ -444,23 +444,46 @@ StatusRecordOr GetResultSetForTables( }; std::vector tasks; - for (auto const& project_id : project_list) { + std::shared_ptr trace_option = TraceOptions::GetTraceOption(); + int max_threads = trace_option->max_threads; + + if (metadata_id == SQL_TRUE) { // When SQL_ATTR_METADATA_ID is true, the schema argument is an exact // dataset identifier (not a pattern). Skip listing every dataset in the // project via datasets.list - if (metadata_id == SQL_TRUE) { + for (auto const& project_id : project_list) { tasks.push_back({project_id, dataset_filter}); - continue; } - auto datasets_status_record_or = GetFilteredDatasetIds( - bq_client, project_id, dataset_filter, metadata_id); - if (!datasets_status_record_or) { - LOG(ERROR) << "GetResultSetForTables::GetFilteredDatasetIds:: " - << datasets_status_record_or.GetStatusRecord().message; - return datasets_status_record_or.GetStatusRecord(); + } else { + // Enumerate datasets for each project in parallel. Previously this was a + // serial loop issuing one datasets.list REST call per project, which + // dominated SQLTables latency when the catalog pattern matches many + // projects (e.g. catalog = "%"). + auto dataset_task = [&](std::string const& project_id) + -> StatusRecordOr> { + auto datasets_status_record_or = GetFilteredDatasetIds( + bq_client, project_id, dataset_filter, metadata_id); + if (!datasets_status_record_or) { + LOG(ERROR) << "GetResultSetForTables::GetFilteredDatasetIds:: " + << datasets_status_record_or.GetStatusRecord().message; + return datasets_status_record_or.GetStatusRecord(); + } + std::vector project_tasks; + project_tasks.reserve(datasets_status_record_or->size()); + for (auto& dataset_id : *datasets_status_record_or) { + project_tasks.push_back({project_id, std::move(dataset_id)}); + } + return project_tasks; + }; + auto dataset_results_or = + ExecuteParallelTasks>( + max_threads, project_list, dataset_task); + if (!dataset_results_or) { + return dataset_results_or.GetStatusRecord(); } - for (auto const& dataset_id : *datasets_status_record_or) { - tasks.push_back({project_id, dataset_id}); + for (auto& project_tasks : *dataset_results_or) { + tasks.insert(tasks.end(), std::make_move_iterator(project_tasks.begin()), + std::make_move_iterator(project_tasks.end())); } } @@ -491,8 +514,6 @@ StatusRecordOr GetResultSetForTables( }; // 3. Execute tasks using the generic utility - std::shared_ptr trace_option = TraceOptions::GetTraceOption(); - int max_threads = trace_option->max_threads; auto parallel_results_or = ExecuteParallelTasks( max_threads, tasks, parallel_func); From d43b33d3fd6e18458bf0a49f8877b7e984e4965b Mon Sep 17 00:00:00 2001 From: Shivam Dabas Date: Mon, 27 Jul 2026 11:07:36 +0530 Subject: [PATCH 02/13] revert sqltables change --- google/cloud/odbc/bq_client_interface/utils.h | 2 +- .../bq_driver/internal/odbc_sql_tables.cc | 47 +++++-------------- 2 files changed, 14 insertions(+), 35 deletions(-) diff --git a/google/cloud/odbc/bq_client_interface/utils.h b/google/cloud/odbc/bq_client_interface/utils.h index a7d30483e5..227d3bed54 100644 --- a/google/cloud/odbc/bq_client_interface/utils.h +++ b/google/cloud/odbc/bq_client_interface/utils.h @@ -31,7 +31,7 @@ struct MaxRetriesOption { // Page size requested from paginated metadata list APIs (projects.list, // datasets.list, tables.list). When maxResults is unset the BigQuery REST API // returns small pages (50), which multiplies sequential HTTP round trips -constexpr std::int32_t kMetadataPageSize = 1000; +constexpr std::int32_t kMetadataPageSize = 10000; // URL-encodes a value for safe use as a single path segment. inline std::string UrlEncodeSegment(std::string const& value) { diff --git a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc index 05614540d2..88e7f6db84 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc +++ b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc @@ -444,46 +444,23 @@ StatusRecordOr GetResultSetForTables( }; std::vector tasks; - std::shared_ptr trace_option = TraceOptions::GetTraceOption(); - int max_threads = trace_option->max_threads; - - if (metadata_id == SQL_TRUE) { + for (auto const& project_id : project_list) { // When SQL_ATTR_METADATA_ID is true, the schema argument is an exact // dataset identifier (not a pattern). Skip listing every dataset in the // project via datasets.list - for (auto const& project_id : project_list) { + if (metadata_id == SQL_TRUE) { tasks.push_back({project_id, dataset_filter}); + continue; } - } else { - // Enumerate datasets for each project in parallel. Previously this was a - // serial loop issuing one datasets.list REST call per project, which - // dominated SQLTables latency when the catalog pattern matches many - // projects (e.g. catalog = "%"). - auto dataset_task = [&](std::string const& project_id) - -> StatusRecordOr> { - auto datasets_status_record_or = GetFilteredDatasetIds( - bq_client, project_id, dataset_filter, metadata_id); - if (!datasets_status_record_or) { - LOG(ERROR) << "GetResultSetForTables::GetFilteredDatasetIds:: " - << datasets_status_record_or.GetStatusRecord().message; - return datasets_status_record_or.GetStatusRecord(); - } - std::vector project_tasks; - project_tasks.reserve(datasets_status_record_or->size()); - for (auto& dataset_id : *datasets_status_record_or) { - project_tasks.push_back({project_id, std::move(dataset_id)}); - } - return project_tasks; - }; - auto dataset_results_or = - ExecuteParallelTasks>( - max_threads, project_list, dataset_task); - if (!dataset_results_or) { - return dataset_results_or.GetStatusRecord(); + auto datasets_status_record_or = GetFilteredDatasetIds( + bq_client, project_id, dataset_filter, metadata_id); + if (!datasets_status_record_or) { + LOG(ERROR) << "GetResultSetForTables::GetFilteredDatasetIds:: " + << datasets_status_record_or.GetStatusRecord().message; + return datasets_status_record_or.GetStatusRecord(); } - for (auto& project_tasks : *dataset_results_or) { - tasks.insert(tasks.end(), std::make_move_iterator(project_tasks.begin()), - std::make_move_iterator(project_tasks.end())); + for (auto const& dataset_id : *datasets_status_record_or) { + tasks.push_back({project_id, dataset_id}); } } @@ -514,6 +491,8 @@ StatusRecordOr GetResultSetForTables( }; // 3. Execute tasks using the generic utility + std::shared_ptr trace_option = TraceOptions::GetTraceOption(); + int max_threads = trace_option->max_threads; auto parallel_results_or = ExecuteParallelTasks( max_threads, tasks, parallel_func); From 37443da14b7897956649b1a64c9149a6e855a5f3 Mon Sep 17 00:00:00 2001 From: Shivam Dabas Date: Tue, 28 Jul 2026 18:22:18 +0530 Subject: [PATCH 03/13] imrove parallel --- google/cloud/odbc/bq_driver/internal/utils.h | 128 ++++++++++-------- .../odbc/bq_driver/internal/utils_test.cc | 66 +++++++++ 2 files changed, 140 insertions(+), 54 deletions(-) diff --git a/google/cloud/odbc/bq_driver/internal/utils.h b/google/cloud/odbc/bq_driver/internal/utils.h index a557ce6725..fdc77c527f 100644 --- a/google/cloud/odbc/bq_driver/internal/utils.h +++ b/google/cloud/odbc/bq_driver/internal/utils.h @@ -33,6 +33,7 @@ extern HINSTANCE g_hDllInstance; #include "google/cloud/status_or.h" #include "re2/re2.h" #include +#include #include #include #include @@ -42,9 +43,12 @@ extern HINSTANCE g_hDllInstance; #include #include #include +#include +#include #include #include #include +#include #include namespace google::cloud::odbc_bq_driver_internal { @@ -102,75 +106,91 @@ size_t BufferSizeForType(SQLSMALLINT type, size_t requested); // TaskInput: The type of a single item in the input vector. // TaskResult: The type of data returned by the function on success. // -// Returns: A vector containing the results of all successful tasks, or the -// StatusRecord of the first error encountered. +// Returns: A vector containing the results of all successful tasks in input +// order, or the StatusRecord of the first (in input order) error encountered. +// After a task fails, tasks not yet started are skipped, but tasks already +// in flight run to completion before this function returns. +// +// Implemented as a fixed pool of max_threads workers pulling the next input +// from a shared atomic index. Each worker moves on to the next input the +// moment its current task finishes, so one slow task (a straggler REST call) +// never stalls the dispatch of the remaining work -- a previous +// dispatcher-based implementation blocked on the oldest in-flight task to +// free a slot, which serialized the whole batch behind stragglers and made +// catalog enumeration latency vary heavily from run to run. template odbc_internal::StatusRecordOr> ExecuteParallelTasks( std::uint32_t max_threads, std::vector const& inputs, std::function(TaskInput const&)> task_func) { - using FutureType = std::future>; - - std::vector active_futures; - std::vector aggregated_results; - odbc_internal::StatusRecord error_status = odbc_internal::StatusRecord::Ok(); - bool error_occurred = false; - - // Helper to collect result from a finished future - auto process_future = [&](FutureType& f) { - auto result = f.get(); - if (!result) { - // Record the first error that occurs, but keep draining threads - if (!error_occurred) { - error_status = result.GetStatusRecord(); - error_occurred = true; + if (inputs.empty()) { + return std::vector{}; + } + // A misconfigured MaxThreads of 0 previously spun the dispatch loop forever; + // treat it as serial execution. Never spawn more workers than inputs. + std::size_t const num_workers = (std::min)( + static_cast((std::max)(max_threads, std::uint32_t{1})), + inputs.size()); + + // One pre-sized slot per input: workers write disjoint indices, so no + // synchronization is needed, and results come back in input order. + std::vector>> slots( + inputs.size()); + std::atomic next_index{0}; + std::atomic error_occurred{false}; + std::exception_ptr first_exception; + std::mutex exception_mutex; + + auto worker = [&] { + while (!error_occurred.load(std::memory_order_relaxed)) { + std::size_t const index = + next_index.fetch_add(1, std::memory_order_relaxed); + if (index >= inputs.size()) { + return; } - } else if (!error_occurred) { - // Only store results if we are still in a success state - aggregated_results.push_back(std::move(*result)); - } - }; - - for (auto const& input : inputs) { - // If we have hit the thread limit, wait for at least one thread to finish - while (active_futures.size() >= max_threads) { - bool slot_freed = false; - for (auto it = active_futures.begin(); it != active_futures.end();) { - // Check if ready without blocking - if (it->wait_for(std::chrono::milliseconds(1)) == - std::future_status::ready) { - process_future(*it); - it = active_futures.erase(it); - slot_freed = true; - break; // We freed a slot, proceed to launch next task + try { + auto result = task_func(inputs[index]); + if (!result) { + error_occurred.store(true, std::memory_order_relaxed); } - ++it; - } - - // If no threads finished yet, block on the oldest one to prevent spinning - if (!slot_freed && !active_futures.empty()) { - process_future(active_futures.front()); - active_futures.erase(active_futures.begin()); + slots[index] = std::move(result); + } catch (...) { + // Propagate the exception on the calling thread (std::async did this + // via future::get); letting it escape a worker would call terminate. + std::lock_guard lock(exception_mutex); + if (!first_exception) { + first_exception = std::current_exception(); + } + error_occurred.store(true, std::memory_order_relaxed); + return; } } + }; - // If an error occurred previously, we stop spawning new tasks, - // but the loop continues to ensure we drain existing futures safely. - if (!error_occurred) { - active_futures.push_back( - std::async(std::launch::async, task_func, input)); - } + std::vector workers; + workers.reserve(num_workers); + for (std::size_t i = 0; i < num_workers; ++i) { + workers.emplace_back(worker); } - - // Wait for and collect all remaining threads - for (auto& f : active_futures) { - process_future(f); + for (auto& thread : workers) { + thread.join(); } - if (error_occurred) { - return error_status; + if (first_exception) { + std::rethrow_exception(first_exception); } + std::vector aggregated_results; + aggregated_results.reserve(inputs.size()); + for (auto& slot : slots) { + if (!slot.has_value()) { + continue; // Skipped after an error; an errored slot below reports it. + } + if (!*slot) { + return slot->GetStatusRecord(); + } + aggregated_results.push_back(std::move(**slot)); + } return aggregated_results; } diff --git a/google/cloud/odbc/bq_driver/internal/utils_test.cc b/google/cloud/odbc/bq_driver/internal/utils_test.cc index f134d0a0ca..41fe818c6b 100644 --- a/google/cloud/odbc/bq_driver/internal/utils_test.cc +++ b/google/cloud/odbc/bq_driver/internal/utils_test.cc @@ -27,6 +27,7 @@ using ::google::cloud::odbc_internal::SQLStates; using ::google::cloud::odbc_internal::StatusRecord; using ::google::cloud::odbc_internal::StatusRecordOr; using google::cloud::odbc_testing_utils::StatusRecordIs; +using ::testing::ElementsAre; using ::testing::HasSubstr; using ::testing::IsEmpty; using ::testing::UnorderedElementsAre; @@ -1016,4 +1017,69 @@ TEST(ExecuteParallelTasksTest, RespectsSlidingWindow) { EXPECT_GE(duration, (task_count / max_threads) * min_sleep_ms); } +TEST(ExecuteParallelTasksTest, ResultsPreserveInputOrder) { + std::vector inputs = {5, 1, 4, 2, 3}; + + // Sleep inversely to the input so tasks complete in the reverse of their + // submission order; the results must still come back in input order. + auto task = [](int input) -> StatusRecordOr { + std::this_thread::sleep_for(std::chrono::milliseconds(input * 10)); + return input; + }; + + auto result = ExecuteParallelTasks(5, inputs, task); + + ASSERT_STATUS_RECORD_OK(result); + EXPECT_THAT(*result, ElementsAre(5, 1, 4, 2, 3)); +} + +TEST(ExecuteParallelTasksTest, ZeroMaxThreadsRunsSerially) { + // A misconfigured MaxThreads of 0 must not hang; it degrades to serial + // execution. + std::vector inputs = {1, 2, 3}; + auto task = [](int input) -> StatusRecordOr { return input * 2; }; + + auto result = ExecuteParallelTasks(0, inputs, task); + + ASSERT_STATUS_RECORD_OK(result); + EXPECT_THAT(*result, ElementsAre(2, 4, 6)); +} + +TEST(ExecuteParallelTasksTest, StragglerDoesNotStallDispatch) { + // One slow task at the head of the queue must not prevent the other worker + // from chewing through the remaining short tasks. The previous + // dispatcher-based implementation blocked on the oldest in-flight future, + // so this workload took ~straggler + sum(short tasks) instead of + // ~max(straggler, sum(short tasks)). + int const straggler_ms = 300; + int const short_task_ms = 20; + int const short_task_count = 8; + + std::vector inputs; + inputs.push_back(straggler_ms); + for (int i = 0; i < short_task_count; ++i) { + inputs.push_back(short_task_ms); + } + + auto sleeping_task = [](int sleep_ms) -> StatusRecordOr { + std::this_thread::sleep_for(std::chrono::milliseconds(sleep_ms)); + return sleep_ms; + }; + + auto start_time = std::chrono::steady_clock::now(); + auto result = + ExecuteParallelTasks(2, inputs, sleeping_task); + auto duration = std::chrono::duration_cast( + std::chrono::steady_clock::now() - start_time) + .count(); + + ASSERT_STATUS_RECORD_OK(result); + EXPECT_EQ(result->size(), inputs.size()); + // Ideal: worker 1 takes the straggler (300ms) while worker 2 runs the eight + // 20ms tasks (160ms) => ~300ms total. The old scheduler needed ~460ms + // (straggler + all short tasks serialized behind it). Use a generous bound + // to stay robust on loaded CI machines while still distinguishing the two. + EXPECT_LT(duration, straggler_ms + short_task_count * short_task_ms - 60); +} + } // namespace google::cloud::odbc_bq_driver_internal From f76e9e13c7f4a1dbc2dffd2f171e082897fe1f3f Mon Sep 17 00:00:00 2001 From: Shivam Dabas Date: Tue, 28 Jul 2026 18:22:33 +0530 Subject: [PATCH 04/13] Revert "revert sqltables change" This reverts commit 8f7240c1b9c01f796e2b2ce3ea1f6f3c51e842c2. --- google/cloud/odbc/bq_client_interface/utils.h | 2 +- .../bq_driver/internal/odbc_sql_tables.cc | 47 ++++++++++++++----- 2 files changed, 35 insertions(+), 14 deletions(-) diff --git a/google/cloud/odbc/bq_client_interface/utils.h b/google/cloud/odbc/bq_client_interface/utils.h index 227d3bed54..a7d30483e5 100644 --- a/google/cloud/odbc/bq_client_interface/utils.h +++ b/google/cloud/odbc/bq_client_interface/utils.h @@ -31,7 +31,7 @@ struct MaxRetriesOption { // Page size requested from paginated metadata list APIs (projects.list, // datasets.list, tables.list). When maxResults is unset the BigQuery REST API // returns small pages (50), which multiplies sequential HTTP round trips -constexpr std::int32_t kMetadataPageSize = 10000; +constexpr std::int32_t kMetadataPageSize = 1000; // URL-encodes a value for safe use as a single path segment. inline std::string UrlEncodeSegment(std::string const& value) { diff --git a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc index 88e7f6db84..05614540d2 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc +++ b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc @@ -444,23 +444,46 @@ StatusRecordOr GetResultSetForTables( }; std::vector tasks; - for (auto const& project_id : project_list) { + std::shared_ptr trace_option = TraceOptions::GetTraceOption(); + int max_threads = trace_option->max_threads; + + if (metadata_id == SQL_TRUE) { // When SQL_ATTR_METADATA_ID is true, the schema argument is an exact // dataset identifier (not a pattern). Skip listing every dataset in the // project via datasets.list - if (metadata_id == SQL_TRUE) { + for (auto const& project_id : project_list) { tasks.push_back({project_id, dataset_filter}); - continue; } - auto datasets_status_record_or = GetFilteredDatasetIds( - bq_client, project_id, dataset_filter, metadata_id); - if (!datasets_status_record_or) { - LOG(ERROR) << "GetResultSetForTables::GetFilteredDatasetIds:: " - << datasets_status_record_or.GetStatusRecord().message; - return datasets_status_record_or.GetStatusRecord(); + } else { + // Enumerate datasets for each project in parallel. Previously this was a + // serial loop issuing one datasets.list REST call per project, which + // dominated SQLTables latency when the catalog pattern matches many + // projects (e.g. catalog = "%"). + auto dataset_task = [&](std::string const& project_id) + -> StatusRecordOr> { + auto datasets_status_record_or = GetFilteredDatasetIds( + bq_client, project_id, dataset_filter, metadata_id); + if (!datasets_status_record_or) { + LOG(ERROR) << "GetResultSetForTables::GetFilteredDatasetIds:: " + << datasets_status_record_or.GetStatusRecord().message; + return datasets_status_record_or.GetStatusRecord(); + } + std::vector project_tasks; + project_tasks.reserve(datasets_status_record_or->size()); + for (auto& dataset_id : *datasets_status_record_or) { + project_tasks.push_back({project_id, std::move(dataset_id)}); + } + return project_tasks; + }; + auto dataset_results_or = + ExecuteParallelTasks>( + max_threads, project_list, dataset_task); + if (!dataset_results_or) { + return dataset_results_or.GetStatusRecord(); } - for (auto const& dataset_id : *datasets_status_record_or) { - tasks.push_back({project_id, dataset_id}); + for (auto& project_tasks : *dataset_results_or) { + tasks.insert(tasks.end(), std::make_move_iterator(project_tasks.begin()), + std::make_move_iterator(project_tasks.end())); } } @@ -491,8 +514,6 @@ StatusRecordOr GetResultSetForTables( }; // 3. Execute tasks using the generic utility - std::shared_ptr trace_option = TraceOptions::GetTraceOption(); - int max_threads = trace_option->max_threads; auto parallel_results_or = ExecuteParallelTasks( max_threads, tasks, parallel_func); From e31d4233bc997235f9632d8d53debdcf2b8312ab Mon Sep 17 00:00:00 2001 From: Shivam Dabas Date: Tue, 28 Jul 2026 18:22:35 +0530 Subject: [PATCH 05/13] Revert "improve(bq_driver): SQLtables improve" This reverts commit 790b96a90ef2c50a0efdce8f804bb90aa6618a05. --- .../odbc/bq_client_interface/datasets.cc | 3 -- .../odbc/bq_client_interface/projects.cc | 4 -- .../cloud/odbc/bq_client_interface/tables.cc | 1 - google/cloud/odbc/bq_client_interface/utils.h | 6 --- .../bq_driver/internal/odbc_sql_tables.cc | 47 +++++-------------- 5 files changed, 13 insertions(+), 48 deletions(-) diff --git a/google/cloud/odbc/bq_client_interface/datasets.cc b/google/cloud/odbc/bq_client_interface/datasets.cc index 7111f39dea..834403ef66 100644 --- a/google/cloud/odbc/bq_client_interface/datasets.cc +++ b/google/cloud/odbc/bq_client_interface/datasets.cc @@ -13,7 +13,6 @@ // limitations under the License. #include "google/cloud/odbc/bq_client_interface/datasets.h" -#include "google/cloud/odbc/bq_client_interface/utils.h" #include "google/cloud/odbc/internal/status_record_or.h" #include "google/cloud/bigquery/v2/minimal/internal/dataset_client.h" #include "absl/log/log.h" @@ -59,7 +58,6 @@ StatusRecordOr> ListAllDatasets( ListDatasetsRequest request; request.set_project_id(project_id); request.set_all_datasets(true); - request.set_max_results(kMetadataPageSize); LOG(INFO) << "ListAllDatasets:: Request body: " << request.DebugString(""); StreamRange datasets_response = dataset_client.ListDatasets(request, options); @@ -88,7 +86,6 @@ StatusRecordOr> FilterDatasets( request.set_project_id(project_id); request.set_all_datasets(dataset_filter.all); request.set_filter(dataset_filter.filter); - request.set_max_results(kMetadataPageSize); LOG(INFO) << "FilterDatasets:: Request body: " << request.DebugString(""); StreamRange datasets_response = diff --git a/google/cloud/odbc/bq_client_interface/projects.cc b/google/cloud/odbc/bq_client_interface/projects.cc index 9a11afb6f6..d21cdc2422 100644 --- a/google/cloud/odbc/bq_client_interface/projects.cc +++ b/google/cloud/odbc/bq_client_interface/projects.cc @@ -14,7 +14,6 @@ #include "google/cloud/odbc/bq_client_interface/projects.h" #include "google/cloud/odbc/bq_client_interface/datasets.h" -#include "google/cloud/odbc/bq_client_interface/utils.h" #include "google/cloud/odbc/internal/sql_state_constants.h" #include "google/cloud/odbc/internal/status_record_or.h" #include "google/cloud/resourcemanager/v3/projects.pb.h" @@ -93,7 +92,6 @@ bool IsProjectBQEnabled(std::string const& bq_project_id, StatusRecordOr> ListAllProjects( ProjectClient& project_client, Options const& options) { ListProjectsRequest request; - request.set_max_results(kMetadataPageSize); StreamRange projects_response = project_client.ListProjects(request, options); @@ -114,7 +112,6 @@ StatusRecordOr GetProject(ProjectClient& project_client, std::string const& project_id, Options const& options) { ListProjectsRequest request; - request.set_max_results(kMetadataPageSize); StreamRange projects_response = project_client.ListProjects(request, options); @@ -255,7 +252,6 @@ StatusRecordOr> FilterProjects( ProjectClient& project_client, std::vector const& project_ids, Options const& options) { ListProjectsRequest request; - request.set_max_results(kMetadataPageSize); StreamRange projects_response = project_client.ListProjects(request, options); diff --git a/google/cloud/odbc/bq_client_interface/tables.cc b/google/cloud/odbc/bq_client_interface/tables.cc index 654881b8f9..d51903cb0c 100644 --- a/google/cloud/odbc/bq_client_interface/tables.cc +++ b/google/cloud/odbc/bq_client_interface/tables.cc @@ -69,7 +69,6 @@ StatusRecordOr> ListAllTables( ListTablesRequest request; request.set_project_id(project_id); request.set_dataset_id(dataset_id); - request.set_max_results(kMetadataPageSize); LOG(INFO) << "ListAllTables:: Request body: " << request.DebugString(""); StreamRange tables_response = diff --git a/google/cloud/odbc/bq_client_interface/utils.h b/google/cloud/odbc/bq_client_interface/utils.h index a7d30483e5..60ae235e9c 100644 --- a/google/cloud/odbc/bq_client_interface/utils.h +++ b/google/cloud/odbc/bq_client_interface/utils.h @@ -17,7 +17,6 @@ #include "absl/log/log.h" #include -#include #include #include #include @@ -28,11 +27,6 @@ struct MaxRetriesOption { using Type = int; }; -// Page size requested from paginated metadata list APIs (projects.list, -// datasets.list, tables.list). When maxResults is unset the BigQuery REST API -// returns small pages (50), which multiplies sequential HTTP round trips -constexpr std::int32_t kMetadataPageSize = 1000; - // URL-encodes a value for safe use as a single path segment. inline std::string UrlEncodeSegment(std::string const& value) { std::ostringstream os; diff --git a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc index 05614540d2..88e7f6db84 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc +++ b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc @@ -444,46 +444,23 @@ StatusRecordOr GetResultSetForTables( }; std::vector tasks; - std::shared_ptr trace_option = TraceOptions::GetTraceOption(); - int max_threads = trace_option->max_threads; - - if (metadata_id == SQL_TRUE) { + for (auto const& project_id : project_list) { // When SQL_ATTR_METADATA_ID is true, the schema argument is an exact // dataset identifier (not a pattern). Skip listing every dataset in the // project via datasets.list - for (auto const& project_id : project_list) { + if (metadata_id == SQL_TRUE) { tasks.push_back({project_id, dataset_filter}); + continue; } - } else { - // Enumerate datasets for each project in parallel. Previously this was a - // serial loop issuing one datasets.list REST call per project, which - // dominated SQLTables latency when the catalog pattern matches many - // projects (e.g. catalog = "%"). - auto dataset_task = [&](std::string const& project_id) - -> StatusRecordOr> { - auto datasets_status_record_or = GetFilteredDatasetIds( - bq_client, project_id, dataset_filter, metadata_id); - if (!datasets_status_record_or) { - LOG(ERROR) << "GetResultSetForTables::GetFilteredDatasetIds:: " - << datasets_status_record_or.GetStatusRecord().message; - return datasets_status_record_or.GetStatusRecord(); - } - std::vector project_tasks; - project_tasks.reserve(datasets_status_record_or->size()); - for (auto& dataset_id : *datasets_status_record_or) { - project_tasks.push_back({project_id, std::move(dataset_id)}); - } - return project_tasks; - }; - auto dataset_results_or = - ExecuteParallelTasks>( - max_threads, project_list, dataset_task); - if (!dataset_results_or) { - return dataset_results_or.GetStatusRecord(); + auto datasets_status_record_or = GetFilteredDatasetIds( + bq_client, project_id, dataset_filter, metadata_id); + if (!datasets_status_record_or) { + LOG(ERROR) << "GetResultSetForTables::GetFilteredDatasetIds:: " + << datasets_status_record_or.GetStatusRecord().message; + return datasets_status_record_or.GetStatusRecord(); } - for (auto& project_tasks : *dataset_results_or) { - tasks.insert(tasks.end(), std::make_move_iterator(project_tasks.begin()), - std::make_move_iterator(project_tasks.end())); + for (auto const& dataset_id : *datasets_status_record_or) { + tasks.push_back({project_id, dataset_id}); } } @@ -514,6 +491,8 @@ StatusRecordOr GetResultSetForTables( }; // 3. Execute tasks using the generic utility + std::shared_ptr trace_option = TraceOptions::GetTraceOption(); + int max_threads = trace_option->max_threads; auto parallel_results_or = ExecuteParallelTasks( max_threads, tasks, parallel_func); From 3f5a6a420f01ab40f7b4e2d3c293715307226cd0 Mon Sep 17 00:00:00 2001 From: Anshu6250 Date: Wed, 29 Jul 2026 10:35:06 +0530 Subject: [PATCH 06/13] fix: perf benchmark issues --- .github/workflows/windows-benchmark.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/windows-benchmark.yml b/.github/workflows/windows-benchmark.yml index a78ecc4300..201bed6628 100644 --- a/.github/workflows/windows-benchmark.yml +++ b/.github/workflows/windows-benchmark.yml @@ -144,8 +144,8 @@ jobs: echo "Running performance benchmark executable against $BUILD_SHARD..." set +e - "$EXE_PATH" > "$RESULTS_FILE" 2>&1 - TEST_EXIT_CODE=$? + "$EXE_PATH" 2>&1 | tee "$RESULTS_FILE" + TEST_EXIT_CODE=${PIPESTATUS[0]} set -e echo "Uploading results to GCS..." From f4facfd6c6b540214dbe529573eb05ef25bedcfd Mon Sep 17 00:00:00 2001 From: Shivam Dabas Date: Thu, 30 Jul 2026 10:15:37 +0530 Subject: [PATCH 07/13] improve --- .../odbc/bq_client_interface/datasets.cc | 3 + .../odbc/bq_client_interface/projects.cc | 4 + .../cloud/odbc/bq_client_interface/tables.cc | 1 + google/cloud/odbc/bq_client_interface/utils.h | 6 + .../bq_driver/internal/odbc_sql_tables.cc | 121 ++++++++++++++---- .../odbc/bq_driver/internal/odbc_sql_tables.h | 15 +++ .../internal/odbc_sql_tables_test.cc | 43 +++++++ 7 files changed, 168 insertions(+), 25 deletions(-) diff --git a/google/cloud/odbc/bq_client_interface/datasets.cc b/google/cloud/odbc/bq_client_interface/datasets.cc index 834403ef66..7111f39dea 100644 --- a/google/cloud/odbc/bq_client_interface/datasets.cc +++ b/google/cloud/odbc/bq_client_interface/datasets.cc @@ -13,6 +13,7 @@ // limitations under the License. #include "google/cloud/odbc/bq_client_interface/datasets.h" +#include "google/cloud/odbc/bq_client_interface/utils.h" #include "google/cloud/odbc/internal/status_record_or.h" #include "google/cloud/bigquery/v2/minimal/internal/dataset_client.h" #include "absl/log/log.h" @@ -58,6 +59,7 @@ StatusRecordOr> ListAllDatasets( ListDatasetsRequest request; request.set_project_id(project_id); request.set_all_datasets(true); + request.set_max_results(kMetadataPageSize); LOG(INFO) << "ListAllDatasets:: Request body: " << request.DebugString(""); StreamRange datasets_response = dataset_client.ListDatasets(request, options); @@ -86,6 +88,7 @@ StatusRecordOr> FilterDatasets( request.set_project_id(project_id); request.set_all_datasets(dataset_filter.all); request.set_filter(dataset_filter.filter); + request.set_max_results(kMetadataPageSize); LOG(INFO) << "FilterDatasets:: Request body: " << request.DebugString(""); StreamRange datasets_response = diff --git a/google/cloud/odbc/bq_client_interface/projects.cc b/google/cloud/odbc/bq_client_interface/projects.cc index d21cdc2422..9a11afb6f6 100644 --- a/google/cloud/odbc/bq_client_interface/projects.cc +++ b/google/cloud/odbc/bq_client_interface/projects.cc @@ -14,6 +14,7 @@ #include "google/cloud/odbc/bq_client_interface/projects.h" #include "google/cloud/odbc/bq_client_interface/datasets.h" +#include "google/cloud/odbc/bq_client_interface/utils.h" #include "google/cloud/odbc/internal/sql_state_constants.h" #include "google/cloud/odbc/internal/status_record_or.h" #include "google/cloud/resourcemanager/v3/projects.pb.h" @@ -92,6 +93,7 @@ bool IsProjectBQEnabled(std::string const& bq_project_id, StatusRecordOr> ListAllProjects( ProjectClient& project_client, Options const& options) { ListProjectsRequest request; + request.set_max_results(kMetadataPageSize); StreamRange projects_response = project_client.ListProjects(request, options); @@ -112,6 +114,7 @@ StatusRecordOr GetProject(ProjectClient& project_client, std::string const& project_id, Options const& options) { ListProjectsRequest request; + request.set_max_results(kMetadataPageSize); StreamRange projects_response = project_client.ListProjects(request, options); @@ -252,6 +255,7 @@ StatusRecordOr> FilterProjects( ProjectClient& project_client, std::vector const& project_ids, Options const& options) { ListProjectsRequest request; + request.set_max_results(kMetadataPageSize); StreamRange projects_response = project_client.ListProjects(request, options); diff --git a/google/cloud/odbc/bq_client_interface/tables.cc b/google/cloud/odbc/bq_client_interface/tables.cc index d51903cb0c..654881b8f9 100644 --- a/google/cloud/odbc/bq_client_interface/tables.cc +++ b/google/cloud/odbc/bq_client_interface/tables.cc @@ -69,6 +69,7 @@ StatusRecordOr> ListAllTables( ListTablesRequest request; request.set_project_id(project_id); request.set_dataset_id(dataset_id); + request.set_max_results(kMetadataPageSize); LOG(INFO) << "ListAllTables:: Request body: " << request.DebugString(""); StreamRange tables_response = diff --git a/google/cloud/odbc/bq_client_interface/utils.h b/google/cloud/odbc/bq_client_interface/utils.h index 60ae235e9c..a7d30483e5 100644 --- a/google/cloud/odbc/bq_client_interface/utils.h +++ b/google/cloud/odbc/bq_client_interface/utils.h @@ -17,6 +17,7 @@ #include "absl/log/log.h" #include +#include #include #include #include @@ -27,6 +28,11 @@ struct MaxRetriesOption { using Type = int; }; +// Page size requested from paginated metadata list APIs (projects.list, +// datasets.list, tables.list). When maxResults is unset the BigQuery REST API +// returns small pages (50), which multiplies sequential HTTP round trips +constexpr std::int32_t kMetadataPageSize = 1000; + // URL-encodes a value for safe use as a single path segment. inline std::string UrlEncodeSegment(std::string const& value) { std::ostringstream os; diff --git a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc index 88e7f6db84..173366e452 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc +++ b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc @@ -17,6 +17,7 @@ #include "google/cloud/odbc/bq_driver/internal/trace_utils.h" #include "google/cloud/odbc/bq_driver/internal/utils.h" #include +#include namespace google::cloud::odbc_bq_driver_internal { @@ -54,6 +55,37 @@ std::string NormalizeRestTableType(std::string type) { } } // namespace +std::optional LiteralFromOdbcPattern(std::string const& filter, + SQLULEN metadata_id) { + if (metadata_id == SQL_TRUE) { + std::string literal = filter; + RTrim(literal); + return literal; + } + if (filter.empty()) { + return std::nullopt; + } + std::string literal; + literal.reserve(filter.size()); + for (size_t i = 0; i < filter.size(); ++i) { + char c = filter[i]; + if (c == '\\') { + // Escape: the next char is a literal (consume both). A lone trailing + // backslash is dropped, matching CastOdbcRegexToCppRegex(). + if (i + 1 < filter.size()) { + literal.push_back(filter[i + 1]); + ++i; + } + } else if (c == '%' || c == '_') { + // A genuine wildcard: the filter must be expanded by enumeration. + return std::nullopt; + } else { + literal.push_back(c); + } + } + return literal; +} + StatusRecord ValidateInputParameters( const SQLCHAR* catalog_name, SQLSMALLINT catalog_name_len, const SQLCHAR* schema_name, SQLSMALLINT schema_name_len, @@ -116,9 +148,19 @@ StatusRecordOr> GetFilteredDatasetIds( StatusRecordOr> datasets = bq_client.FilterDatasets(project_id, filter, options); if (!datasets) { - LOG(ERROR) << "GetFilteredDatasetIds::FilterDatasets:: " - << datasets.GetStatusRecord().message; - return datasets.GetStatusRecord(); + auto const& status = datasets.GetStatusRecord(); + // A catalog passed as a literal (no wildcard) is used directly without + // first confirming it via projects.list, so an unknown or inaccessible + // project surfaces here. Treat "not found" as an empty catalog rather than + // failing the whole call: the pattern-based path simply would not have + // matched that project. Mirrors GetFilteredTables' 404 handling. + if (status.native_error_code == 404) { + LOG(WARNING) << "GetFilteredDatasetIds:: Skipping project not found: '" + << project_id << "': " << status.message; + return std::vector{}; + } + LOG(ERROR) << "GetFilteredDatasetIds::FilterDatasets:: " << status.message; + return status; } for (auto const& dataset : *datasets) { if ((!metadata_id && datasets_filter == "%") || @@ -416,12 +458,15 @@ StatusRecordOr GetResultSetForTables( std::string const& project_filter, std::string const& dataset_filter, std::string const& table_filter, std::string const& table_type_filter, SQLULEN metadata_id) { - // When SQL_ATTR_METADATA_ID is true, the catalog argument is an exact project - // identifier (not a pattern). Skip enumerating every accessible project via - // projects.list and use the name directly. + // When the catalog argument is an exact project identifier -- either because + // SQL_ATTR_METADATA_ID is true, or because the LIKE pattern contains no + // wildcard -- skip enumerating every accessible project via projects.list and + // use the name directly. projects.list has no server-side filter, so it + // returns every project the caller can see just to select one by name. std::vector project_list; - if (metadata_id == SQL_TRUE) { - project_list = {project_filter}; + if (auto project_literal = + LiteralFromOdbcPattern(project_filter, metadata_id)) { + project_list = {std::move(*project_literal)}; } else { auto projects_status_record_or = GetFilteredProjectIds(bq_client, project_filter, metadata_id); @@ -444,23 +489,51 @@ StatusRecordOr GetResultSetForTables( }; std::vector tasks; - for (auto const& project_id : project_list) { - // When SQL_ATTR_METADATA_ID is true, the schema argument is an exact - // dataset identifier (not a pattern). Skip listing every dataset in the - // project via datasets.list - if (metadata_id == SQL_TRUE) { - tasks.push_back({project_id, dataset_filter}); - continue; + std::shared_ptr trace_option = TraceOptions::GetTraceOption(); + int max_threads = trace_option->max_threads; + + // The schema argument is an exact dataset identifier when + // SQL_ATTR_METADATA_ID is true, or when the LIKE pattern contains no + // wildcard. Computed once: it does not depend on the project. + auto const dataset_literal = + LiteralFromOdbcPattern(dataset_filter, metadata_id); + + if (dataset_literal) { + // For an exact dataset identifier, skip listing every dataset in the + // project via datasets.list and address the dataset directly. + for (auto const& project_id : project_list) { + tasks.push_back({project_id, *dataset_literal}); } - auto datasets_status_record_or = GetFilteredDatasetIds( - bq_client, project_id, dataset_filter, metadata_id); - if (!datasets_status_record_or) { - LOG(ERROR) << "GetResultSetForTables::GetFilteredDatasetIds:: " - << datasets_status_record_or.GetStatusRecord().message; - return datasets_status_record_or.GetStatusRecord(); + } else { + // Enumerate datasets for each project in parallel. A serial loop here + // issues one datasets.list REST call per project, which dominates + // SQLTables latency when the catalog pattern matches many projects + // (e.g. catalog = "%"). + auto dataset_task = [&](std::string const& project_id) + -> StatusRecordOr> { + auto datasets_status_record_or = GetFilteredDatasetIds( + bq_client, project_id, dataset_filter, metadata_id); + if (!datasets_status_record_or) { + LOG(ERROR) << "GetResultSetForTables::GetFilteredDatasetIds:: " + << datasets_status_record_or.GetStatusRecord().message; + return datasets_status_record_or.GetStatusRecord(); + } + std::vector project_tasks; + project_tasks.reserve(datasets_status_record_or->size()); + for (auto& dataset_id : *datasets_status_record_or) { + project_tasks.push_back({project_id, std::move(dataset_id)}); + } + return project_tasks; + }; + auto dataset_results_or = + ExecuteParallelTasks>( + max_threads, project_list, dataset_task); + if (!dataset_results_or) { + return dataset_results_or.GetStatusRecord(); } - for (auto const& dataset_id : *datasets_status_record_or) { - tasks.push_back({project_id, dataset_id}); + for (auto& project_tasks : *dataset_results_or) { + tasks.insert(tasks.end(), std::make_move_iterator(project_tasks.begin()), + std::make_move_iterator(project_tasks.end())); } } @@ -491,8 +564,6 @@ StatusRecordOr GetResultSetForTables( }; // 3. Execute tasks using the generic utility - std::shared_ptr trace_option = TraceOptions::GetTraceOption(); - int max_threads = trace_option->max_threads; auto parallel_results_or = ExecuteParallelTasks( max_threads, tasks, parallel_func); diff --git a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.h b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.h index ae0a45e756..41ce56e93a 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.h +++ b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.h @@ -18,6 +18,7 @@ #include "google/cloud/odbc/bq_driver/internal/odbc_sql_execute_utils.h" #include "google/cloud/odbc/bq_driver/internal/odbc_stmt_handle.h" #include "google/cloud/odbc/internal/status_record_or.h" +#include namespace google::cloud::odbc_bq_driver_internal { @@ -68,6 +69,20 @@ odbc_internal::StatusRecordOr> GetFilteredDatasetIds( ODBCBQClient& bq_client, std::string const& project_id, std::string const& datasets_filter, SQLULEN metadata_id); +// Returns the exact identifier a catalog/schema filter denotes, or nullopt when +// the filter is a genuine search pattern that must be expanded by listing. +// +// When SQL_ATTR_METADATA_ID is SQL_TRUE the argument is an exact identifier and +// is returned verbatim (right-trimmed). Otherwise it is an ODBC LIKE pattern +// and is a literal only when it has no unescaped '%'/'_' metacharacter, in +// which case the unescaped form is returned. Lets SQLTables skip a +// projects.list / datasets.list round trip when a concrete project or dataset +// name was passed -- projects.list in particular has no server-side filter and +// enumerates every visible project. Empty patterns return nullopt to preserve +// the existing (match-nothing) listing path. +std::optional LiteralFromOdbcPattern(std::string const& filter, + SQLULEN metadata_id); + // Construct a query to INFORMATION_SCHEMA.TABLES table depending on input // parameters. Populate 'named_query_params' with named parameters if needed. odbc_internal::StatusRecordOr ConstructQuery( diff --git a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables_test.cc b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables_test.cc index d29c987c3c..c69d460c48 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables_test.cc +++ b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables_test.cc @@ -106,6 +106,49 @@ TEST(ValidateInputParameters, SuccessAllnullsMetadatafalse) { EXPECT_TRUE(status.ok()); } +TEST(LiteralFromOdbcPattern, PlainNameIsLiteral) { + auto literal = LiteralFromOdbcPattern("kirltest", SQL_FALSE); + ASSERT_TRUE(literal.has_value()); + EXPECT_EQ(*literal, "kirltest"); +} + +TEST(LiteralFromOdbcPattern, HyphensAreLiteral) { + auto literal = LiteralFromOdbcPattern("bigquery-devtools-drivers", SQL_FALSE); + ASSERT_TRUE(literal.has_value()); + EXPECT_EQ(*literal, "bigquery-devtools-drivers"); +} + +TEST(LiteralFromOdbcPattern, PercentWildcardIsPattern) { + EXPECT_FALSE(LiteralFromOdbcPattern("%", SQL_FALSE).has_value()); + EXPECT_FALSE(LiteralFromOdbcPattern("%timestamp%", SQL_FALSE).has_value()); +} + +TEST(LiteralFromOdbcPattern, UnderscoreWildcardIsPattern) { + // '_' is an ODBC single-character wildcard, so a name containing it must be + // expanded by listing rather than used as an exact identifier. + EXPECT_FALSE( + LiteralFromOdbcPattern("ODBC_TEST_DATASET", SQL_FALSE).has_value()); +} + +TEST(LiteralFromOdbcPattern, EscapedWildcardsAreLiteral) { + auto literal = LiteralFromOdbcPattern("my\\_dataset\\%", SQL_FALSE); + ASSERT_TRUE(literal.has_value()); + EXPECT_EQ(*literal, "my_dataset%"); +} + +TEST(LiteralFromOdbcPattern, EmptyPatternIsNotLiteral) { + // Preserve the prior (match-nothing) listing path for empty filters. + EXPECT_FALSE(LiteralFromOdbcPattern("", SQL_FALSE).has_value()); +} + +TEST(LiteralFromOdbcPattern, MetadataIdTrueIsAlwaysLiteral) { + // With SQL_ATTR_METADATA_ID true, arguments are exact identifiers even when + // they contain characters that would otherwise be wildcards. + auto literal = LiteralFromOdbcPattern("ODBC_TEST_DATASET ", SQL_TRUE); + ASSERT_TRUE(literal.has_value()); + EXPECT_EQ(*literal, "ODBC_TEST_DATASET"); +} + TEST(ConstructQuery, ConstructWithTwoClausesMetadatafalse) { std::vector named_query_params; From 60ffc890a7750fa1a7121702727b2dba541060b8 Mon Sep 17 00:00:00 2001 From: Shivam Dabas Date: Thu, 30 Jul 2026 12:19:57 +0530 Subject: [PATCH 08/13] update pipeline --- .github/workflows/test-runner.yml | 47 ++++++++++++++++----- .github/workflows/windows-benchmark.yml | 56 +++++++++++++++++++++---- 2 files changed, 86 insertions(+), 17 deletions(-) diff --git a/.github/workflows/test-runner.yml b/.github/workflows/test-runner.yml index b3aa1e7dbd..ac3835dce4 100644 --- a/.github/workflows/test-runner.yml +++ b/.github/workflows/test-runner.yml @@ -111,14 +111,20 @@ jobs: windows-benchmark-existing: name: Windows-Benchmark (Existing Driver) - concurrency: - group: ${{ github.workflow }}-${{ github.ref }}-benchmark-existing - cancel-in-progress: true + # Runs *after* windows-benchmark-bq rather than alongside it. Both shards + # benchmark against the same BigQuery project, so running them concurrently + # made each one's numbers depend on the other's API load -- and when that + # tripped rate limits, retry backoff turned the contention into + # multi-second steps. The dependency deliberately does not require the + # BqDriver shard to succeed: '!cancelled()' plus the windows-cmake guard + # preserves the previous run conditions, so the existing-driver baseline is + # still produced when that shard fails or is skipped. if: | !cancelled() && github.event_name == 'workflow_dispatch' && - inputs.run_benchmark_existing == true - needs: [pre-flight] + inputs.run_benchmark_existing == true && + (needs.windows-cmake.result == 'success' || needs.windows-cmake.result == 'skipped') + needs: [pre-flight, windows-cmake, windows-benchmark-bq] uses: ./.github/workflows/windows-benchmark.yml with: checkout-ref: ${{ needs.pre-flight.outputs.checkout-sha }} @@ -169,9 +175,19 @@ jobs: import re def parse_gtest_output(filepath): - results = {} + # The suite is run several times (--gtest_repeat), so each test + # appears once per repetition. Take the median of its timings. + # + # These benchmarks talk to a live BigQuery service and fan list + # calls out concurrently, so a single run samples the latency + # tail and moved tens of percent between runs for no code + # reason. The median rejects a single outlier repetition, + # including the cold-cache first one. Files with only one run per + # test (a --gtest_repeat=1 run, or a baseline recorded before + # this change) still work: the median of one sample is itself. + samples = {} if not os.path.exists(filepath): - return results + return {} pattern = re.compile(r'\[\s+OK\s+\]\s+(\S+)\s+\(([^)]+)\)') try: @@ -179,11 +195,22 @@ jobs: for line in f: match = pattern.search(line) if match: - test_name = match.group(1) - time_taken = match.group(2) - results[test_name] = time_taken + ms = parse_time_to_ms(match.group(2)) + if ms is not None: + samples.setdefault(match.group(1), []).append(ms) except Exception as e: print(f'Error reading {filepath}: {e}') + + results = {} + for test_name, values in samples.items(): + values.sort() + n = len(values) + median = (values[n // 2] if n % 2 == 1 + else (values[n // 2 - 1] + values[n // 2]) / 2.0) + results[test_name] = f'{median}ms' + if n > 1: + print(f'{test_name}: median={median:.0f}ms of {n} runs ' + f'(min={values[0]:.0f}ms max={values[-1]:.0f}ms)') return results def parse_time_to_ms(time_str): diff --git a/.github/workflows/windows-benchmark.yml b/.github/workflows/windows-benchmark.yml index 201bed6628..02c90cce80 100644 --- a/.github/workflows/windows-benchmark.yml +++ b/.github/workflows/windows-benchmark.yml @@ -12,9 +12,14 @@ on: description: "The driver shard to test (Core or BqDriver)" type: string default: "BqDriver" - secrets: - BUILD_CACHE_KEY: - required: true + benchmark_iterations: + required: false + description: >- + How many times to run the whole suite (gtest --gtest_repeat). The + results table reports the median per test, which rejects a single + outlier run. Total runtime scales linearly with this. + type: string + default: "3" workflow_dispatch: inputs: build_shard: @@ -25,6 +30,11 @@ on: - BqDriver - Core default: "BqDriver" + benchmark_iterations: + description: "How many times to run the suite; median is reported" + required: false + type: string + default: "3" permissions: contents: read @@ -33,11 +43,15 @@ jobs: run-benchmarks: name: Run ODBC Performance Benchmarks (${{ inputs.build_shard }}) runs-on: windows-2022 - timeout-minutes: 60 + # The suite is now repeated benchmark_iterations times so the results + # table can take a median, so it takes proportionally longer. The timeout + # is sized for that and, more importantly, bounds a hang: without it a + # stuck run holds the runner until GitHub's 6-hour default. + timeout-minutes: 180 env: DRIVER_ARCH: x64 BUILD_SHARD: ${{ inputs.build_shard }} - ODBC_GOOGLE_DRIVER_VERSION: 99.99.99 + BENCHMARK_ITERATIONS: ${{ inputs.benchmark_iterations || '3' }} steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: @@ -143,9 +157,37 @@ jobs: EXE_PATH="c:/b/google/cloud/odbc/integration_tests/Release/performance_test.exe" echo "Running performance benchmark executable against $BUILD_SHARD..." + echo " repeats=${BENCHMARK_ITERATIONS}" set +e - "$EXE_PATH" 2>&1 | tee "$RESULTS_FILE" - TEST_EXIT_CODE=${PIPESTATUS[0]} + # Run the suite BENCHMARK_ITERATIONS times and let the results parser + # take the median of each test's timings. A single run is dominated by + # BigQuery/service latency variance, so one sample per test moved tens + # of percent between runs for no code reason. + # + # Deliberately separate processes rather than --gtest_repeat: repeating + # in-process re-allocates and frees SQL_HANDLE_ENV once per test, so + # the Driver Manager loads and unloads the driver DLL on every test. + # Tripling that churn crashed the Simba driver mid-run (abort, exit 3). + # A fresh process per repetition keeps that count identical to a + # single-shot run, and an iteration that dies still leaves the other + # iterations' timings in the results file. + # + # tee -a appends each run; the file is truncated first. tee so progress + # is visible in the live job log -- previously all output went to a + # file only uploaded at the end, making a slow run indistinguishable + # from a hung one. PIPESTATUS keeps the executable's exit code, not + # tee's. + : > "$RESULTS_FILE" + TEST_EXIT_CODE=0 + for i in $(seq 1 "${BENCHMARK_ITERATIONS}"); do + echo "=== benchmark iteration ${i}/${BENCHMARK_ITERATIONS} ===" | tee -a "$RESULTS_FILE" + "$EXE_PATH" 2>&1 | tee -a "$RESULTS_FILE" + RUN_EXIT=${PIPESTATUS[0]} + if [ $RUN_EXIT -ne 0 ]; then + echo "WARNING: iteration ${i} exited with code ${RUN_EXIT}" + TEST_EXIT_CODE=$RUN_EXIT + fi + done set -e echo "Uploading results to GCS..." From f0ed7506243ced25fc20ccb2659375669377c45e Mon Sep 17 00:00:00 2001 From: Shivam Dabas Date: Thu, 30 Jul 2026 15:25:46 +0530 Subject: [PATCH 09/13] test_table --- .github/workflows/windows-benchmark.yml | 13 +- .../odbc/bq_driver/internal/trace_utils.cc | 4 +- .../odbc/bq_driver/internal/trace_utils.h | 8 +- google/cloud/odbc/bq_driver/internal/utils.cc | 13 ++ google/cloud/odbc/bq_driver/internal/utils.h | 144 +++++++++--------- .../odbc/bq_driver/internal/utils_test.cc | 70 +++------ .../odbc/bq_driver/odbc_driver_metadata.cc | 10 +- google/cloud/odbc/bq_driver/odbc_windows.h | 4 +- 8 files changed, 136 insertions(+), 130 deletions(-) diff --git a/.github/workflows/windows-benchmark.yml b/.github/workflows/windows-benchmark.yml index 02c90cce80..28495b7609 100644 --- a/.github/workflows/windows-benchmark.yml +++ b/.github/workflows/windows-benchmark.yml @@ -15,11 +15,12 @@ on: benchmark_iterations: required: false description: >- - How many times to run the whole suite (gtest --gtest_repeat). The - results table reports the median per test, which rejects a single - outlier run. Total runtime scales linearly with this. + How many times to run the whole suite. The results table reports the + median per test, which rejects a single outlier run. Total runtime + scales linearly with this. 5 is enough to see through the ~27% + run-to-run spread the full-catalog enumeration tests exhibit. type: string - default: "3" + default: "5" workflow_dispatch: inputs: build_shard: @@ -34,7 +35,7 @@ on: description: "How many times to run the suite; median is reported" required: false type: string - default: "3" + default: "5" permissions: contents: read @@ -51,7 +52,7 @@ jobs: env: DRIVER_ARCH: x64 BUILD_SHARD: ${{ inputs.build_shard }} - BENCHMARK_ITERATIONS: ${{ inputs.benchmark_iterations || '3' }} + BENCHMARK_ITERATIONS: ${{ inputs.benchmark_iterations || '5' }} steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: diff --git a/google/cloud/odbc/bq_driver/internal/trace_utils.cc b/google/cloud/odbc/bq_driver/internal/trace_utils.cc index 9d9ee8aded..75e6090a2c 100644 --- a/google/cloud/odbc/bq_driver/internal/trace_utils.cc +++ b/google/cloud/odbc/bq_driver/internal/trace_utils.cc @@ -283,7 +283,9 @@ TraceOptions::CreateTraceOptionsFile( int log_level = 0; int log_file_count; int log_file_size; - std::uint32_t max_threads = 8; // default max_threads + // Must track kDefaultMaxThreads: a literal here silently overrides it + // whenever the driver config key has no MaxThreads value. + std::uint32_t max_threads = kDefaultMaxThreads; for (auto const& s : trace_sections) { if (s.first == kLogLevel && !s.second.empty()) { log_level = std::strtol(s.second.c_str(), nullptr, 10); diff --git a/google/cloud/odbc/bq_driver/internal/trace_utils.h b/google/cloud/odbc/bq_driver/internal/trace_utils.h index 6124b12495..76f9847d30 100644 --- a/google/cloud/odbc/bq_driver/internal/trace_utils.h +++ b/google/cloud/odbc/bq_driver/internal/trace_utils.h @@ -41,7 +41,13 @@ inline std::string const kLogPath = "LogPath"; inline std::string const kLogFileCount = "LogFileCount"; inline std::string const kLogFileSize = "LogFileSize"; inline std::string const kMaxThreadsParam = "MaxThreads"; -inline std::uint32_t const kDefaultMaxThreads = 8; +// Concurrency for the catalog metadata fan-out (SQLTables / SQLColumns issue +// one datasets.list / tables.list REST call per project / dataset through +// ExecuteParallelTasks). These threads spend essentially all their time waiting +// on network I/O, not CPU, so full-catalog enumeration wall time is +// ceil(calls / kDefaultMaxThreads) * per_call_latency and the previous value of +// 8 bounded throughput well below what the service will serve. +inline std::uint32_t const kDefaultMaxThreads = 16; inline std::string const kDefaultMaxFiles = "50"; inline std::string const kDefaultMaxSize = "2000"; diff --git a/google/cloud/odbc/bq_driver/internal/utils.cc b/google/cloud/odbc/bq_driver/internal/utils.cc index b9fb984502..b36366020e 100644 --- a/google/cloud/odbc/bq_driver/internal/utils.cc +++ b/google/cloud/odbc/bq_driver/internal/utils.cc @@ -979,6 +979,19 @@ std::string CastOdbcRegexToCppRegex(std::string const& str) { return result; } +std::string EscapeOdbcPattern(std::string const& identifier) { + std::string escaped; + // Worst case: every character needs a backslash. + escaped.reserve(identifier.size() * 2); + for (char c : identifier) { + if (c == '%' || c == '_' || c == '\\') { + escaped.push_back('\\'); + } + escaped.push_back(c); + } + return escaped; +} + std::unique_ptr BuildRegex(std::string filter_pattern, SQLULEN metadata_id) { if (metadata_id == SQL_TRUE) { diff --git a/google/cloud/odbc/bq_driver/internal/utils.h b/google/cloud/odbc/bq_driver/internal/utils.h index fdc77c527f..70214a0ec9 100644 --- a/google/cloud/odbc/bq_driver/internal/utils.h +++ b/google/cloud/odbc/bq_driver/internal/utils.h @@ -33,7 +33,6 @@ extern HINSTANCE g_hDllInstance; #include "google/cloud/status_or.h" #include "re2/re2.h" #include -#include #include #include #include @@ -43,12 +42,9 @@ extern HINSTANCE g_hDllInstance; #include #include #include -#include -#include #include #include #include -#include #include namespace google::cloud::odbc_bq_driver_internal { @@ -106,91 +102,80 @@ size_t BufferSizeForType(SQLSMALLINT type, size_t requested); // TaskInput: The type of a single item in the input vector. // TaskResult: The type of data returned by the function on success. // -// Returns: A vector containing the results of all successful tasks in input -// order, or the StatusRecord of the first (in input order) error encountered. -// After a task fails, tasks not yet started are skipped, but tasks already -// in flight run to completion before this function returns. -// -// Implemented as a fixed pool of max_threads workers pulling the next input -// from a shared atomic index. Each worker moves on to the next input the -// moment its current task finishes, so one slow task (a straggler REST call) -// never stalls the dispatch of the remaining work -- a previous -// dispatcher-based implementation blocked on the oldest in-flight task to -// free a slot, which serialized the whole batch behind stragglers and made -// catalog enumeration latency vary heavily from run to run. +// Returns: A vector containing the results of all successful tasks, or the +// StatusRecord of the first error encountered. template odbc_internal::StatusRecordOr> ExecuteParallelTasks( std::uint32_t max_threads, std::vector const& inputs, std::function(TaskInput const&)> task_func) { - if (inputs.empty()) { - return std::vector{}; - } - // A misconfigured MaxThreads of 0 previously spun the dispatch loop forever; - // treat it as serial execution. Never spawn more workers than inputs. - std::size_t const num_workers = (std::min)( - static_cast((std::max)(max_threads, std::uint32_t{1})), - inputs.size()); - - // One pre-sized slot per input: workers write disjoint indices, so no - // synchronization is needed, and results come back in input order. - std::vector>> slots( - inputs.size()); - std::atomic next_index{0}; - std::atomic error_occurred{false}; - std::exception_ptr first_exception; - std::mutex exception_mutex; - - auto worker = [&] { - while (!error_occurred.load(std::memory_order_relaxed)) { - std::size_t const index = - next_index.fetch_add(1, std::memory_order_relaxed); - if (index >= inputs.size()) { - return; + using FutureType = std::future>; + + std::vector active_futures; + std::vector aggregated_results; + odbc_internal::StatusRecord error_status = odbc_internal::StatusRecord::Ok(); + bool error_occurred = false; + + // Helper to collect result from a finished future + auto process_future = [&](FutureType& f) { + auto result = f.get(); + if (!result) { + // Record the first error that occurs, but keep draining threads + if (!error_occurred) { + error_status = result.GetStatusRecord(); + error_occurred = true; } - try { - auto result = task_func(inputs[index]); - if (!result) { - error_occurred.store(true, std::memory_order_relaxed); - } - slots[index] = std::move(result); - } catch (...) { - // Propagate the exception on the calling thread (std::async did this - // via future::get); letting it escape a worker would call terminate. - std::lock_guard lock(exception_mutex); - if (!first_exception) { - first_exception = std::current_exception(); + } else if (!error_occurred) { + // Only store results if we are still in a success state + aggregated_results.push_back(std::move(*result)); + } + }; + + // A max_threads of 0 (a misconfigured MaxThreads) would make the slot-wait + // loop below spin forever: the condition 0 >= 0 holds while there is no + // future to drain. Treat it as serial execution. + if (max_threads == 0) max_threads = 1; + + for (auto const& input : inputs) { + // If we have hit the thread limit, wait for at least one thread to finish + while (active_futures.size() >= max_threads) { + bool slot_freed = false; + for (auto it = active_futures.begin(); it != active_futures.end();) { + // Check if ready without blocking + if (it->wait_for(std::chrono::milliseconds(1)) == + std::future_status::ready) { + process_future(*it); + it = active_futures.erase(it); + slot_freed = true; + break; // We freed a slot, proceed to launch next task } - error_occurred.store(true, std::memory_order_relaxed); - return; + ++it; + } + + // If no threads finished yet, block on the oldest one to prevent spinning + if (!slot_freed && !active_futures.empty()) { + process_future(active_futures.front()); + active_futures.erase(active_futures.begin()); } } - }; - std::vector workers; - workers.reserve(num_workers); - for (std::size_t i = 0; i < num_workers; ++i) { - workers.emplace_back(worker); - } - for (auto& thread : workers) { - thread.join(); + // If an error occurred previously, we stop spawning new tasks, + // but the loop continues to ensure we drain existing futures safely. + if (!error_occurred) { + active_futures.push_back( + std::async(std::launch::async, task_func, input)); + } } - if (first_exception) { - std::rethrow_exception(first_exception); + // Wait for and collect all remaining threads + for (auto& f : active_futures) { + process_future(f); } - std::vector aggregated_results; - aggregated_results.reserve(inputs.size()); - for (auto& slot : slots) { - if (!slot.has_value()) { - continue; // Skipped after an error; an errored slot below reports it. - } - if (!*slot) { - return slot->GetStatusRecord(); - } - aggregated_results.push_back(std::move(**slot)); + if (error_occurred) { + return error_status; } + return aggregated_results; } @@ -387,6 +372,17 @@ std::string GetDefaultPemFile(); // on some hosts (e.g. SAP HANA with libstdc++/libc++). std::string CastOdbcRegexToCppRegex(std::string const& str); +// Escapes the ODBC LIKE metacharacters in `identifier` -- '%', '_', and the +// escape character '\' itself -- so that it matches only itself when the result +// is interpreted as a search pattern. +// +// Required wherever an exact identifier that did not come from the application +// (e.g. the DSN's configured default dataset) is substituted into an argument +// the ODBC spec defines as a pattern. BigQuery dataset and table names +// routinely contain '_', which would otherwise act as a single-character +// wildcard and match objects the user never configured. +std::string EscapeOdbcPattern(std::string const& identifier); + std::vector SplitTableTypes(std::string const& table_types); std::unique_ptr BuildRegex(std::string filter_pattern, diff --git a/google/cloud/odbc/bq_driver/internal/utils_test.cc b/google/cloud/odbc/bq_driver/internal/utils_test.cc index 41fe818c6b..49351bf86d 100644 --- a/google/cloud/odbc/bq_driver/internal/utils_test.cc +++ b/google/cloud/odbc/bq_driver/internal/utils_test.cc @@ -1017,21 +1017,6 @@ TEST(ExecuteParallelTasksTest, RespectsSlidingWindow) { EXPECT_GE(duration, (task_count / max_threads) * min_sleep_ms); } -TEST(ExecuteParallelTasksTest, ResultsPreserveInputOrder) { - std::vector inputs = {5, 1, 4, 2, 3}; - - // Sleep inversely to the input so tasks complete in the reverse of their - // submission order; the results must still come back in input order. - auto task = [](int input) -> StatusRecordOr { - std::this_thread::sleep_for(std::chrono::milliseconds(input * 10)); - return input; - }; - - auto result = ExecuteParallelTasks(5, inputs, task); - - ASSERT_STATUS_RECORD_OK(result); - EXPECT_THAT(*result, ElementsAre(5, 1, 4, 2, 3)); -} TEST(ExecuteParallelTasksTest, ZeroMaxThreadsRunsSerially) { // A misconfigured MaxThreads of 0 must not hang; it degrades to serial @@ -1045,41 +1030,34 @@ TEST(ExecuteParallelTasksTest, ZeroMaxThreadsRunsSerially) { EXPECT_THAT(*result, ElementsAre(2, 4, 6)); } -TEST(ExecuteParallelTasksTest, StragglerDoesNotStallDispatch) { - // One slow task at the head of the queue must not prevent the other worker - // from chewing through the remaining short tasks. The previous - // dispatcher-based implementation blocked on the oldest in-flight future, - // so this workload took ~straggler + sum(short tasks) instead of - // ~max(straggler, sum(short tasks)). - int const straggler_ms = 300; - int const short_task_ms = 20; - int const short_task_count = 8; - std::vector inputs; - inputs.push_back(straggler_ms); - for (int i = 0; i < short_task_count; ++i) { - inputs.push_back(short_task_ms); - } +TEST(EscapeOdbcPattern, PlainNameUnchanged) { + EXPECT_EQ(EscapeOdbcPattern("kirltest"), "kirltest"); +} - auto sleeping_task = [](int sleep_ms) -> StatusRecordOr { - std::this_thread::sleep_for(std::chrono::milliseconds(sleep_ms)); - return sleep_ms; - }; +TEST(EscapeOdbcPattern, EscapesUnderscores) { + // '_' is an ODBC single-character wildcard; a configured dataset name + // containing it must match only itself. + EXPECT_EQ(EscapeOdbcPattern("ODBC_TEST_DATASET"), + "ODBC\\_TEST\\_DATASET"); +} - auto start_time = std::chrono::steady_clock::now(); - auto result = - ExecuteParallelTasks(2, inputs, sleeping_task); - auto duration = std::chrono::duration_cast( - std::chrono::steady_clock::now() - start_time) - .count(); +TEST(EscapeOdbcPattern, EscapesPercentAndBackslash) { + EXPECT_EQ(EscapeOdbcPattern("a%b"), "a\\%b"); + EXPECT_EQ(EscapeOdbcPattern("a\\b"), "a\\\\b"); +} - ASSERT_STATUS_RECORD_OK(result); - EXPECT_EQ(result->size(), inputs.size()); - // Ideal: worker 1 takes the straggler (300ms) while worker 2 runs the eight - // 20ms tasks (160ms) => ~300ms total. The old scheduler needed ~460ms - // (straggler + all short tasks serialized behind it). Use a generous bound - // to stay robust on loaded CI machines while still distinguishing the two. - EXPECT_LT(duration, straggler_ms + short_task_count * short_task_ms - 60); +TEST(EscapeOdbcPattern, EscapedNameMatchesOnlyItself) { + // The escaped form must compile to a regex that matches the literal name and + // rejects the wildcard expansions the unescaped form would have accepted. + auto regex = BuildRegex(EscapeOdbcPattern("ODBC_TEST_DATASET"), SQL_FALSE); + EXPECT_TRUE(re2::RE2::FullMatch("ODBC_TEST_DATASET", *regex)); + EXPECT_FALSE(re2::RE2::FullMatch("ODBCxTESTyDATASET", *regex)); + EXPECT_FALSE(re2::RE2::FullMatch("ODBC-TEST-DATASET", *regex)); + + // Without escaping, '_' acts as a wildcard -- the behaviour being fixed. + auto unescaped = BuildRegex("ODBC_TEST_DATASET", SQL_FALSE); + EXPECT_TRUE(re2::RE2::FullMatch("ODBCxTESTyDATASET", *unescaped)); } } // namespace google::cloud::odbc_bq_driver_internal diff --git a/google/cloud/odbc/bq_driver/odbc_driver_metadata.cc b/google/cloud/odbc/bq_driver/odbc_driver_metadata.cc index 5d17ce2085..03c27915bc 100644 --- a/google/cloud/odbc/bq_driver/odbc_driver_metadata.cc +++ b/google/cloud/odbc/bq_driver/odbc_driver_metadata.cc @@ -32,6 +32,7 @@ using google::cloud::odbc_bq_driver_internal::ConnectionHandle; using google::cloud::odbc_bq_driver_internal::CreateResultSetForTableTypes; using google::cloud::odbc_bq_driver_internal::DescriptorHandle; using google::cloud::odbc_bq_driver_internal::DescriptorType; +using google::cloud::odbc_bq_driver_internal::EscapeOdbcPattern; using google::cloud::odbc_bq_driver_internal::DSResults; using google::cloud::odbc_bq_driver_internal::FetchBQSQLProceduresData; using google::cloud::odbc_bq_driver_internal::FetchBQTablesData; @@ -457,7 +458,14 @@ SQLRETURN SQLTablesInternal(SQLHSTMT stmt_handle, SQLCHAR* catalog_name, if (!metadata_id && dataset_filter == kMatchAll) { auto const dsn = conn_handle.GetDsn(); if (dsn.filter_tables_on_default_dataset && !dsn.default_dataset.empty()) { - dataset_filter = dsn.default_dataset; + // The configured default dataset is an exact identifier, not something + // the application asked us to pattern-match, so escape any LIKE + // metacharacters in it. Without this a name like "ODBC_TEST_DATASET" + // would treat each '_' as a single-character wildcard: it would report + // tables from datasets that were never configured (e.g. + // "ODBCxTESTyDATASET"), and would force a full datasets.list plus + // client-side matching instead of addressing the dataset directly. + dataset_filter = EscapeOdbcPattern(dsn.default_dataset); } } if (!conn_handle.IsConnected()) { diff --git a/google/cloud/odbc/bq_driver/odbc_windows.h b/google/cloud/odbc/bq_driver/odbc_windows.h index 16a565904c..ebfac96cb1 100644 --- a/google/cloud/odbc/bq_driver/odbc_windows.h +++ b/google/cloud/odbc/bq_driver/odbc_windows.h @@ -73,7 +73,9 @@ std::string const kDefaultLargeDatasetName = "_odbc_temp_tables"; std::string const kDefaultRowsPerBlock = "100000"; std::string const kDefaultStringLength = "16384"; std::string const kDefaultTempExpiration = "3600000"; -std::string const kDefaultMaxThreads = "8"; +// Keep in sync with odbc_bq_driver_internal::kDefaultMaxThreads; this is the +// value pre-filled in the Windows DSN setup dialog. +std::string const kDefaultMaxThreads = "32"; std::string const kDefaultMaxRetries = "6"; } // namespace google::cloud::odbc_bq_driver From 3cd4da2dae3366c3b561ede9adfd0d54c8628b57 Mon Sep 17 00:00:00 2001 From: Anshu6250 Date: Fri, 31 Jul 2026 15:55:33 +0530 Subject: [PATCH 10/13] added perf pipeline itreration changes --- .github/workflows/test-runner.yml | 8 ++++--- .github/workflows/windows-benchmark.yml | 29 +++++++++++-------------- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/.github/workflows/test-runner.yml b/.github/workflows/test-runner.yml index ac3835dce4..3107e75467 100644 --- a/.github/workflows/test-runner.yml +++ b/.github/workflows/test-runner.yml @@ -111,6 +111,9 @@ jobs: windows-benchmark-existing: name: Windows-Benchmark (Existing Driver) + concurrency: + group: ${{ github.workflow }}-${{ github.ref }}-benchmark-existing + cancel-in-progress: true # Runs *after* windows-benchmark-bq rather than alongside it. Both shards # benchmark against the same BigQuery project, so running them concurrently # made each one's numbers depend on the other's API load -- and when that @@ -122,9 +125,8 @@ jobs: if: | !cancelled() && github.event_name == 'workflow_dispatch' && - inputs.run_benchmark_existing == true && - (needs.windows-cmake.result == 'success' || needs.windows-cmake.result == 'skipped') - needs: [pre-flight, windows-cmake, windows-benchmark-bq] + inputs.run_benchmark_existing == true + needs: [pre-flight, windows-benchmark-bq] uses: ./.github/workflows/windows-benchmark.yml with: checkout-ref: ${{ needs.pre-flight.outputs.checkout-sha }} diff --git a/.github/workflows/windows-benchmark.yml b/.github/workflows/windows-benchmark.yml index 28495b7609..d1fe3784fa 100644 --- a/.github/workflows/windows-benchmark.yml +++ b/.github/workflows/windows-benchmark.yml @@ -15,12 +15,14 @@ on: benchmark_iterations: required: false description: >- - How many times to run the whole suite. The results table reports the - median per test, which rejects a single outlier run. Total runtime - scales linearly with this. 5 is enough to see through the ~27% - run-to-run spread the full-catalog enumeration tests exhibit. + How many times to run the whole suite (gtest --gtest_repeat). The + results table reports the median per test, which rejects a single + outlier run. Total runtime scales linearly with this. type: string - default: "5" + default: "3" + secrets: + BUILD_CACHE_KEY: + required: true workflow_dispatch: inputs: build_shard: @@ -35,7 +37,7 @@ on: description: "How many times to run the suite; median is reported" required: false type: string - default: "5" + default: "3" permissions: contents: read @@ -52,7 +54,8 @@ jobs: env: DRIVER_ARCH: x64 BUILD_SHARD: ${{ inputs.build_shard }} - BENCHMARK_ITERATIONS: ${{ inputs.benchmark_iterations || '5' }} + ODBC_GOOGLE_DRIVER_VERSION: 99.99.99 + BENCHMARK_ITERATIONS: ${{ inputs.benchmark_iterations || '3' }} steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: @@ -172,18 +175,12 @@ jobs: # A fresh process per repetition keeps that count identical to a # single-shot run, and an iteration that dies still leaves the other # iterations' timings in the results file. - # - # tee -a appends each run; the file is truncated first. tee so progress - # is visible in the live job log -- previously all output went to a - # file only uploaded at the end, making a slow run indistinguishable - # from a hung one. PIPESTATUS keeps the executable's exit code, not - # tee's. : > "$RESULTS_FILE" TEST_EXIT_CODE=0 for i in $(seq 1 "${BENCHMARK_ITERATIONS}"); do - echo "=== benchmark iteration ${i}/${BENCHMARK_ITERATIONS} ===" | tee -a "$RESULTS_FILE" - "$EXE_PATH" 2>&1 | tee -a "$RESULTS_FILE" - RUN_EXIT=${PIPESTATUS[0]} + echo "=== benchmark iteration ${i}/${BENCHMARK_ITERATIONS} ===" >> "$RESULTS_FILE" + "$EXE_PATH" >> "$RESULTS_FILE" 2>&1 + RUN_EXIT=$? if [ $RUN_EXIT -ne 0 ]; then echo "WARNING: iteration ${i} exited with code ${RUN_EXIT}" TEST_EXIT_CODE=$RUN_EXIT From c0c8f31528505e28c077da41fcf59a2e960eaf82 Mon Sep 17 00:00:00 2001 From: Anshu6250 Date: Fri, 31 Jul 2026 16:22:39 +0530 Subject: [PATCH 11/13] Revert "improve" This reverts commit f4facfd6c6b540214dbe529573eb05ef25bedcfd. --- .../odbc/bq_client_interface/datasets.cc | 3 - .../odbc/bq_client_interface/projects.cc | 4 - .../cloud/odbc/bq_client_interface/tables.cc | 1 - google/cloud/odbc/bq_client_interface/utils.h | 6 - .../bq_driver/internal/odbc_sql_tables.cc | 121 ++++-------------- .../odbc/bq_driver/internal/odbc_sql_tables.h | 15 --- .../internal/odbc_sql_tables_test.cc | 43 ------- .../odbc/bq_driver/internal/utils_test.cc | 5 +- .../odbc/bq_driver/odbc_driver_metadata.cc | 2 +- 9 files changed, 27 insertions(+), 173 deletions(-) diff --git a/google/cloud/odbc/bq_client_interface/datasets.cc b/google/cloud/odbc/bq_client_interface/datasets.cc index 7111f39dea..834403ef66 100644 --- a/google/cloud/odbc/bq_client_interface/datasets.cc +++ b/google/cloud/odbc/bq_client_interface/datasets.cc @@ -13,7 +13,6 @@ // limitations under the License. #include "google/cloud/odbc/bq_client_interface/datasets.h" -#include "google/cloud/odbc/bq_client_interface/utils.h" #include "google/cloud/odbc/internal/status_record_or.h" #include "google/cloud/bigquery/v2/minimal/internal/dataset_client.h" #include "absl/log/log.h" @@ -59,7 +58,6 @@ StatusRecordOr> ListAllDatasets( ListDatasetsRequest request; request.set_project_id(project_id); request.set_all_datasets(true); - request.set_max_results(kMetadataPageSize); LOG(INFO) << "ListAllDatasets:: Request body: " << request.DebugString(""); StreamRange datasets_response = dataset_client.ListDatasets(request, options); @@ -88,7 +86,6 @@ StatusRecordOr> FilterDatasets( request.set_project_id(project_id); request.set_all_datasets(dataset_filter.all); request.set_filter(dataset_filter.filter); - request.set_max_results(kMetadataPageSize); LOG(INFO) << "FilterDatasets:: Request body: " << request.DebugString(""); StreamRange datasets_response = diff --git a/google/cloud/odbc/bq_client_interface/projects.cc b/google/cloud/odbc/bq_client_interface/projects.cc index 9a11afb6f6..d21cdc2422 100644 --- a/google/cloud/odbc/bq_client_interface/projects.cc +++ b/google/cloud/odbc/bq_client_interface/projects.cc @@ -14,7 +14,6 @@ #include "google/cloud/odbc/bq_client_interface/projects.h" #include "google/cloud/odbc/bq_client_interface/datasets.h" -#include "google/cloud/odbc/bq_client_interface/utils.h" #include "google/cloud/odbc/internal/sql_state_constants.h" #include "google/cloud/odbc/internal/status_record_or.h" #include "google/cloud/resourcemanager/v3/projects.pb.h" @@ -93,7 +92,6 @@ bool IsProjectBQEnabled(std::string const& bq_project_id, StatusRecordOr> ListAllProjects( ProjectClient& project_client, Options const& options) { ListProjectsRequest request; - request.set_max_results(kMetadataPageSize); StreamRange projects_response = project_client.ListProjects(request, options); @@ -114,7 +112,6 @@ StatusRecordOr GetProject(ProjectClient& project_client, std::string const& project_id, Options const& options) { ListProjectsRequest request; - request.set_max_results(kMetadataPageSize); StreamRange projects_response = project_client.ListProjects(request, options); @@ -255,7 +252,6 @@ StatusRecordOr> FilterProjects( ProjectClient& project_client, std::vector const& project_ids, Options const& options) { ListProjectsRequest request; - request.set_max_results(kMetadataPageSize); StreamRange projects_response = project_client.ListProjects(request, options); diff --git a/google/cloud/odbc/bq_client_interface/tables.cc b/google/cloud/odbc/bq_client_interface/tables.cc index 654881b8f9..d51903cb0c 100644 --- a/google/cloud/odbc/bq_client_interface/tables.cc +++ b/google/cloud/odbc/bq_client_interface/tables.cc @@ -69,7 +69,6 @@ StatusRecordOr> ListAllTables( ListTablesRequest request; request.set_project_id(project_id); request.set_dataset_id(dataset_id); - request.set_max_results(kMetadataPageSize); LOG(INFO) << "ListAllTables:: Request body: " << request.DebugString(""); StreamRange tables_response = diff --git a/google/cloud/odbc/bq_client_interface/utils.h b/google/cloud/odbc/bq_client_interface/utils.h index a7d30483e5..60ae235e9c 100644 --- a/google/cloud/odbc/bq_client_interface/utils.h +++ b/google/cloud/odbc/bq_client_interface/utils.h @@ -17,7 +17,6 @@ #include "absl/log/log.h" #include -#include #include #include #include @@ -28,11 +27,6 @@ struct MaxRetriesOption { using Type = int; }; -// Page size requested from paginated metadata list APIs (projects.list, -// datasets.list, tables.list). When maxResults is unset the BigQuery REST API -// returns small pages (50), which multiplies sequential HTTP round trips -constexpr std::int32_t kMetadataPageSize = 1000; - // URL-encodes a value for safe use as a single path segment. inline std::string UrlEncodeSegment(std::string const& value) { std::ostringstream os; diff --git a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc index 173366e452..88e7f6db84 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc +++ b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc @@ -17,7 +17,6 @@ #include "google/cloud/odbc/bq_driver/internal/trace_utils.h" #include "google/cloud/odbc/bq_driver/internal/utils.h" #include -#include namespace google::cloud::odbc_bq_driver_internal { @@ -55,37 +54,6 @@ std::string NormalizeRestTableType(std::string type) { } } // namespace -std::optional LiteralFromOdbcPattern(std::string const& filter, - SQLULEN metadata_id) { - if (metadata_id == SQL_TRUE) { - std::string literal = filter; - RTrim(literal); - return literal; - } - if (filter.empty()) { - return std::nullopt; - } - std::string literal; - literal.reserve(filter.size()); - for (size_t i = 0; i < filter.size(); ++i) { - char c = filter[i]; - if (c == '\\') { - // Escape: the next char is a literal (consume both). A lone trailing - // backslash is dropped, matching CastOdbcRegexToCppRegex(). - if (i + 1 < filter.size()) { - literal.push_back(filter[i + 1]); - ++i; - } - } else if (c == '%' || c == '_') { - // A genuine wildcard: the filter must be expanded by enumeration. - return std::nullopt; - } else { - literal.push_back(c); - } - } - return literal; -} - StatusRecord ValidateInputParameters( const SQLCHAR* catalog_name, SQLSMALLINT catalog_name_len, const SQLCHAR* schema_name, SQLSMALLINT schema_name_len, @@ -148,19 +116,9 @@ StatusRecordOr> GetFilteredDatasetIds( StatusRecordOr> datasets = bq_client.FilterDatasets(project_id, filter, options); if (!datasets) { - auto const& status = datasets.GetStatusRecord(); - // A catalog passed as a literal (no wildcard) is used directly without - // first confirming it via projects.list, so an unknown or inaccessible - // project surfaces here. Treat "not found" as an empty catalog rather than - // failing the whole call: the pattern-based path simply would not have - // matched that project. Mirrors GetFilteredTables' 404 handling. - if (status.native_error_code == 404) { - LOG(WARNING) << "GetFilteredDatasetIds:: Skipping project not found: '" - << project_id << "': " << status.message; - return std::vector{}; - } - LOG(ERROR) << "GetFilteredDatasetIds::FilterDatasets:: " << status.message; - return status; + LOG(ERROR) << "GetFilteredDatasetIds::FilterDatasets:: " + << datasets.GetStatusRecord().message; + return datasets.GetStatusRecord(); } for (auto const& dataset : *datasets) { if ((!metadata_id && datasets_filter == "%") || @@ -458,15 +416,12 @@ StatusRecordOr GetResultSetForTables( std::string const& project_filter, std::string const& dataset_filter, std::string const& table_filter, std::string const& table_type_filter, SQLULEN metadata_id) { - // When the catalog argument is an exact project identifier -- either because - // SQL_ATTR_METADATA_ID is true, or because the LIKE pattern contains no - // wildcard -- skip enumerating every accessible project via projects.list and - // use the name directly. projects.list has no server-side filter, so it - // returns every project the caller can see just to select one by name. + // When SQL_ATTR_METADATA_ID is true, the catalog argument is an exact project + // identifier (not a pattern). Skip enumerating every accessible project via + // projects.list and use the name directly. std::vector project_list; - if (auto project_literal = - LiteralFromOdbcPattern(project_filter, metadata_id)) { - project_list = {std::move(*project_literal)}; + if (metadata_id == SQL_TRUE) { + project_list = {project_filter}; } else { auto projects_status_record_or = GetFilteredProjectIds(bq_client, project_filter, metadata_id); @@ -489,51 +444,23 @@ StatusRecordOr GetResultSetForTables( }; std::vector tasks; - std::shared_ptr trace_option = TraceOptions::GetTraceOption(); - int max_threads = trace_option->max_threads; - - // The schema argument is an exact dataset identifier when - // SQL_ATTR_METADATA_ID is true, or when the LIKE pattern contains no - // wildcard. Computed once: it does not depend on the project. - auto const dataset_literal = - LiteralFromOdbcPattern(dataset_filter, metadata_id); - - if (dataset_literal) { - // For an exact dataset identifier, skip listing every dataset in the - // project via datasets.list and address the dataset directly. - for (auto const& project_id : project_list) { - tasks.push_back({project_id, *dataset_literal}); + for (auto const& project_id : project_list) { + // When SQL_ATTR_METADATA_ID is true, the schema argument is an exact + // dataset identifier (not a pattern). Skip listing every dataset in the + // project via datasets.list + if (metadata_id == SQL_TRUE) { + tasks.push_back({project_id, dataset_filter}); + continue; } - } else { - // Enumerate datasets for each project in parallel. A serial loop here - // issues one datasets.list REST call per project, which dominates - // SQLTables latency when the catalog pattern matches many projects - // (e.g. catalog = "%"). - auto dataset_task = [&](std::string const& project_id) - -> StatusRecordOr> { - auto datasets_status_record_or = GetFilteredDatasetIds( - bq_client, project_id, dataset_filter, metadata_id); - if (!datasets_status_record_or) { - LOG(ERROR) << "GetResultSetForTables::GetFilteredDatasetIds:: " - << datasets_status_record_or.GetStatusRecord().message; - return datasets_status_record_or.GetStatusRecord(); - } - std::vector project_tasks; - project_tasks.reserve(datasets_status_record_or->size()); - for (auto& dataset_id : *datasets_status_record_or) { - project_tasks.push_back({project_id, std::move(dataset_id)}); - } - return project_tasks; - }; - auto dataset_results_or = - ExecuteParallelTasks>( - max_threads, project_list, dataset_task); - if (!dataset_results_or) { - return dataset_results_or.GetStatusRecord(); + auto datasets_status_record_or = GetFilteredDatasetIds( + bq_client, project_id, dataset_filter, metadata_id); + if (!datasets_status_record_or) { + LOG(ERROR) << "GetResultSetForTables::GetFilteredDatasetIds:: " + << datasets_status_record_or.GetStatusRecord().message; + return datasets_status_record_or.GetStatusRecord(); } - for (auto& project_tasks : *dataset_results_or) { - tasks.insert(tasks.end(), std::make_move_iterator(project_tasks.begin()), - std::make_move_iterator(project_tasks.end())); + for (auto const& dataset_id : *datasets_status_record_or) { + tasks.push_back({project_id, dataset_id}); } } @@ -564,6 +491,8 @@ StatusRecordOr GetResultSetForTables( }; // 3. Execute tasks using the generic utility + std::shared_ptr trace_option = TraceOptions::GetTraceOption(); + int max_threads = trace_option->max_threads; auto parallel_results_or = ExecuteParallelTasks( max_threads, tasks, parallel_func); diff --git a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.h b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.h index 41ce56e93a..ae0a45e756 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.h +++ b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.h @@ -18,7 +18,6 @@ #include "google/cloud/odbc/bq_driver/internal/odbc_sql_execute_utils.h" #include "google/cloud/odbc/bq_driver/internal/odbc_stmt_handle.h" #include "google/cloud/odbc/internal/status_record_or.h" -#include namespace google::cloud::odbc_bq_driver_internal { @@ -69,20 +68,6 @@ odbc_internal::StatusRecordOr> GetFilteredDatasetIds( ODBCBQClient& bq_client, std::string const& project_id, std::string const& datasets_filter, SQLULEN metadata_id); -// Returns the exact identifier a catalog/schema filter denotes, or nullopt when -// the filter is a genuine search pattern that must be expanded by listing. -// -// When SQL_ATTR_METADATA_ID is SQL_TRUE the argument is an exact identifier and -// is returned verbatim (right-trimmed). Otherwise it is an ODBC LIKE pattern -// and is a literal only when it has no unescaped '%'/'_' metacharacter, in -// which case the unescaped form is returned. Lets SQLTables skip a -// projects.list / datasets.list round trip when a concrete project or dataset -// name was passed -- projects.list in particular has no server-side filter and -// enumerates every visible project. Empty patterns return nullopt to preserve -// the existing (match-nothing) listing path. -std::optional LiteralFromOdbcPattern(std::string const& filter, - SQLULEN metadata_id); - // Construct a query to INFORMATION_SCHEMA.TABLES table depending on input // parameters. Populate 'named_query_params' with named parameters if needed. odbc_internal::StatusRecordOr ConstructQuery( diff --git a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables_test.cc b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables_test.cc index c69d460c48..d29c987c3c 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables_test.cc +++ b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables_test.cc @@ -106,49 +106,6 @@ TEST(ValidateInputParameters, SuccessAllnullsMetadatafalse) { EXPECT_TRUE(status.ok()); } -TEST(LiteralFromOdbcPattern, PlainNameIsLiteral) { - auto literal = LiteralFromOdbcPattern("kirltest", SQL_FALSE); - ASSERT_TRUE(literal.has_value()); - EXPECT_EQ(*literal, "kirltest"); -} - -TEST(LiteralFromOdbcPattern, HyphensAreLiteral) { - auto literal = LiteralFromOdbcPattern("bigquery-devtools-drivers", SQL_FALSE); - ASSERT_TRUE(literal.has_value()); - EXPECT_EQ(*literal, "bigquery-devtools-drivers"); -} - -TEST(LiteralFromOdbcPattern, PercentWildcardIsPattern) { - EXPECT_FALSE(LiteralFromOdbcPattern("%", SQL_FALSE).has_value()); - EXPECT_FALSE(LiteralFromOdbcPattern("%timestamp%", SQL_FALSE).has_value()); -} - -TEST(LiteralFromOdbcPattern, UnderscoreWildcardIsPattern) { - // '_' is an ODBC single-character wildcard, so a name containing it must be - // expanded by listing rather than used as an exact identifier. - EXPECT_FALSE( - LiteralFromOdbcPattern("ODBC_TEST_DATASET", SQL_FALSE).has_value()); -} - -TEST(LiteralFromOdbcPattern, EscapedWildcardsAreLiteral) { - auto literal = LiteralFromOdbcPattern("my\\_dataset\\%", SQL_FALSE); - ASSERT_TRUE(literal.has_value()); - EXPECT_EQ(*literal, "my_dataset%"); -} - -TEST(LiteralFromOdbcPattern, EmptyPatternIsNotLiteral) { - // Preserve the prior (match-nothing) listing path for empty filters. - EXPECT_FALSE(LiteralFromOdbcPattern("", SQL_FALSE).has_value()); -} - -TEST(LiteralFromOdbcPattern, MetadataIdTrueIsAlwaysLiteral) { - // With SQL_ATTR_METADATA_ID true, arguments are exact identifiers even when - // they contain characters that would otherwise be wildcards. - auto literal = LiteralFromOdbcPattern("ODBC_TEST_DATASET ", SQL_TRUE); - ASSERT_TRUE(literal.has_value()); - EXPECT_EQ(*literal, "ODBC_TEST_DATASET"); -} - TEST(ConstructQuery, ConstructWithTwoClausesMetadatafalse) { std::vector named_query_params; diff --git a/google/cloud/odbc/bq_driver/internal/utils_test.cc b/google/cloud/odbc/bq_driver/internal/utils_test.cc index 49351bf86d..943bb634d1 100644 --- a/google/cloud/odbc/bq_driver/internal/utils_test.cc +++ b/google/cloud/odbc/bq_driver/internal/utils_test.cc @@ -1017,7 +1017,6 @@ TEST(ExecuteParallelTasksTest, RespectsSlidingWindow) { EXPECT_GE(duration, (task_count / max_threads) * min_sleep_ms); } - TEST(ExecuteParallelTasksTest, ZeroMaxThreadsRunsSerially) { // A misconfigured MaxThreads of 0 must not hang; it degrades to serial // execution. @@ -1030,7 +1029,6 @@ TEST(ExecuteParallelTasksTest, ZeroMaxThreadsRunsSerially) { EXPECT_THAT(*result, ElementsAre(2, 4, 6)); } - TEST(EscapeOdbcPattern, PlainNameUnchanged) { EXPECT_EQ(EscapeOdbcPattern("kirltest"), "kirltest"); } @@ -1038,8 +1036,7 @@ TEST(EscapeOdbcPattern, PlainNameUnchanged) { TEST(EscapeOdbcPattern, EscapesUnderscores) { // '_' is an ODBC single-character wildcard; a configured dataset name // containing it must match only itself. - EXPECT_EQ(EscapeOdbcPattern("ODBC_TEST_DATASET"), - "ODBC\\_TEST\\_DATASET"); + EXPECT_EQ(EscapeOdbcPattern("ODBC_TEST_DATASET"), "ODBC\\_TEST\\_DATASET"); } TEST(EscapeOdbcPattern, EscapesPercentAndBackslash) { diff --git a/google/cloud/odbc/bq_driver/odbc_driver_metadata.cc b/google/cloud/odbc/bq_driver/odbc_driver_metadata.cc index 03c27915bc..3b6ae05f64 100644 --- a/google/cloud/odbc/bq_driver/odbc_driver_metadata.cc +++ b/google/cloud/odbc/bq_driver/odbc_driver_metadata.cc @@ -32,8 +32,8 @@ using google::cloud::odbc_bq_driver_internal::ConnectionHandle; using google::cloud::odbc_bq_driver_internal::CreateResultSetForTableTypes; using google::cloud::odbc_bq_driver_internal::DescriptorHandle; using google::cloud::odbc_bq_driver_internal::DescriptorType; -using google::cloud::odbc_bq_driver_internal::EscapeOdbcPattern; using google::cloud::odbc_bq_driver_internal::DSResults; +using google::cloud::odbc_bq_driver_internal::EscapeOdbcPattern; using google::cloud::odbc_bq_driver_internal::FetchBQSQLProceduresData; using google::cloud::odbc_bq_driver_internal::FetchBQTablesData; using google::cloud::odbc_bq_driver_internal::FetchForeignKeysFromDataSource; From 20a6ad334d5181accbdd3b8d233583f1e8268b35 Mon Sep 17 00:00:00 2001 From: Anshu6250 Date: Fri, 31 Jul 2026 18:01:42 +0530 Subject: [PATCH 12/13] revert pipeline --- .github/workflows/test-runner.yml | 41 ++++------------------ .github/workflows/windows-benchmark.yml | 46 ++----------------------- 2 files changed, 9 insertions(+), 78 deletions(-) diff --git a/.github/workflows/test-runner.yml b/.github/workflows/test-runner.yml index 3107e75467..b3aa1e7dbd 100644 --- a/.github/workflows/test-runner.yml +++ b/.github/workflows/test-runner.yml @@ -114,19 +114,11 @@ jobs: concurrency: group: ${{ github.workflow }}-${{ github.ref }}-benchmark-existing cancel-in-progress: true - # Runs *after* windows-benchmark-bq rather than alongside it. Both shards - # benchmark against the same BigQuery project, so running them concurrently - # made each one's numbers depend on the other's API load -- and when that - # tripped rate limits, retry backoff turned the contention into - # multi-second steps. The dependency deliberately does not require the - # BqDriver shard to succeed: '!cancelled()' plus the windows-cmake guard - # preserves the previous run conditions, so the existing-driver baseline is - # still produced when that shard fails or is skipped. if: | !cancelled() && github.event_name == 'workflow_dispatch' && inputs.run_benchmark_existing == true - needs: [pre-flight, windows-benchmark-bq] + needs: [pre-flight] uses: ./.github/workflows/windows-benchmark.yml with: checkout-ref: ${{ needs.pre-flight.outputs.checkout-sha }} @@ -177,19 +169,9 @@ jobs: import re def parse_gtest_output(filepath): - # The suite is run several times (--gtest_repeat), so each test - # appears once per repetition. Take the median of its timings. - # - # These benchmarks talk to a live BigQuery service and fan list - # calls out concurrently, so a single run samples the latency - # tail and moved tens of percent between runs for no code - # reason. The median rejects a single outlier repetition, - # including the cold-cache first one. Files with only one run per - # test (a --gtest_repeat=1 run, or a baseline recorded before - # this change) still work: the median of one sample is itself. - samples = {} + results = {} if not os.path.exists(filepath): - return {} + return results pattern = re.compile(r'\[\s+OK\s+\]\s+(\S+)\s+\(([^)]+)\)') try: @@ -197,22 +179,11 @@ jobs: for line in f: match = pattern.search(line) if match: - ms = parse_time_to_ms(match.group(2)) - if ms is not None: - samples.setdefault(match.group(1), []).append(ms) + test_name = match.group(1) + time_taken = match.group(2) + results[test_name] = time_taken except Exception as e: print(f'Error reading {filepath}: {e}') - - results = {} - for test_name, values in samples.items(): - values.sort() - n = len(values) - median = (values[n // 2] if n % 2 == 1 - else (values[n // 2 - 1] + values[n // 2]) / 2.0) - results[test_name] = f'{median}ms' - if n > 1: - print(f'{test_name}: median={median:.0f}ms of {n} runs ' - f'(min={values[0]:.0f}ms max={values[-1]:.0f}ms)') return results def parse_time_to_ms(time_str): diff --git a/.github/workflows/windows-benchmark.yml b/.github/workflows/windows-benchmark.yml index d1fe3784fa..a78ecc4300 100644 --- a/.github/workflows/windows-benchmark.yml +++ b/.github/workflows/windows-benchmark.yml @@ -12,14 +12,6 @@ on: description: "The driver shard to test (Core or BqDriver)" type: string default: "BqDriver" - benchmark_iterations: - required: false - description: >- - How many times to run the whole suite (gtest --gtest_repeat). The - results table reports the median per test, which rejects a single - outlier run. Total runtime scales linearly with this. - type: string - default: "3" secrets: BUILD_CACHE_KEY: required: true @@ -33,11 +25,6 @@ on: - BqDriver - Core default: "BqDriver" - benchmark_iterations: - description: "How many times to run the suite; median is reported" - required: false - type: string - default: "3" permissions: contents: read @@ -46,16 +33,11 @@ jobs: run-benchmarks: name: Run ODBC Performance Benchmarks (${{ inputs.build_shard }}) runs-on: windows-2022 - # The suite is now repeated benchmark_iterations times so the results - # table can take a median, so it takes proportionally longer. The timeout - # is sized for that and, more importantly, bounds a hang: without it a - # stuck run holds the runner until GitHub's 6-hour default. - timeout-minutes: 180 + timeout-minutes: 60 env: DRIVER_ARCH: x64 BUILD_SHARD: ${{ inputs.build_shard }} ODBC_GOOGLE_DRIVER_VERSION: 99.99.99 - BENCHMARK_ITERATIONS: ${{ inputs.benchmark_iterations || '3' }} steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: @@ -161,31 +143,9 @@ jobs: EXE_PATH="c:/b/google/cloud/odbc/integration_tests/Release/performance_test.exe" echo "Running performance benchmark executable against $BUILD_SHARD..." - echo " repeats=${BENCHMARK_ITERATIONS}" set +e - # Run the suite BENCHMARK_ITERATIONS times and let the results parser - # take the median of each test's timings. A single run is dominated by - # BigQuery/service latency variance, so one sample per test moved tens - # of percent between runs for no code reason. - # - # Deliberately separate processes rather than --gtest_repeat: repeating - # in-process re-allocates and frees SQL_HANDLE_ENV once per test, so - # the Driver Manager loads and unloads the driver DLL on every test. - # Tripling that churn crashed the Simba driver mid-run (abort, exit 3). - # A fresh process per repetition keeps that count identical to a - # single-shot run, and an iteration that dies still leaves the other - # iterations' timings in the results file. - : > "$RESULTS_FILE" - TEST_EXIT_CODE=0 - for i in $(seq 1 "${BENCHMARK_ITERATIONS}"); do - echo "=== benchmark iteration ${i}/${BENCHMARK_ITERATIONS} ===" >> "$RESULTS_FILE" - "$EXE_PATH" >> "$RESULTS_FILE" 2>&1 - RUN_EXIT=$? - if [ $RUN_EXIT -ne 0 ]; then - echo "WARNING: iteration ${i} exited with code ${RUN_EXIT}" - TEST_EXIT_CODE=$RUN_EXIT - fi - done + "$EXE_PATH" > "$RESULTS_FILE" 2>&1 + TEST_EXIT_CODE=$? set -e echo "Uploading results to GCS..." From 3d4e170635b4fa0309cf0ae6e1a5ae022d2768d8 Mon Sep 17 00:00:00 2001 From: Anshu6250 Date: Fri, 31 Jul 2026 18:08:51 +0530 Subject: [PATCH 13/13] Revert "Revert "improve"" This reverts commit c0c8f31528505e28c077da41fcf59a2e960eaf82. --- .../odbc/bq_client_interface/datasets.cc | 3 + .../odbc/bq_client_interface/projects.cc | 4 + .../cloud/odbc/bq_client_interface/tables.cc | 1 + google/cloud/odbc/bq_client_interface/utils.h | 6 + .../bq_driver/internal/odbc_sql_tables.cc | 121 ++++++++++++++---- .../odbc/bq_driver/internal/odbc_sql_tables.h | 15 +++ .../internal/odbc_sql_tables_test.cc | 43 +++++++ .../odbc/bq_driver/internal/utils_test.cc | 5 +- .../odbc/bq_driver/odbc_driver_metadata.cc | 2 +- 9 files changed, 173 insertions(+), 27 deletions(-) diff --git a/google/cloud/odbc/bq_client_interface/datasets.cc b/google/cloud/odbc/bq_client_interface/datasets.cc index 834403ef66..7111f39dea 100644 --- a/google/cloud/odbc/bq_client_interface/datasets.cc +++ b/google/cloud/odbc/bq_client_interface/datasets.cc @@ -13,6 +13,7 @@ // limitations under the License. #include "google/cloud/odbc/bq_client_interface/datasets.h" +#include "google/cloud/odbc/bq_client_interface/utils.h" #include "google/cloud/odbc/internal/status_record_or.h" #include "google/cloud/bigquery/v2/minimal/internal/dataset_client.h" #include "absl/log/log.h" @@ -58,6 +59,7 @@ StatusRecordOr> ListAllDatasets( ListDatasetsRequest request; request.set_project_id(project_id); request.set_all_datasets(true); + request.set_max_results(kMetadataPageSize); LOG(INFO) << "ListAllDatasets:: Request body: " << request.DebugString(""); StreamRange datasets_response = dataset_client.ListDatasets(request, options); @@ -86,6 +88,7 @@ StatusRecordOr> FilterDatasets( request.set_project_id(project_id); request.set_all_datasets(dataset_filter.all); request.set_filter(dataset_filter.filter); + request.set_max_results(kMetadataPageSize); LOG(INFO) << "FilterDatasets:: Request body: " << request.DebugString(""); StreamRange datasets_response = diff --git a/google/cloud/odbc/bq_client_interface/projects.cc b/google/cloud/odbc/bq_client_interface/projects.cc index d21cdc2422..9a11afb6f6 100644 --- a/google/cloud/odbc/bq_client_interface/projects.cc +++ b/google/cloud/odbc/bq_client_interface/projects.cc @@ -14,6 +14,7 @@ #include "google/cloud/odbc/bq_client_interface/projects.h" #include "google/cloud/odbc/bq_client_interface/datasets.h" +#include "google/cloud/odbc/bq_client_interface/utils.h" #include "google/cloud/odbc/internal/sql_state_constants.h" #include "google/cloud/odbc/internal/status_record_or.h" #include "google/cloud/resourcemanager/v3/projects.pb.h" @@ -92,6 +93,7 @@ bool IsProjectBQEnabled(std::string const& bq_project_id, StatusRecordOr> ListAllProjects( ProjectClient& project_client, Options const& options) { ListProjectsRequest request; + request.set_max_results(kMetadataPageSize); StreamRange projects_response = project_client.ListProjects(request, options); @@ -112,6 +114,7 @@ StatusRecordOr GetProject(ProjectClient& project_client, std::string const& project_id, Options const& options) { ListProjectsRequest request; + request.set_max_results(kMetadataPageSize); StreamRange projects_response = project_client.ListProjects(request, options); @@ -252,6 +255,7 @@ StatusRecordOr> FilterProjects( ProjectClient& project_client, std::vector const& project_ids, Options const& options) { ListProjectsRequest request; + request.set_max_results(kMetadataPageSize); StreamRange projects_response = project_client.ListProjects(request, options); diff --git a/google/cloud/odbc/bq_client_interface/tables.cc b/google/cloud/odbc/bq_client_interface/tables.cc index d51903cb0c..654881b8f9 100644 --- a/google/cloud/odbc/bq_client_interface/tables.cc +++ b/google/cloud/odbc/bq_client_interface/tables.cc @@ -69,6 +69,7 @@ StatusRecordOr> ListAllTables( ListTablesRequest request; request.set_project_id(project_id); request.set_dataset_id(dataset_id); + request.set_max_results(kMetadataPageSize); LOG(INFO) << "ListAllTables:: Request body: " << request.DebugString(""); StreamRange tables_response = diff --git a/google/cloud/odbc/bq_client_interface/utils.h b/google/cloud/odbc/bq_client_interface/utils.h index 60ae235e9c..a7d30483e5 100644 --- a/google/cloud/odbc/bq_client_interface/utils.h +++ b/google/cloud/odbc/bq_client_interface/utils.h @@ -17,6 +17,7 @@ #include "absl/log/log.h" #include +#include #include #include #include @@ -27,6 +28,11 @@ struct MaxRetriesOption { using Type = int; }; +// Page size requested from paginated metadata list APIs (projects.list, +// datasets.list, tables.list). When maxResults is unset the BigQuery REST API +// returns small pages (50), which multiplies sequential HTTP round trips +constexpr std::int32_t kMetadataPageSize = 1000; + // URL-encodes a value for safe use as a single path segment. inline std::string UrlEncodeSegment(std::string const& value) { std::ostringstream os; diff --git a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc index 88e7f6db84..173366e452 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc +++ b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc @@ -17,6 +17,7 @@ #include "google/cloud/odbc/bq_driver/internal/trace_utils.h" #include "google/cloud/odbc/bq_driver/internal/utils.h" #include +#include namespace google::cloud::odbc_bq_driver_internal { @@ -54,6 +55,37 @@ std::string NormalizeRestTableType(std::string type) { } } // namespace +std::optional LiteralFromOdbcPattern(std::string const& filter, + SQLULEN metadata_id) { + if (metadata_id == SQL_TRUE) { + std::string literal = filter; + RTrim(literal); + return literal; + } + if (filter.empty()) { + return std::nullopt; + } + std::string literal; + literal.reserve(filter.size()); + for (size_t i = 0; i < filter.size(); ++i) { + char c = filter[i]; + if (c == '\\') { + // Escape: the next char is a literal (consume both). A lone trailing + // backslash is dropped, matching CastOdbcRegexToCppRegex(). + if (i + 1 < filter.size()) { + literal.push_back(filter[i + 1]); + ++i; + } + } else if (c == '%' || c == '_') { + // A genuine wildcard: the filter must be expanded by enumeration. + return std::nullopt; + } else { + literal.push_back(c); + } + } + return literal; +} + StatusRecord ValidateInputParameters( const SQLCHAR* catalog_name, SQLSMALLINT catalog_name_len, const SQLCHAR* schema_name, SQLSMALLINT schema_name_len, @@ -116,9 +148,19 @@ StatusRecordOr> GetFilteredDatasetIds( StatusRecordOr> datasets = bq_client.FilterDatasets(project_id, filter, options); if (!datasets) { - LOG(ERROR) << "GetFilteredDatasetIds::FilterDatasets:: " - << datasets.GetStatusRecord().message; - return datasets.GetStatusRecord(); + auto const& status = datasets.GetStatusRecord(); + // A catalog passed as a literal (no wildcard) is used directly without + // first confirming it via projects.list, so an unknown or inaccessible + // project surfaces here. Treat "not found" as an empty catalog rather than + // failing the whole call: the pattern-based path simply would not have + // matched that project. Mirrors GetFilteredTables' 404 handling. + if (status.native_error_code == 404) { + LOG(WARNING) << "GetFilteredDatasetIds:: Skipping project not found: '" + << project_id << "': " << status.message; + return std::vector{}; + } + LOG(ERROR) << "GetFilteredDatasetIds::FilterDatasets:: " << status.message; + return status; } for (auto const& dataset : *datasets) { if ((!metadata_id && datasets_filter == "%") || @@ -416,12 +458,15 @@ StatusRecordOr GetResultSetForTables( std::string const& project_filter, std::string const& dataset_filter, std::string const& table_filter, std::string const& table_type_filter, SQLULEN metadata_id) { - // When SQL_ATTR_METADATA_ID is true, the catalog argument is an exact project - // identifier (not a pattern). Skip enumerating every accessible project via - // projects.list and use the name directly. + // When the catalog argument is an exact project identifier -- either because + // SQL_ATTR_METADATA_ID is true, or because the LIKE pattern contains no + // wildcard -- skip enumerating every accessible project via projects.list and + // use the name directly. projects.list has no server-side filter, so it + // returns every project the caller can see just to select one by name. std::vector project_list; - if (metadata_id == SQL_TRUE) { - project_list = {project_filter}; + if (auto project_literal = + LiteralFromOdbcPattern(project_filter, metadata_id)) { + project_list = {std::move(*project_literal)}; } else { auto projects_status_record_or = GetFilteredProjectIds(bq_client, project_filter, metadata_id); @@ -444,23 +489,51 @@ StatusRecordOr GetResultSetForTables( }; std::vector tasks; - for (auto const& project_id : project_list) { - // When SQL_ATTR_METADATA_ID is true, the schema argument is an exact - // dataset identifier (not a pattern). Skip listing every dataset in the - // project via datasets.list - if (metadata_id == SQL_TRUE) { - tasks.push_back({project_id, dataset_filter}); - continue; + std::shared_ptr trace_option = TraceOptions::GetTraceOption(); + int max_threads = trace_option->max_threads; + + // The schema argument is an exact dataset identifier when + // SQL_ATTR_METADATA_ID is true, or when the LIKE pattern contains no + // wildcard. Computed once: it does not depend on the project. + auto const dataset_literal = + LiteralFromOdbcPattern(dataset_filter, metadata_id); + + if (dataset_literal) { + // For an exact dataset identifier, skip listing every dataset in the + // project via datasets.list and address the dataset directly. + for (auto const& project_id : project_list) { + tasks.push_back({project_id, *dataset_literal}); } - auto datasets_status_record_or = GetFilteredDatasetIds( - bq_client, project_id, dataset_filter, metadata_id); - if (!datasets_status_record_or) { - LOG(ERROR) << "GetResultSetForTables::GetFilteredDatasetIds:: " - << datasets_status_record_or.GetStatusRecord().message; - return datasets_status_record_or.GetStatusRecord(); + } else { + // Enumerate datasets for each project in parallel. A serial loop here + // issues one datasets.list REST call per project, which dominates + // SQLTables latency when the catalog pattern matches many projects + // (e.g. catalog = "%"). + auto dataset_task = [&](std::string const& project_id) + -> StatusRecordOr> { + auto datasets_status_record_or = GetFilteredDatasetIds( + bq_client, project_id, dataset_filter, metadata_id); + if (!datasets_status_record_or) { + LOG(ERROR) << "GetResultSetForTables::GetFilteredDatasetIds:: " + << datasets_status_record_or.GetStatusRecord().message; + return datasets_status_record_or.GetStatusRecord(); + } + std::vector project_tasks; + project_tasks.reserve(datasets_status_record_or->size()); + for (auto& dataset_id : *datasets_status_record_or) { + project_tasks.push_back({project_id, std::move(dataset_id)}); + } + return project_tasks; + }; + auto dataset_results_or = + ExecuteParallelTasks>( + max_threads, project_list, dataset_task); + if (!dataset_results_or) { + return dataset_results_or.GetStatusRecord(); } - for (auto const& dataset_id : *datasets_status_record_or) { - tasks.push_back({project_id, dataset_id}); + for (auto& project_tasks : *dataset_results_or) { + tasks.insert(tasks.end(), std::make_move_iterator(project_tasks.begin()), + std::make_move_iterator(project_tasks.end())); } } @@ -491,8 +564,6 @@ StatusRecordOr GetResultSetForTables( }; // 3. Execute tasks using the generic utility - std::shared_ptr trace_option = TraceOptions::GetTraceOption(); - int max_threads = trace_option->max_threads; auto parallel_results_or = ExecuteParallelTasks( max_threads, tasks, parallel_func); diff --git a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.h b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.h index ae0a45e756..41ce56e93a 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.h +++ b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables.h @@ -18,6 +18,7 @@ #include "google/cloud/odbc/bq_driver/internal/odbc_sql_execute_utils.h" #include "google/cloud/odbc/bq_driver/internal/odbc_stmt_handle.h" #include "google/cloud/odbc/internal/status_record_or.h" +#include namespace google::cloud::odbc_bq_driver_internal { @@ -68,6 +69,20 @@ odbc_internal::StatusRecordOr> GetFilteredDatasetIds( ODBCBQClient& bq_client, std::string const& project_id, std::string const& datasets_filter, SQLULEN metadata_id); +// Returns the exact identifier a catalog/schema filter denotes, or nullopt when +// the filter is a genuine search pattern that must be expanded by listing. +// +// When SQL_ATTR_METADATA_ID is SQL_TRUE the argument is an exact identifier and +// is returned verbatim (right-trimmed). Otherwise it is an ODBC LIKE pattern +// and is a literal only when it has no unescaped '%'/'_' metacharacter, in +// which case the unescaped form is returned. Lets SQLTables skip a +// projects.list / datasets.list round trip when a concrete project or dataset +// name was passed -- projects.list in particular has no server-side filter and +// enumerates every visible project. Empty patterns return nullopt to preserve +// the existing (match-nothing) listing path. +std::optional LiteralFromOdbcPattern(std::string const& filter, + SQLULEN metadata_id); + // Construct a query to INFORMATION_SCHEMA.TABLES table depending on input // parameters. Populate 'named_query_params' with named parameters if needed. odbc_internal::StatusRecordOr ConstructQuery( diff --git a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables_test.cc b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables_test.cc index d29c987c3c..c69d460c48 100644 --- a/google/cloud/odbc/bq_driver/internal/odbc_sql_tables_test.cc +++ b/google/cloud/odbc/bq_driver/internal/odbc_sql_tables_test.cc @@ -106,6 +106,49 @@ TEST(ValidateInputParameters, SuccessAllnullsMetadatafalse) { EXPECT_TRUE(status.ok()); } +TEST(LiteralFromOdbcPattern, PlainNameIsLiteral) { + auto literal = LiteralFromOdbcPattern("kirltest", SQL_FALSE); + ASSERT_TRUE(literal.has_value()); + EXPECT_EQ(*literal, "kirltest"); +} + +TEST(LiteralFromOdbcPattern, HyphensAreLiteral) { + auto literal = LiteralFromOdbcPattern("bigquery-devtools-drivers", SQL_FALSE); + ASSERT_TRUE(literal.has_value()); + EXPECT_EQ(*literal, "bigquery-devtools-drivers"); +} + +TEST(LiteralFromOdbcPattern, PercentWildcardIsPattern) { + EXPECT_FALSE(LiteralFromOdbcPattern("%", SQL_FALSE).has_value()); + EXPECT_FALSE(LiteralFromOdbcPattern("%timestamp%", SQL_FALSE).has_value()); +} + +TEST(LiteralFromOdbcPattern, UnderscoreWildcardIsPattern) { + // '_' is an ODBC single-character wildcard, so a name containing it must be + // expanded by listing rather than used as an exact identifier. + EXPECT_FALSE( + LiteralFromOdbcPattern("ODBC_TEST_DATASET", SQL_FALSE).has_value()); +} + +TEST(LiteralFromOdbcPattern, EscapedWildcardsAreLiteral) { + auto literal = LiteralFromOdbcPattern("my\\_dataset\\%", SQL_FALSE); + ASSERT_TRUE(literal.has_value()); + EXPECT_EQ(*literal, "my_dataset%"); +} + +TEST(LiteralFromOdbcPattern, EmptyPatternIsNotLiteral) { + // Preserve the prior (match-nothing) listing path for empty filters. + EXPECT_FALSE(LiteralFromOdbcPattern("", SQL_FALSE).has_value()); +} + +TEST(LiteralFromOdbcPattern, MetadataIdTrueIsAlwaysLiteral) { + // With SQL_ATTR_METADATA_ID true, arguments are exact identifiers even when + // they contain characters that would otherwise be wildcards. + auto literal = LiteralFromOdbcPattern("ODBC_TEST_DATASET ", SQL_TRUE); + ASSERT_TRUE(literal.has_value()); + EXPECT_EQ(*literal, "ODBC_TEST_DATASET"); +} + TEST(ConstructQuery, ConstructWithTwoClausesMetadatafalse) { std::vector named_query_params; diff --git a/google/cloud/odbc/bq_driver/internal/utils_test.cc b/google/cloud/odbc/bq_driver/internal/utils_test.cc index 943bb634d1..49351bf86d 100644 --- a/google/cloud/odbc/bq_driver/internal/utils_test.cc +++ b/google/cloud/odbc/bq_driver/internal/utils_test.cc @@ -1017,6 +1017,7 @@ TEST(ExecuteParallelTasksTest, RespectsSlidingWindow) { EXPECT_GE(duration, (task_count / max_threads) * min_sleep_ms); } + TEST(ExecuteParallelTasksTest, ZeroMaxThreadsRunsSerially) { // A misconfigured MaxThreads of 0 must not hang; it degrades to serial // execution. @@ -1029,6 +1030,7 @@ TEST(ExecuteParallelTasksTest, ZeroMaxThreadsRunsSerially) { EXPECT_THAT(*result, ElementsAre(2, 4, 6)); } + TEST(EscapeOdbcPattern, PlainNameUnchanged) { EXPECT_EQ(EscapeOdbcPattern("kirltest"), "kirltest"); } @@ -1036,7 +1038,8 @@ TEST(EscapeOdbcPattern, PlainNameUnchanged) { TEST(EscapeOdbcPattern, EscapesUnderscores) { // '_' is an ODBC single-character wildcard; a configured dataset name // containing it must match only itself. - EXPECT_EQ(EscapeOdbcPattern("ODBC_TEST_DATASET"), "ODBC\\_TEST\\_DATASET"); + EXPECT_EQ(EscapeOdbcPattern("ODBC_TEST_DATASET"), + "ODBC\\_TEST\\_DATASET"); } TEST(EscapeOdbcPattern, EscapesPercentAndBackslash) { diff --git a/google/cloud/odbc/bq_driver/odbc_driver_metadata.cc b/google/cloud/odbc/bq_driver/odbc_driver_metadata.cc index 3b6ae05f64..03c27915bc 100644 --- a/google/cloud/odbc/bq_driver/odbc_driver_metadata.cc +++ b/google/cloud/odbc/bq_driver/odbc_driver_metadata.cc @@ -32,8 +32,8 @@ using google::cloud::odbc_bq_driver_internal::ConnectionHandle; using google::cloud::odbc_bq_driver_internal::CreateResultSetForTableTypes; using google::cloud::odbc_bq_driver_internal::DescriptorHandle; using google::cloud::odbc_bq_driver_internal::DescriptorType; -using google::cloud::odbc_bq_driver_internal::DSResults; using google::cloud::odbc_bq_driver_internal::EscapeOdbcPattern; +using google::cloud::odbc_bq_driver_internal::DSResults; using google::cloud::odbc_bq_driver_internal::FetchBQSQLProceduresData; using google::cloud::odbc_bq_driver_internal::FetchBQTablesData; using google::cloud::odbc_bq_driver_internal::FetchForeignKeysFromDataSource;