[CONTRIB] Promote ExpectColumnValuesToMatchStrftimeFormat to supported-core - #12009
Conversation
Brings the expectation up to the Gallery-supported bar: fixes the broken docstring (the FAILURE_SEVERITY_DESCRIPTION token was never interpolated), adds Gallery support metadata and a generated JSON schema, and declares Pandas + Spark as the supported backend matrix (SQL is out of scope for now, per the discussion in the issue - strftime tokens don't map cleanly onto SQL dialects' date-format models). Extends the integration tests to cover Pandas alongside the existing Spark cases, plus mostly-threshold coverage. Closes fivetran#12004 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
👷 Deploy request for niobium-lead-7998 pending review.Visit the deploys page to approve it
|
|
All committers have signed the CLA. ✅ |
|
@cla-bot check |
joshua-stauffer
left a comment
There was a problem hiding this comment.
hey @nanjeshramesh, thanks for the PR! this looks great, I just have some notes for the docstring. The passing case which fails is the critical one, the other two are more nits, but would be nice to clean up while we're making changes.
Once those changes are in we'll get this merged. Thanks again!
| }}, | ||
| "meta": {{}}, | ||
| "success": false | ||
| }} |
There was a problem hiding this comment.
This is labeled as a passing case, but success is (correctly) false. We could use the pattern in great_expectations/expectations/core/expect_column_values_to_match_regex.py and have the example data use two columns instead, then use one for the passing case and the other for the failing case.
There was a problem hiding this comment.
the docstrings are rendered in the Expectations gallery, so we want one actual passing case, and one failing case.
|
|
||
| SQL data sources are not currently supported: strftime format tokens do not map cleanly \ | ||
| onto the date-format models of SQL dialects. | ||
|
|
There was a problem hiding this comment.
the newline continuation here is likely to cause extra whitespace to be rendered. Can we remove it, and move this section up to the bottom of the Column Map Expectations are one of the most common types of Expectation. paragraph?
| mostly (None or a float between 0 and 1): \ | ||
| Successful if at least mostly fraction of values match the expectation. \ | ||
| {MOSTLY_DESCRIPTION} \ | ||
| For more detail, see [mostly](https://docs.greatexpectations.io/docs/reference/expectations/standard_arguments/#mostly). |
There was a problem hiding this comment.
Can we append Default 1. to this line, like in great_expectations/expectations/core/expect_column_values_to_match_regex.py?
- Rework the Gallery Code Examples to use two columns (event_date / invalid_date), so the Passing Case genuinely passes (success: true) and the Failing Case genuinely fails, following the pattern in ExpectColumnValuesToMatchRegex. Both examples were run against the actual expectation to confirm the JSON output is accurate. - Move the SQL-unsupported caveat out of the indented Supported Data Sources block (where its line continuation rendered with extra whitespace) and onto the end of the introductory Column Map Expectations paragraph instead. - Append "Default 1." to the mostly parameter description, matching the regex exemplar. Regenerated the JSON schema to match the updated docstring. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
My previous commit moved the SQL-unsupported sentence to the end of the intro paragraph but kept the backslash line-continuation between its two lines, reproducing the exact rendering issue that was flagged instead of fixing it. The surrounding sentences in that paragraph use plain lines with no continuation, so match that: each sentence is now its own line with no trailing backslash. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Hey @joshua-stauffer, thanks for the review! Pushed fixes for all three:
Ready for another look whenever you get a chance. Thanks again! |
joshua-stauffer
left a comment
There was a problem hiding this comment.
looks great, thanks @nanjeshramesh
Summary
Closes #12004.
ExpectColumnValuesToMatchStrftimeFormatalready existed and was exported, but wasn't in the Expectation Gallery, had no generated schema, carried none of the support metadata, and had an undefined backend matrix. This brings it up to the supported-core bar:{FAILURE_SEVERITY_DESCRIPTION}. Converted to an__doc__ = f"""..."""f-string in the standard Gallery format (short description, Args, Supported Data Sources, Data Quality Issues, Example Data, passing/failing Code Examples), following theExpectColumnValuesToMatchRegexexemplar.EXPECTATION_SHORT_DESCRIPTION,DATA_QUALITY_ISSUES,SUPPORTED_DATA_SOURCES, alibrary_metadataClassVar with_library_metadataalias, andConfig.schema_extra.supported_expectationsintasks.pyand generatesgreat_expectations/expectations/core/schemas/ExpectColumnValuesToMatchStrftimeFormat.jsonviainvoke schemas --sync.Backend matrix decision: shipping outcome (A) from the issue — Pandas + Spark supported, SQL explicitly declared out of scope. The backing metric (
column_values_match_strftime_format.py) has no SqlAlchemy branch, and generic strftime-format validation doesn't map cleanly onto SQL dialects' date-format models (as noted in the issue, with precedent inExpectColumnValuesToMatchRegex, which also omits some SQL dialects). Happy to follow up with a SQL implementation (outcome B) separately if maintainers want it split out.Changes
great_expectations/expectations/core/expect_column_values_to_match_strftime_format.py— Gallery-format docstring, support metadata, schema configgreat_expectations/expectations/core/schemas/ExpectColumnValuesToMatchStrftimeFormat.json— generated schema (new file)tasks.py— added tosupported_expectationstests/integration/data_sources_and_expectations/expectations/test_expect_column_values_to_match_strftime_format.py— extended from Spark-only to also cover Pandas, plus addedmostly-threshold success/failure casesTest plan
ruff check/ruff format --checkpass on all changed filespytest tests/expectations/core/test_core_model_schemas.pypasses (schema-sync check)pytest tests/expectations/ -m unit— 1498 passed, 0 failedpytest tests/ -m unit— 4330 passed, 0 failed-m "not spark"): timezone-aware format success (regression test for Validating datetime strings with timezone produces an error #9203), non-matching format failure, mostly-threshold met/not-met