Skip to content

Minor: Refactor: unify OutputFormat handling via OutputFormat.resolve()#3027

Open
jamesblackburn wants to merge 1 commit into
masterfrom
jb/output_format
Open

Minor: Refactor: unify OutputFormat handling via OutputFormat.resolve()#3027
jamesblackburn wants to merge 1 commit into
masterfrom
jb/output_format

Conversation

@jamesblackburn

Copy link
Copy Markdown
Collaborator

Add OutputFormat.resolve() static method for centralised, case-insensitive conversion of str/enum/None to OutputFormat. Replace ad-hoc .lower() comparisons throughout the read path with direct enum equality checks.

  • options.py: add resolve() method, use it in output_format_to_internal, arrow_output_string_format_to_internal, and RuntimeOptions
  • _store.py: use resolve() in _get_read_options_and_output_format and _adapt_frame_data; tighten return type to OutputFormat

Comment thread python/tests/unit/arcticdb/test_output_format.py Outdated
Comment thread python/arcticdb/options.py
Comment thread python/arcticdb/options.py
@jamesblackburn jamesblackburn added the minor Feature change, should increase minor version label Apr 13, 2026
Add OutputFormat.resolve() static method for centralised, case-insensitive
conversion of str/enum/None to OutputFormat. Replace ad-hoc .lower()
comparisons throughout the read path with direct enum equality checks.

- options.py: add resolve() method, use it in output_format_to_internal,
  arrow_output_string_format_to_internal, and RuntimeOptions
- _store.py: use resolve() in _get_read_options_and_output_format and
  _adapt_frame_data; tighten return type to OutputFormat
@man-group man-group deleted a comment from claude Bot Apr 13, 2026
@claude

claude Bot commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

ArcticDB Code Review Summary

PR #3027 · Refactor: unify OutputFormat handling via OutputFormat.resolve()
Reviewed at: dca9212

API & Compatibility

  • ✅ No breaking changes to Arctic, Library, or NativeVersionStore
  • ✅ No deprecation protocol needed (additive change)
  • ✅ No on-disk format changes
  • ✅ No protobuf schema changes
  • ✅ No key type / ValueType enum changes
  • Subtle public behaviour change: RuntimeOptions.output_format and RuntimeOptions.set_output_format() now always store a resolved OutputFormat enum, never a raw string. Code that previously passed a string and later read back opts.output_format expecting a str (e.g. via == with a string literal) will now get an OutputFormat enum. This should be called out in the PR description.

Memory & Safety

  • ✅ Python-only change; no C++ memory management involved

Correctness

  • output_format_to_internal has no exhaustive fallback (flagged inline at options.py:225): OutputFormat.resolve() can return None when called with None (the default). If None propagates into output_format_to_internal(), all three if/elif branches are skipped and the function silently returns None, which will cause a confusing failure downstream when set_output_format(None) is called. Add an else: raise ValueError(...) guard, or assert the result of resolve() is non-None before passing it to this function.
  • ✅ Exception handling in resolve() is correct — (ValueError, AttributeError) is the right catch for OutputFormat(value.upper()) on non-string inputs

Code Quality

  • ✅ Removed all ad-hoc .lower() comparisons consistently
  • ✅ Test class structure resolved (standalone functions used)

Testing

  • ✅ Behavioural changes have corresponding tests in test_output_format.py
  • test_output_format_to_internal_pyarrow has no availability guard: if pyarrow is not installed in the test environment this test will raise ModuleNotFoundError rather than skip. Add pytest.importorskip("pyarrow") or a skipif marker.
  • No test for output_format_to_internal(OutputFormat.POLARS): the POLARS branch is exercised indirectly but there is no direct unit test equivalent to test_output_format_to_internal_pyarrow for Polars.

Build & Dependencies

  • ✅ Pure Python change; no CMake, vcpkg, or dependency changes

Security

  • ✅ No credentials, secrets, or input-validation regressions

PR Title & Description

  • ✅ Title is clear and concise
  • ✅ Description explains what changed and why
  • ❌ Description does not mention the RuntimeOptions attribute behaviour change (str→enum) noted above under API & Compatibility

Documentation

  • OutputFormat.resolve() has a docstring
  • ✅ No public methods in library.py / arctic.py were changed
  • _store.py changes are internal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor Feature change, should increase minor version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant