feat: add is_valid_email row check function#1158
Conversation
|
All commits in PR should be signed ('git commit -S ...'). See https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits |
aarushisingh04
force-pushed
the
feat/is-valid-email
branch
from
adeee39 to
c1234b5
Compare
aarushisingh04
force-pushed
the
feat/is-valid-email
branch
from
7da2c71 to
5f57700
Compare
ghanse
added
the
under-review
ghanse
left a comment
ghanse
left a comment
There was a problem hiding this comment.
Looks good overall. You will need to sign your commits (see the contributor docs for more info).
Can you add documentation for this function to /docs/reference/quality_checks.mdx? You will need to update the row-level checks table, Python examples, and YAML examples.
aarushisingh04
force-pushed
the
feat/is-valid-email
branch
from
e33c783 to
72ddac9
Compare
|
@ghanse thanks for the review, i have addressed all of the above mentioned points |
mwojtyczka
left a comment
mwojtyczka
left a comment
There was a problem hiding this comment.
Solid contribution overall — well-tested, well-documented, and follows the existing _matches_pattern shape. Posting 6 inline comments (1 correctness gap, several polish items). One additional ask below.
Add a performance test
Regex-based row checks like this can have non-obvious cost on adversarial inputs. Could you add a perf test under tests/perf/ benchmarking is_valid_email against a representative mix (valid dot-atom, quoted-local, IP-literal, long-but-rejected strings) so we have a baseline before merge? The existing IPv4/IPv6 perf cases are a good template.
| # Domain: LDH hostname with alphabetic TLD, or IP-literal | ||
| rf"(?:" | ||
| rf"(?:{_EMAIL_DOMAIN_LABEL}\.)+[A-Za-z]{{2,63}}" | ||
| rf"|\[(?:{_IPV4_OCTET}(?:\.{_IPV4_OCTET}){{3}}|IPv6:[A-Fa-f0-9:]+)\]" |
There was a problem hiding this comment.
(Cosmetic) Captured-group count grows.
_IPV4_OCTET = r"(25[0-5]|2[0-4]\d|1\d{2}|[1-9]?\d)" is a capturing group. Each reuse here adds 4 more capture groups beyond what's needed (the existing IPV4_ADDRESS does the same). No correctness impact, but switching _IPV4_OCTET to non-capturing (?:…) once at its definition would clean this up across all callers. Optional; can be a follow-up.
There was a problem hiding this comment.
Thanks for your contribution. The PR is generally close to be ready. I left some comments. Please add also one performance test and add the check here: https://github.com/databrickslabs/dqx/blob/main/tests/resources/all_row_checks.yaml
|
@ghanse thanks for the ongoing fixes. to avoid potential duplication of efforts, shall i address the comments above that are to be resolved? i am ready to do it, please let me know if you have a different plan. |
I think this looks good now. Made a few small changes to the pattern and added a few test cases. |
|
|
|
@ghanse @aarushisingh04 perf tests have some small issues, left comments. Otherwise LGTM |
4 participants
changes
added a new
is_valid_emailrow-level check function that validates email addresses following a pragmatic subset of RFC 5321/5322.implementation:
EMAIL_ADDRESSpattern to theDQPatternregistry.is_valid_ipv4_addressby ignoringnullvalues (no violation reported).DQRowRule/DQForEachColRulecreation and declarative YAML metadata setups (function: is_valid_email).linked issues
resolves #1156
tests
documentation and demos