feat: Add safe data preview results 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?

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).

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).

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

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

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

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

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


shouldn't this be an error?

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``.

kw-datamasque · 2026-07-16T02:53:11Z

+    assert parse_safe_data_preview(42) is None
+
+
+def _column_data(**overrides: object) -> dict:


Function naming: could we use verb_noun here for clarity, e.g. _build_column_data()?

kw-datamasque · 2026-07-16T23:06:38Z

+
+
+class StringStatistics(BaseModel):
+    """String preview statistics; `patterns`/`first_chars` appear only at the matching disclosure level."""


Suggested change

"""String preview statistics; `patterns`/`first_chars` appear only at the matching disclosure level."""

"""String preview statistics; `patterns`/`first_chars` appear at or above certain disclosure levels."""

Since patterns appears even if the user selects the first_chars disclosure level

kw-datamasque · 2026-07-16T23:10:45Z

+        return UnknownPreview.model_construct(**value)  # type: ignore[arg-type]
+
+
+SafeDataPreview = SerializeAsAny[ColumnPreview]


Being explicit here could help mypy.

Suggestion:

Suggested change

SafeDataPreview = SerializeAsAny[ColumnPreview]

SafeDataPreview = Union[

StringPreview, NumericPreview, TemporalPreview, BooleanPreview, UnsupportedPreview, UnknownPreview

]

kw-datamasque · 2026-07-16T23:12:57Z

+    unsupported = "unsupported"
+
+
+class UnsupportedPreviewReason(str, Enum):


This will stale if DataMasque's unsupported preview reasons change or get added to, and furthermore it's only used in tests (Might have been intended to be used in UnsupportedStatistics)?

Can we delete UnsupportedPreviewReason() and leave UnsupportedStatistics untyped with reason: str like it currently is?

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

ClassicMMT mentioned this pull request Jul 17, 2026

Add configurable discovery datamasque/datamasque-cli#18

Draft

ClassicMMT added 3 commits July 17, 2026 16:15

feat: safe data preview

cc49b65

feat: add row_count to schema-discovery table_metadata

5bcf818

feat: add configurable discovery validation

62e576e

ClassicMMT force-pushed the typed-safe-data-preview branch from fc7ac8d to 62e576e Compare July 17, 2026 04:30

		}


		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():

		assert parse_safe_data_preview(42) is None


		def _column_data(**overrides: object) -> dict:



		class StringStatistics(BaseModel):
		"""String preview statistics; `patterns`/`first_chars` appear only at the matching disclosure level."""

		return UnknownPreview.model_construct(**value) # type: ignore[arg-type]


		SafeDataPreview = SerializeAsAny[ColumnPreview]

-SafeDataPreview = SerializeAsAny[ColumnPreview]
+SafeDataPreview = Union[
+    StringPreview, NumericPreview, TemporalPreview, BooleanPreview, UnsupportedPreview, UnknownPreview
+]

		unsupported = "unsupported"


		class UnsupportedPreviewReason(str, Enum):

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants