refactor: trivial removals and deprecation completions

cpp/arcticdb/CMakeLists.txt

@@ -402,7 +402,6 @@ set(arcticdb_srcs
         util/name_validation.hpp
         util/native_handler.hpp
         util/offset_string.hpp
-        util/optional_defaults.hpp
         util/pb_util.hpp
         util/preconditions.hpp
         util/preprocess.hpp

cpp/arcticdb/entity/key.hpp

@@ -156,7 +156,8 @@ enum class KeyType : int {
      */
     OFFSET = 19,
     /*
-     * Temporary - remove_me
+     * No longer written by any code path. Retained because KeyType enum values are
+     * persisted to storage and removing or renumbering would break existing data.
      */
     BACKUP_SNAPSHOT_REF = 20,
     /*

cpp/arcticdb/pipeline/read_options.hpp

@@ -9,7 +9,6 @@
 #pragma once
 
 #include <arcticdb/entity/output_format.hpp>
-#include <arcticdb/util/optional_defaults.hpp>
 #include <arcticdb/util/variant.hpp>
 #include <arcticdb/arrow/arrow_output_options.hpp>
 
@@ -40,7 +39,7 @@ struct ReadOptions {
 
     void set_incompletes(const std::optional<bool>& incompletes) { data_->incompletes_ = incompletes; }
 
-    [[nodiscard]] bool get_incompletes() const { return opt_false(data_->incompletes_); }
+    [[nodiscard]] bool get_incompletes() const { return data_->incompletes_ && *data_->incompletes_; }
 
     void set_dynamic_schema(const std::optional<bool>& dynamic_schema) { data_->dynamic_schema_ = dynamic_schema; }
 

cpp/arcticdb/version/local_versioned_engine.cpp

@@ -11,7 +11,6 @@
 #include <arcticdb/version/version_core.hpp>
 #include <arcticdb/storage/storage.hpp>
 #include <arcticdb/storage/storage_options.hpp>
-#include <arcticdb/util/optional_defaults.hpp>
 #include <arcticdb/version/snapshot.hpp>
 #include <arcticdb/stream/stream_sink.hpp>
 #include <arcticdb/util/preconditions.hpp>
@@ -414,7 +413,7 @@ VersionIdentifier get_version_identifier(
         const std::optional<VersionedItem>& version
 ) {
     if (!version) {
-        if (opt_false(read_options.incompletes())) {
+        if (read_options.incompletes() && *read_options.incompletes()) {
             log::version().warn("No index: Key not found for {}, will attempt to use incomplete segments.", stream_id);
             return stream_id;
         } else {

cpp/arcticdb/version/version_core.cpp

@@ -56,11 +56,12 @@ static void modify_descriptor(
         const std::shared_ptr<pipelines::PipelineContext>& pipeline_context, const ReadOptions& read_options
 ) {
 
-    if (opt_false(read_options.force_strings_to_object()) || opt_false(read_options.force_strings_to_fixed()))
+    if ((read_options.force_strings_to_object() && *read_options.force_strings_to_object()) ||
+        (read_options.force_strings_to_fixed() && *read_options.force_strings_to_fixed()))
         pipeline_context->orig_desc_ = pipeline_context->desc_;
 
     auto& desc = *pipeline_context->desc_;
-    if (opt_false(read_options.force_strings_to_object())) {
+    if (read_options.force_strings_to_object() && *read_options.force_strings_to_object()) {
         auto& fields = desc.fields();
         for (Field& field_desc : fields) {
             if (field_desc.type().data_type() == DataType::ASCII_FIXED64)
@@ -69,7 +70,7 @@ static void modify_descriptor(
             if (field_desc.type().data_type() == DataType::UTF_FIXED64)
                 set_data_type(DataType::UTF_DYNAMIC64, field_desc.mutable_type());
         }
-    } else if (opt_false(read_options.force_strings_to_fixed())) {
+    } else if (read_options.force_strings_to_fixed() && *read_options.force_strings_to_fixed()) {
         auto& fields = desc.fields();
         for (Field& field_desc : fields) {
             if (field_desc.type().data_type() == DataType::ASCII_DYNAMIC64)
@@ -1169,7 +1170,7 @@ folly::Future<std::vector<EntityId>> read_and_schedule_processing(
         std::shared_ptr<ComponentManager> component_manager
 ) {
     const ProcessingConfig processing_config{
-            opt_false(read_options.dynamic_schema()),
+            read_options.dynamic_schema() && *read_options.dynamic_schema(),
             pipeline_context->rows_,
             pipeline_context->descriptor().index().type()
     };
@@ -1356,7 +1357,7 @@ static void read_indexed_keys_to_pipeline(
     const bool bucketize_dynamic = index_segment_reader.bucketize_dynamic();
     pipeline_context->desc_ = tsd.as_stream_descriptor();
 
-    const bool dynamic_schema = opt_false(read_options.dynamic_schema());
+    const bool dynamic_schema = read_options.dynamic_schema() && *read_options.dynamic_schema();
     auto queries = get_column_bitset_and_query_functions<index::IndexSegmentReader>(
             read_query, pipeline_context, dynamic_schema, bucketize_dynamic
     );
@@ -2881,7 +2882,8 @@ std::shared_ptr<PipelineContext> setup_pipeline_context(
         const auto existing_range = pipeline_context->index_range();
         if (!existing_range.specified_ || query_range.end_ > existing_range.end_) {
             const ReadIncompletesFlags read_incompletes_flags{
-                    .dynamic_schema = opt_false(read_options.dynamic_schema()), .has_active_version = has_active_version
+                    .dynamic_schema = read_options.dynamic_schema() && *read_options.dynamic_schema(),
+                    .has_active_version = has_active_version
             };
             read_incompletes_to_pipeline(
                     store, pipeline_context, std::nullopt, read_query, read_options, read_incompletes_flags

cpp/arcticdb/version/version_map.hpp

@@ -531,7 +531,6 @@ class VersionMapImpl {
 
     /**
      * @param skip_compat Do not check the legacy "journal" entries
-     * @param iterate_on_failure Use `iterate_type` (slow!) if the linked-list-based load logic throws
      */
     std::shared_ptr<VersionMapEntry> check_reload(
             std::shared_ptr<Store> store, const StreamId& stream_id, const LoadStrategy& load_strategy,

cpp/arcticdb/version/version_store_api.cpp

@@ -15,7 +15,6 @@
 #include <arcticdb/entity/descriptor_item.hpp>
 #include <arcticdb/pipeline/query.hpp>
 #include <arcticdb/pipeline/input_frame.hpp>
-#include <arcticdb/util/optional_defaults.hpp>
 #include <arcticdb/python/python_to_tensor_frame.hpp>
 #include <arcticdb/version/version_map_batch_methods.hpp>
 #include <arcticdb/version/version_utils.hpp>
@@ -429,7 +428,7 @@ std::pair<std::vector<AtomKey>, py::object> get_versions_and_metadata_from_snaps
 std::vector<std::pair<SnapshotId, py::object>> PythonVersionStore::list_snapshots(std::optional<bool> load_metadata) {
     ARCTICDB_RUNTIME_DEBUG(log::version(), "Command: list_snapshots");
     auto snap_ids = std::vector<std::pair<SnapshotId, py::object>>();
-    auto fetch_metadata = opt_false(load_metadata);
+    auto fetch_metadata = load_metadata && *load_metadata;
     iterate_snapshots(store(), [store = store(), &snap_ids, fetch_metadata](const VariantKey& vk) {
         auto snapshot_meta_as_pyobject = fetch_metadata ? get_metadata_for_snapshot(store, vk) : py::none{};
         auto snapshot_id = fmt::format("{}", variant_key_id(vk));

python/arcticdb/adapters/s3_library_adapter.py

@@ -48,9 +48,6 @@ class ParsedQuery:
 
     path_prefix: Optional[str] = None
 
-    # DEPRECATED - see https://github.com/man-group/ArcticDB/pull/833
-    force_uri_lib_config: Optional[bool] = True
-
     # winhttp is used as s3 backend support on Windows by default; winhttp itself maintains ca cert.
     # The options has no effect on Windows
     CA_cert_path: Optional[str] = ""  # CURLOPT_CAINFO in curl
@@ -77,12 +74,6 @@ def __init__(self, uri: str, encoding_version: EncodingVersion, *args, **kwargs)
 
         self._query_params: ParsedQuery = self._parse_query(match["query"])
 
-        if self._query_params.force_uri_lib_config is False:
-            raise ValueError(
-                "The support of 'force_uri_lib_config=false' has been dropped due to security concerns. Please refer to"
-                " https://github.com/man-group/ArcticDB/pull/803 for more information."
-            )
-
         if self._query_params.port:
             self._endpoint += f":{self._query_params.port}"
 
@@ -181,6 +172,16 @@ def _parse_query(self, query: str) -> ParsedQuery:
         parsed_query = re.split("[;&]", query)
         parsed_query = {t.split("=", 1)[0]: t.split("=", 1)[1] for t in parsed_query}
 
+        # force_uri_lib_config was removed but may still appear in existing URIs.
+        # Setting it to false was never supported; true was a no-op.
+        if "force_uri_lib_config" in parsed_query:
+            if not bool(strtobool(parsed_query["force_uri_lib_config"][0])):
+                raise ValueError(
+                    "The support of 'force_uri_lib_config=false' has been dropped due to security concerns. Please refer to"
+                    " https://github.com/man-group/ArcticDB/pull/803 for more information."
+                )
+            del parsed_query["force_uri_lib_config"]
+
         field_dict = {field.name: field for field in fields(ParsedQuery)}
         for key in parsed_query.keys():
             if key not in field_dict.keys():

python/arcticdb/version_store/_store.py

@@ -2945,7 +2945,6 @@ def list_versions(
         symbol: Optional[str] = None,
         snapshot: Optional[str] = None,
         latest_only: Optional[bool] = False,
-        iterate_on_failure: Optional[bool] = False,
         skip_snapshots: Optional[bool] = False,
     ) -> List[Dict]:
         """
@@ -2963,8 +2962,6 @@ def list_versions(
         latest_only : `bool`
             Only include the latest version for each returned symbol. Has no effect if `snapshot` argument is also
             specified.
-        iterate_on_failure: `bool`
-            DEPRECATED: Passing this doesn't change behavior
         skip_snapshots: `bool`
             Don't populate version list with snapshot information.
             Can improve performance significantly if there are many snapshots.
@@ -2991,11 +2988,6 @@ def list_versions(
         `List[Dict]`
             List of dictionaries describing the discovered versions in the library.
         """
-        if iterate_on_failure:
-            log.warning(
-                "The iterate_on_failure argument is deprecated and will soon be removed. It's safe to remove since it doesn't change behavior."
-            )
-
         if latest_only and snapshot and not NativeVersionStore._warned_about_list_version_latest_only_and_snapshot:
             log.warning("latest_only has no effect when snapshot is specified")
             NativeVersionStore._warned_about_list_version_latest_only_and_snapshot = True
@@ -3367,9 +3359,7 @@ def _prune_previous_versions(
         self.delete_versions(symbol, delete_versions)
         log.info(f"Done deleting versions: {delete_versions} for symbol {symbol}")
 
-    def has_symbol(
-        self, symbol: str, as_of: Optional[VersionQueryInput] = None, iterate_on_failure: Optional[bool] = False
-    ) -> bool:
+    def has_symbol(self, symbol: str, as_of: Optional[VersionQueryInput] = None) -> bool:
         """
         Return True if the 'symbol' exists in this library AND the symbol isn't deleted in the specified as_of.
         It's possible for a deleted symbol to exist in snapshots.
@@ -3380,19 +3370,12 @@ def has_symbol(
             symbol name
         as_of : `Optional[VersionQueryInput]`, default=None
             See documentation of `read` method for more details.
-        iterate_on_failure: `Optional[bool]`, default=False
-            DEPRECATED: Passing this doesn't change behavior
 
         Returns
         -------
         `bool`
             True if the symbol exists as_of the specified revision, False otherwise.
         """
-        if iterate_on_failure:
-            log.warning(
-                "The iterate_on_failure argument is deprecated and will soon be removed. It's safe to remove since it doesn't change behavior."
-            )
-
         return self._find_version(symbol, as_of=as_of, raise_on_missing=False) is not None
 
     def column_names(self, symbol: str, as_of: Optional[VersionQueryInput] = None) -> List[str]:
@@ -4040,12 +4023,14 @@ def compact_data_experimental(
         cxx_versioned_item = self.version_store._compact_data(symbol, rows_per_segment, prune_previous_version)
         return self._convert_thin_cxx_item_to_python(cxx_versioned_item, None)
 
-    # TODO: Mark these and Library methods as deprecated
     def is_symbol_fragmented(self, symbol: str, segment_size: Optional[int] = None) -> bool:
         """
         Check whether the number of segments that would be reduced by compaction is more than or equal to the
         value specified by the configuration option "SymbolDataCompact.SegmentCount" (defaults to 100).
 
+        .. deprecated::
+            Use ``defragment_symbol_data`` instead, which checks fragmentation internally.
+
         Parameters
         ----------
         symbol: `str`
@@ -4063,6 +4048,11 @@ def is_symbol_fragmented(self, symbol: str, segment_size: Optional[int] = None)
         -------
         bool
         """
+        warn(
+            "is_symbol_fragmented is deprecated. Use defragment_symbol_data instead, which checks fragmentation internally.",
+            DeprecationWarning,
+            stacklevel=2,
+        )
         return self.version_store.is_symbol_fragmented(symbol, segment_size)
 
     def defragment_symbol_data(

python/arcticdb/version_store/library.py

@@ -9,6 +9,7 @@
 import copy
 import datetime
 import os
+import warnings
 
 import pytz
 from enum import Enum, auto
@@ -3256,6 +3257,9 @@ def is_symbol_fragmented(self, symbol: str, segment_size: Optional[int] = None)
         Check whether the number of segments that would be reduced by compaction is more than or equal to the
         value specified by the configuration option "SymbolDataCompact.SegmentCount" (defaults to 100).
 
+        .. deprecated::
+            Use ``defragment_symbol_data`` instead, which checks fragmentation internally.
+
         Parameters
         ----------
         symbol: `str`
@@ -3273,6 +3277,11 @@ def is_symbol_fragmented(self, symbol: str, segment_size: Optional[int] = None)
         -------
         bool
         """
+        warnings.warn(
+            "is_symbol_fragmented is deprecated. Use defragment_symbol_data instead, which checks fragmentation internally.",
+            DeprecationWarning,
+            stacklevel=2,
+        )
         return self._nvs.is_symbol_fragmented(symbol, segment_size)
 
     def defragment_symbol_data(