Feat: Add Safe Data Preview by kw-datamasque · Pull Request #36 · datamasque/datamasque-python

kw-datamasque · 2026-07-15T20:16:21Z

Adds SafeDataPreviewOptions on InDataDiscoveryConfig and a typed safe_data_preview field on schema-discovery result columns. Changelog entry under 1.1.8 (unreleased).

cph-datamasque

Have a think about whether the UnknownPreview is a safe idea from the point of view of a person using this library.

Remove tests that are just testing pydantic, and collapse repetitive tests using pytest.mark.parametrize().

cph-datamasque · 2026-07-16T21:36:47Z

+            pass
+    try:
+        return UnknownPreview.model_validate(value)
+    except ValidationError:


This kind of future-proofing fallback isn't present for any other model in the client, so unsure how I feel about it - catching client bugs is good and having a defined shape makes using it much easier. But on the other hand we can catch bugs using tests. What do you think?

Yeah I agree with the principle in general but would defend it here.
I added this fallback so in some case where something changed in datamasque, say a new kind was added, and dm-python wasn't updated then falling back wouldn't cause the discovery run to crash.
In my opinion this is important so a change in SDP doesn't crash discovery runs since there's nothing pinning the client and servers together currently.
However happy to change it if I'm wrong.

After looking into it further have concluded that it's not as bad as I thought if a run fails here since discovery runs are owned inside DM itself. Therefore, will get rid of the UnknownPreview entirely since the shape should be always known.

Would be good, in the future, to add some sort of compatibility check/matrix between DM and dm-python.

cph-datamasque · 2026-07-16T21:38:44Z

    }


+def test_schema_discovery_request_model_dump_includes_safe_data_preview():


First of several tests that don't add any value. This is just testing pydantic's behaviour. If you want to test this, at least call the API method that sends the SD request and check the request body (or fold into one of the existing tests that does that).

Good catch. Dropped.

cph-datamasque · 2026-07-16T21:40:43Z

+    assert preview.statistics_common.count_distinct == 988
+
+
+def test_schema_discovery_result_without_safe_data_preview():


Borderline not worth it (again you're testing that the default is None, which it is because we can see it right there - no actual logic tested, except for the bit where the validator tolerates and spits out None when given None).

Removed. Good spotting!

cph-datamasque · 2026-07-16T21:41:26Z

+            _assert_no_unexpected_extras(item)
+
+
+def test_each_preview_parses_to_its_typed_variant():


use pytest.mark.parametrize()

cph-datamasque · 2026-07-16T21:41:50Z

+    assert type(parse_safe_data_preview(_UNSUPPORTED_PREVIEW)) is UnsupportedPreview
+
+
+def test_each_preview_round_trips():


tests pydantic not our code, remove

cph-datamasque · 2026-07-16T21:42:07Z

+    assert parse_safe_data_preview(_UNSUPPORTED_PREVIEW).model_dump(mode="json") == _UNSUPPORTED_PREVIEW
+
+
+def test_each_preview_has_no_untyped_fields():


use pytest.mark.parametrize

Good catch. Changed.

cph-datamasque · 2026-07-16T21:42:35Z

+    assert preview.statistics_kind.reason == "Binary data isn't previewed"
+
+
+def test_none_passes_through():


already tested by the one I flagged earlier, remove one of them

Combined all tests testing null behaviour into one with pytest.mark.parametrize. Also removed one since it was testing the same arm.

cph-datamasque · 2026-07-16T21:45:06Z

+    assert preview.statistics_common.count_null == 0
+
+
+def test_unparseable_payload_falls_back_without_raising():


this is the kind of thing I was worried about with the fallback. If someone wrote some code using the library that tried to do maths on the count_row, it would just crash. We should at least ensure UnknownPreview has all fields present, typed, and with defaults if we are going to keep it

Won't be a problem anymore because of this.

cph-datamasque · 2026-07-16T21:45:37Z

+    assert preview.kind == 123
+
+
+def test_non_dict_preview_is_none():


shouldn't this be an error?

Not a problem anymore either because of this.

cph-datamasque · 2026-07-16T21:47:02Z

+  * ``safe_data_preview`` on ``InDataDiscoveryConfig`` (via ``SafeDataPreviewOptions``)
+    to configure or disable the preview.
+  * ``safe_data_preview`` on schema-discovery result columns and file-discovery locators,
+    typed by ``kind``.


line length is 120

Suggested change

* ``safe_data_preview`` on ``InDataDiscoveryConfig`` (via ``SafeDataPreviewOptions``)

to configure or disable the preview.

* ``safe_data_preview`` on schema-discovery result columns and file-discovery locators,

typed by ``kind``.

* ``safe_data_preview`` on ``InDataDiscoveryConfig`` (via ``SafeDataPreviewOptions``) to configure or disable the preview.

* ``safe_data_preview`` on schema-discovery result columns and file-discovery locators, typed by ``kind``.

Nice find. Fixed.

kanewilliams

Looks good on my end, conditional on @cph-datamasque's review

kw-datamasque assigned ClassicMMT Jul 15, 2026

ClassicMMT force-pushed the typed-safe-data-preview branch 3 times, most recently from 69ef38f to 9111d65 Compare July 16, 2026 02:27

ClassicMMT changed the title ~~feat(discovery): Add typed Safe Data Preview options and result field~~ feat: Add safe data preview results Jul 16, 2026

ClassicMMT marked this pull request as ready for review July 16, 2026 02:33

ClassicMMT requested review from cph-datamasque and kanewilliams July 16, 2026 02:34

cph-datamasque reviewed Jul 16, 2026

View reviewed changes

kw-datamasque commented Jul 16, 2026

View reviewed changes

Comment thread tests/test_safe_data_preview.py Outdated

Comment thread datamasque/client/models/safe_data_preview.py Outdated

Comment thread datamasque/client/models/safe_data_preview.py Outdated

Comment thread datamasque/client/models/safe_data_preview.py Outdated

ClassicMMT mentioned this pull request Jul 17, 2026

Feat: Add configurable discovery datamasque/datamasque-cli#18

Draft

ClassicMMT added 2 commits July 17, 2026 16:15

feat: safe data preview

cc49b65

feat: add row_count to schema-discovery table_metadata

5bcf818

ClassicMMT force-pushed the typed-safe-data-preview branch 2 times, most recently from 62e576e to 5bcf818 Compare July 19, 2026 22:44

ClassicMMT mentioned this pull request Jul 19, 2026

Feat: Add Configurable Discovery Validation Capabilities #37

Draft

ClassicMMT changed the title ~~feat: Add safe data preview results~~ Feat: Add Safe Data Preview Jul 19, 2026

kw-datamasque commented Jul 20, 2026

View reviewed changes

Comment thread datamasque/client/models/safe_data_preview.py Outdated

ClassicMMT force-pushed the typed-safe-data-preview branch 2 times, most recently from 799f58f to 10f2e69 Compare July 20, 2026 03:16

Fix: Remove UnknownPreview and Clean Up

f01e0b2

ClassicMMT force-pushed the typed-safe-data-preview branch from 10f2e69 to f01e0b2 Compare July 20, 2026 20:07

kanewilliams approved these changes Jul 20, 2026

View reviewed changes

		}


		def test_schema_discovery_request_model_dump_includes_safe_data_preview():

		assert preview.statistics_common.count_distinct == 988


		def test_schema_discovery_result_without_safe_data_preview():

		_assert_no_unexpected_extras(item)


		def test_each_preview_parses_to_its_typed_variant():

		assert type(parse_safe_data_preview(_UNSUPPORTED_PREVIEW)) is UnsupportedPreview


		def test_each_preview_round_trips():

		assert parse_safe_data_preview(_UNSUPPORTED_PREVIEW).model_dump(mode="json") == _UNSUPPORTED_PREVIEW


		def test_each_preview_has_no_untyped_fields():

		assert preview.statistics_kind.reason == "Binary data isn't previewed"


		def test_none_passes_through():

		assert preview.statistics_common.count_null == 0


		def test_unparseable_payload_falls_back_without_raising():

		assert preview.kind == 123


		def test_non_dict_preview_is_none():

Uh oh!

Conversation

kw-datamasque commented Jul 15, 2026

Uh oh!

cph-datamasque left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ClassicMMT Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kanewilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ClassicMMT Jul 20, 2026 •

edited

Loading