Skip to content

Add warning for write side with dynamic_schema override at api usage#3064

Draft
phoebusm wants to merge 1 commit into
masterfrom
remove_dynamic_schema_in_update
Draft

Add warning for write side with dynamic_schema override at api usage#3064
phoebusm wants to merge 1 commit into
masterfrom
remove_dynamic_schema_in_update

Conversation

@phoebusm
Copy link
Copy Markdown
Collaborator

@phoebusm phoebusm commented May 1, 2026

Reference Issues/PRs

https://man312219.monday.com/boards/7852509418/pulses/18372990857

dynamic_schema is a library option as it needs to be aligned at both read and write side, or the symbol may get corrupted. Therefore, this parameter in any write needs to be removed.
As someone could have been using this combination, warning is added only at this PR to advise users to get rid of such api call.
The parameter is scheduled to be removed next month.

What does this implement or fix?

Any other comments?

Checklist

Checklist for code changes...
  • Have you updated the relevant docstrings, documentation and copyright notice?
  • Is this contribution tested against all ArcticDB's features?
  • Do all exceptions introduced raise appropriate error messages?
  • Are API changes highlighted in the PR description?
  • Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

@phoebusm phoebusm added the patch Small change, should increase patch version label May 1, 2026
Comment on lines +1148 to 1153
log.warning(
"update() received 'dynamic_schema' parameter which overrides the library setting "
"and may cause data corruption. Please remove it from the call site. "
"Support for this parameter override is scheduled to be removed in June 2026."
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a scheduled removal, the standard Python convention is warnings.warn() with DeprecationWarning (or FutureWarning for user-facing removals) rather than log.warning(). The latter goes to the logging system, so:

  • Test suites using pytest.warns(DeprecationWarning) or warnings.filterwarnings("error", ...) won't catch it
  • The traceback points into this function rather than the caller, making it hard for users to locate the offending call site (stacklevel=2 fixes this)
Suggested change
log.warning(
"update() received 'dynamic_schema' parameter which overrides the library setting "
"and may cause data corruption. Please remove it from the call site. "
"Support for this parameter override is scheduled to be removed in June 2026."
)
if "dynamic_schema" in kwargs:
import warnings
warnings.warn(
"Passing 'dynamic_schema' directly to update() overrides the library setting "
"and may cause data corruption. Please use LibraryOptions(dynamic_schema=...) "
"when creating the library instead. "
"Support for this parameter override is scheduled to be removed in June 2026.",
FutureWarning,
stacklevel=2,
)

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 1, 2026

ArcticDB Code Review Summary

API & Compatibility

  • ✅ No breaking changes to public Python API
  • ✅ On-disk format unchanged
  • ✅ Deprecation is additive (warning only, parameter still works)

Memory & Safety

  • ✅ No memory management concerns (Python-only change)

Correctness

  • ✅ Warning fires before resolve_defaults() consumes the kwarg, so it correctly catches all usages

Code Quality

  • log.warning() vs warnings.warn() — see inline comment. Using the warnings module with FutureWarning is the standard Python deprecation mechanism. It enables pytest.warns() assertions in tests, respects warnings.filterwarnings("error") in CI/user code, and with stacklevel=2 points the traceback at the caller rather than into this function.
  • Inconsistency across sibling methodswrite() (line ~566), delete() with date_range (line ~3236), and read() (line ~2295) all accept dynamic_schema via **kwargs and feed it into resolve_defaults() with the same override behaviour. If this is a genuine data-corruption hazard, those call sites need the same warning; if only update() is problematic, the warning message should say why.

Testing

  • No test added — CLAUDE.md requires a failing test for every behavioural change. There should be at least one test asserting pytest.warns(FutureWarning) (or DeprecationWarning) when dynamic_schema is passed to update(). Example skeleton:
    def test_update_dynamic_schema_kwarg_warns(lmdb_version_store):
        lib = lmdb_version_store
        df = pd.DataFrame({"a": [1]}, index=pd.date_range("2024-01-01", periods=1))
        lib.write("sym", df)
        with pytest.warns(FutureWarning, match="dynamic_schema"):
            lib.update("sym", df, dynamic_schema=True)
  • Existing non-reg tests silently emit the new warningpython/tests/nonreg/arcticdb/version_store/test_nonreg_specific.py lines 40, 57, 74, 91 all call lib.update(..., dynamic_schema=True). These should be updated to either assert the warning or switch to a library configured with LibraryOptions(dynamic_schema=True).

Build & Dependencies

  • ✅ No build system changes

Security

  • ✅ No security concerns

PR Title & Description

  • ✅ Title is clear and concise
  • PR description is empty — there is no explanation of what problem this solves, why passing dynamic_schema to update() can cause data corruption (as opposed to other methods), or a link to the originating issue/discussion. Reviewers cannot evaluate severity or scope without this context.
  • No PR label — should be labelled enhancement or bug for release notes.

Documentation

  • ✅ No public API signature change; no docstring update required
  • ℹ️ Consider adding a note to the update() docstring (or the LibraryOptions.dynamic_schema docs) that per-call dynamic_schema override is deprecated.

@phoebusm phoebusm changed the title Add warning for update(..., dynamic_schema=...) usage Add warning for write side with dynamic_schema override at api usage May 1, 2026
@phoebusm phoebusm marked this pull request as draft May 1, 2026 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

patch Small change, should increase patch version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant