feat(elt-common): Watermark (de)serializiation

[WHTaylor]

ref #321

Adds functionality for (de)serializing watermarks from/to JSON, to be read/written from/to iceberg properties.

Compared with elt-command-without-dlt, I've removed the abstraction of WatermarkSerialization to simplify the code a little. There was only a single implementation, and I don't think we'll need others(?), so seemed like a case of YAGNI.

Also combines WriteMode definitions as mentioned here.

Summary by CodeRabbit

• New Features
  • Watermark data can now be serialised and deserialised as JSON
  • Enhanced type validation for watermark values (string, integer, or float)

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: edc4a3ca-9825-4b64-ad88-e83c4b6c2e3a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• ✅ Review completed - (🔄 Check again to review again)

📝 Walkthrough

Walkthrough

This PR extends the Watermark dataclass with JSON serialization and deserialization capabilities. The value field is type-constrained to str | int | float, and new serialize() and deserialize() methods handle JSON encoding/decoding with explicit field validation. WriteMode is moved to a centralised typing module. Tests cover error handling and success cases comprehensively.

Changes

Watermark serialization and validation

Layer / File(s)	Summary
Watermark serialization and deserialization implementation elt-common/src/elt_common/extract.py	Watermark dataclass constrains value to str | int | float, adds serialize() method for JSON encoding, and adds deserialize() static method with validation of required column (string) and value (string/int/float) fields. Imports updated to import WriteMode from elt_common.typing and add json module.
Watermark deserialization validation tests elt-common/tests/unit_tests/test_extract.py	Parametrised tests validate deserialize() raises ValueError with specific messages for missing/invalid fields and type mismatches, and success cases for valid JSON with all supported value types (string, integer, float).

Poem

A watermark's made, all tidy and grand,
With types now precise across the land,
JSON it speaks, with validation so true,
Each field checked thrice, for rabbit and crew. 🐰✨
Serialise, deserialise—robust and sound!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding serialization and deserialization functionality for Watermark objects.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

elt-common/src/elt_common/extract.py (1)

32-36: 💤 Low value

Consider using isinstance() for type checks.

Whilst type(column) is not str and type(value) not in (str, int, float) work correctly for JSON-deserialised primitives, using isinstance() would be more idiomatic Python and handle subclasses gracefully.

♻️ Proposed refactor

         column = as_json["column"]
-        if type(column) is not str:
+        if not isinstance(column, str):
             raise ValueError(f"Watermark 'column' must be a string, '{column}' is not valid")
 
         value = as_json["value"]
-        if type(value) not in (str, int, float):
+        if not isinstance(value, (str, int, float)):
             raise ValueError(
                 f"Watermark 'value' must be a string or number, '{value}' is not valid"
             )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@elt-common/src/elt_common/extract.py` around lines 32 - 36, Replace the
strict type() checks for the watermark fields with idiomatic isinstance()
checks: in the block handling as_json["column"] and as_json["value"] (variables
column and value in elt_common.extract), use isinstance(column, str) and
isinstance(value, (str, int, float)) respectively; update the ValueError
conditions to trigger when these isinstance checks fail so subclasses are
handled correctly while preserving the original error messages and behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@elt-common/src/elt_common/extract.py`:
- Around line 16-17: Add an explicit return type annotation to the serialize
method: change the signature of serialize to include "-> str" (i.e., def
serialize(self) -> str:) so the method explicitly returns a string; ensure any
related type hints or linters accept this change and leave the existing
implementation (json.dumps(...)) unchanged.
- Around line 20-41: The deserialize function lacks an explicit return type;
update its signature for clarity and typing by adding "-> Watermark" to the
deserialize(...) definition (the function that parses JSON and returns a
Watermark instance), ensure the Watermark symbol is imported/available in the
module so the annotation resolves correctly, and run type checks to confirm no
other type errors are introduced.

---

Nitpick comments:
In `@elt-common/src/elt_common/extract.py`:
- Around line 32-36: Replace the strict type() checks for the watermark fields
with idiomatic isinstance() checks: in the block handling as_json["column"] and
as_json["value"] (variables column and value in elt_common.extract), use
isinstance(column, str) and isinstance(value, (str, int, float)) respectively;
update the ValueError conditions to trigger when these isinstance checks fail so
subclasses are handled correctly while preserving the original error messages
and behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c26d3379-88c8-4756-ac7b-cb7488eee653

📥 Commits

Reviewing files that changed from the base of the PR and between 0e59309 and d14e143.

📒 Files selected for processing (2)

• elt-common/src/elt_common/extract.py
• elt-common/tests/unit_tests/test_extract.py

[martyngigg]

I'm happy with moving the serialize/deserialize methods to Watermark. When I was originally playing with it I didn't use JSON and thought custom ingestion scripts might need to provide there own implementation but JSON should be flexible enough to encode what we need and then the extractor path can do with the value what it wants.