Make batch_throw_error not optional

Instead make it an argument to the constructor of `BatchReadOptions`

Author: IvoDD

cpp/arcticdb/pipeline/read_options.hpp

@@ -100,12 +100,18 @@ using ReadOptionsPerSymbol = std::variant<ReadOptions, std::vector<ReadOptions>>
 
 struct BatchReadOptionsData {
     ReadOptionsPerSymbol read_options_per_symbol_;
-    std::optional<bool> batch_throw_on_error_;
+    bool batch_throw_on_error_;
     OutputFormat output_format_ = OutputFormat::PANDAS;
+
+    BatchReadOptionsData(bool batch_throw_on_error) : batch_throw_on_error_(batch_throw_on_error) {}
 };
 
 struct BatchReadOptions {
-    std::shared_ptr<BatchReadOptionsData> data_ = std::make_shared<BatchReadOptionsData>();
+    std::shared_ptr<BatchReadOptionsData> data_;
+
+    BatchReadOptions(bool batch_throw_on_error) {
+        data_ = std::make_shared<BatchReadOptionsData>(batch_throw_on_error);
+    }
 
     void set_read_options(const ReadOptions& read_options) { data_->read_options_per_symbol_ = read_options; }
 
@@ -123,7 +129,7 @@ struct BatchReadOptions {
 
     void set_batch_throw_on_error(bool batch_throw_on_error) { data_->batch_throw_on_error_ = batch_throw_on_error; }
 
-    [[nodiscard]] const std::optional<bool>& batch_throw_on_error() const { return data_->batch_throw_on_error_; }
+    [[nodiscard]] bool batch_throw_on_error() const { return data_->batch_throw_on_error_; }
 
     void set_output_format(OutputFormat output_format) { data_->output_format_ = output_format; }
 

cpp/arcticdb/version/local_versioned_engine.cpp

@@ -539,12 +539,6 @@ std::vector<std::variant<DescriptorItem, DataError>> LocalVersionedEngine::batch
         const std::vector<StreamId>& stream_ids, const std::vector<VersionQuery>& version_queries,
         const BatchReadOptions& batch_read_options
 ) {
-
-    internal::check<ErrorCode::E_ASSERTION_FAILURE>(
-            batch_read_options.batch_throw_on_error().has_value(),
-            "ReadOptions::batch_throw_on_error_ should always be set here"
-    );
-
     auto opt_index_key_futs = batch_get_versions_async(store(), version_map(), stream_ids, version_queries);
     std::vector<folly::Future<DescriptorItem>> descriptor_futures;
     for (auto&& [idx, opt_index_key_fut] : folly::enumerate(opt_index_key_futs)) {
@@ -554,7 +548,7 @@ std::vector<std::variant<DescriptorItem, DataError>> LocalVersionedEngine::batch
     }
     auto descriptors = folly::collectAll(descriptor_futures).get();
     TransformBatchResultsFlags flags;
-    flags.throw_on_error_ = *batch_read_options.batch_throw_on_error();
+    flags.throw_on_error_ = batch_read_options.batch_throw_on_error();
     return transform_batch_items_or_throw(std::move(descriptors), stream_ids, flags, version_queries);
 }
 
@@ -1295,11 +1289,6 @@ std::vector<std::variant<ReadVersionOutput, DataError>> LocalVersionedEngine::ba
     if (stream_ids.empty()) {
         return {};
     }
-    // This read option should always be set when calling batch_read
-    internal::check<ErrorCode::E_ASSERTION_FAILURE>(
-            batch_read_options.batch_throw_on_error().has_value(),
-            "ReadOptions::batch_throw_on_error_ should always be set here"
-    );
     auto opt_index_key_futs = batch_get_versions_async(store(), version_map(), stream_ids, version_queries);
     std::vector<folly::Future<ReadVersionOutput>> read_versions_futs;
 
@@ -1353,7 +1342,7 @@ std::vector<std::variant<ReadVersionOutput, DataError>> LocalVersionedEngine::ba
 
     TransformBatchResultsFlags flags;
     flags.convert_no_data_found_to_key_not_found_ = true;
-    flags.throw_on_error_ = *batch_read_options.batch_throw_on_error();
+    flags.throw_on_error_ = batch_read_options.batch_throw_on_error();
     return transform_batch_items_or_throw(std::move(all_results), stream_ids, flags, version_queries);
 }
 
@@ -2085,11 +2074,6 @@ std::vector<std::variant<std::pair<VariantKey, std::optional<google::protobuf::A
                 const std::vector<StreamId>& stream_ids, const std::vector<VersionQuery>& version_queries,
                 const BatchReadOptions& batch_read_options
         ) {
-    // This read option should always be set when calling batch_read_metadata
-    internal::check<ErrorCode::E_ASSERTION_FAILURE>(
-            batch_read_options.batch_throw_on_error().has_value(),
-            "ReadOptions::batch_throw_on_error_ should always be set here"
-    );
     auto opt_index_key_futs = batch_get_versions_async(store(), version_map(), stream_ids, version_queries);
     std::vector<folly::Future<std::pair<VariantKey, std::optional<google::protobuf::Any>>>> metadata_futures;
     for (auto&& [idx, opt_index_key_fut] : folly::enumerate(opt_index_key_futs)) {
@@ -2102,7 +2086,7 @@ std::vector<std::variant<std::pair<VariantKey, std::optional<google::protobuf::A
     // For legacy reason read_metadata_batch is not throwing if the symbol is missing
     TransformBatchResultsFlags flags;
     flags.throw_on_missing_symbol_ = false;
-    flags.throw_on_error_ = *batch_read_options.batch_throw_on_error();
+    flags.throw_on_error_ = batch_read_options.batch_throw_on_error();
     return transform_batch_items_or_throw(std::move(metadatas), stream_ids, flags, version_queries);
 }
 

cpp/arcticdb/version/python_bindings.cpp

@@ -261,7 +261,7 @@ void register_bindings(py::module& version, py::exception<arcticdb::ArcticExcept
             .def_property_readonly("output_format", &ReadOptions::output_format);
 
     py::class_<BatchReadOptions>(version, "PythonVersionStoreBatchReadOptions")
-            .def(py::init())
+            .def(py::init([](bool batch_throw_on_error) { return BatchReadOptions(batch_throw_on_error); }))
             .def("set_read_options", &BatchReadOptions::set_read_options)
             .def("set_read_options_per_symbol", &BatchReadOptions::set_read_options_per_symbol)
             .def("set_output_format", &BatchReadOptions::set_output_format)

cpp/arcticdb/version/test/test_version_store.cpp

@@ -522,8 +522,7 @@ TEST_F(VersionStoreTest, StressBatchReadUncompressed) {
     }
 
     std::vector<std::shared_ptr<ReadQuery>> read_queries;
-    BatchReadOptions batch_read_options;
-    batch_read_options.set_batch_throw_on_error(true);
+    BatchReadOptions batch_read_options(true);
     batch_read_options.set_output_format(OutputFormat::NATIVE);
     register_native_handler_data_factory();
     auto handler_data = TypeHandlerRegistry::instance()->get_handler_data(batch_read_options.output_format());

python/arcticdb/version_store/_store.py

@@ -2188,9 +2188,8 @@ def _get_batch_read_options(
                     **kwargs,
                 )
             )
-        batch_read_options = _PythonVersionStoreBatchReadOptions()
+        batch_read_options = _PythonVersionStoreBatchReadOptions(batch_throw_on_error)
         batch_read_options.set_read_options_per_symbol(read_options_per_symbol)
-        batch_read_options.set_batch_throw_on_error(batch_throw_on_error)
         # output_format is also a batch level setting, because we currently don't support mixed output formats
         batch_read_options.set_output_format(
             output_format_to_internal(
@@ -3653,8 +3652,7 @@ def _batch_read_descriptor(self, symbols, as_ofs, throw_on_error, date_range_ns_
         for as_of in as_ofs_lists:
             version_queries.append(self._get_version_query(as_of))
 
-        batch_read_options = _PythonVersionStoreBatchReadOptions()
-        batch_read_options.set_batch_throw_on_error(throw_on_error)
+        batch_read_options = _PythonVersionStoreBatchReadOptions(throw_on_error)
         descriptions_or_errors = self.version_store.batch_read_descriptor(symbols, version_queries, batch_read_options)
         args_list = list(zip(descriptions_or_errors, symbols, version_queries, as_ofs_lists))
         description_results = []