Add valid string case check (#1335)#1347
Conversation
ghanse
left a comment
There was a problem hiding this comment.
Overall looks good. Left a few small comments. Can you add some performance tests for this check?
Here's an example:
dqx/tests/perf/test_apply_checks.py
Lines 2144 to 2167 in 9c8fcaf
| | `is_older_than_col2_for_n_days` | Checks whether the values in one input column are at least N days older than the values in another column. | `column1`: first column to check (can be a string column name or a column expression); `column2`: second column to check (can be a string column name or a column expression); `days`: number of days; `negate`: if the condition should be negated | | ||
| | `regex_match` | Checks whether the values in the input column match a given regex. | `column`: column to check (can be a string column name or a column expression); regex: regex to check; `negate`: if the condition should be negated (true) or not | | ||
| | `is_valid_email` | Checks whether the values in the input column have valid email address format. | `column`: column to check (can be a string column name or a column expression) | | ||
| | `has_valid_string_case` | Checks whether the values match the requested string case. `upper` and `lower` use the corresponding Spark case functions; `title` uses Spark `initcap`; `sentence` splits on literal `.` characters, cases each segment independently, and preserves delimiters and surrounding whitespace. | `column`: column to check (can be a string column name or a column expression); `case`: one of `upper`, `lower`, `title`, or `sentence` | |
There was a problem hiding this comment.
I think it's better to say what each case is rather than how it is implemented. For example:
...`upper` and `lower` check that all characters are uppercase or lowercase, respectively; `title` checks that the 1st character of each word is uppercase; `sentence` checks that the 1st character of each sentence is uppercase
| ## Fallback: custom SQL | ||
|
|
||
| **Search `check_funcs` first** — the built-ins cover null/empty, range, set membership, regex, referential, aggregate, uniqueness, schema, freshness, comparison, and outlier cases with typed error messages and tested edge handling. Drop down to SQL only when no built-in fits. | ||
| **Search `check_funcs` first** - the built-ins cover null/empty, string case, range, set membership, regex, referential, aggregate, uniqueness, schema, freshness, comparison, and outlier cases with typed error messages and tested edge handling. Use `has_valid_string_case` for `upper`, `lower`, `title`, or `sentence` string-case validation. Drop down to SQL only when no built-in fits. |
There was a problem hiding this comment.
Let's remove the specific sentence about case validation. Details about most individual checks are intentionally left out of the skill file to prevent it from getting too large.
| None, | ||
| ], | ||
| [None, None, None, None, None, None], | ||
| [None, None, None, violation(" first SEGMENT. second SEGMENT ", "sentence", "sentence"), None, None], |
There was a problem hiding this comment.
I think the title case should have a violation here?
| lambda segment: F.concat( | ||
| F.regexp_extract(segment, r"^(\s*)", 1), | ||
| F.upper(F.substring(F.regexp_replace(segment, r"^\s*", ""), 1, 1)), | ||
| F.lower(F.substring(F.regexp_replace(segment, r"^\s*", ""), 2, 2147483647)), |
There was a problem hiding this comment.
Maybe best to use an internal constant for 2147483647
| elif case == "lower": | ||
| normalized_case = F.lower(col_expr_cast) | ||
| elif case == "title": | ||
| normalized_case = F.initcap(col_expr_cast) |
There was a problem hiding this comment.
This will flag strings with uppercased abbreviations like "Notes From IEEE Meeting".
Maybe we should use a pattern similar to what's been used for sentence case?
| def test_has_valid_string_case_rule_round_trip(): | ||
| rule = DQRowRule( | ||
| criticality="warn", | ||
| check_func=has_valid_string_case, | ||
| column="a", | ||
| check_func_kwargs={"case": "lower"}, | ||
| ) | ||
| expected_metadata = [ | ||
| { | ||
| "name": "a_has_invalid_lower_string_case", | ||
| "criticality": "warn", | ||
| "check": { | ||
| "function": "has_valid_string_case", | ||
| "arguments": {"column": "a", "case": "lower"}, | ||
| }, | ||
| } | ||
| ] | ||
|
|
||
| assert serialize_checks([rule]) == expected_metadata | ||
|
|
||
| deserialized_rules = deserialize_checks(expected_metadata) | ||
| assert len(deserialized_rules) == 1 | ||
| deserialized_rule = deserialized_rules[0] | ||
| assert deserialized_rule.name == "a_has_invalid_lower_string_case" | ||
| assert deserialized_rule.check_func is has_valid_string_case | ||
| assert deserialized_rule.column == "a" | ||
| assert deserialized_rule.check_func_kwargs == {"case": "lower"} |
There was a problem hiding this comment.
I don't think we need a specific test for this check. We can add it to the rulesets in the existing tests.
|
Thanks for the review and the helpful comments. I’ll work through the requested changes, add the performance test coverage, and update the PR accordingly. |
|
Hi @ghanse, I’ve incorporated the feedback from the review into PR #1347. The updates include the requested documentation changes, revised title-case behavior for uppercase abbreviations, updated integration expectations, use of an internal substring-length constant, consolidation of the rule-construction coverage, and performance tests for all four supported case modes. The PR is ready for another review. Thanks! |
Changes
has_valid_string_caserow-level check for validating consistent string casing.upper,lower,title, andsentencecase modes.caseargument and reject unsupported values.Linked issues
Resolves #1335
Tests
Documentation and Demos