Implement Features Per Issue 579#580
Conversation
|
I'm looking for feedback on this PR. I think it is ready to go. I can promote it to a full PR once I get some indication that I'm headed in the right direction. I'd like to also fix #533 but can do that as a separate PR if that is desirable. |
Adding Link to synapse install instructions.
adding instructions
|
is anyone available to review this and help me get it merged? |
|
@jasonhorner We apologize for the delay in reviewing this PR. We are available to review it and help it get merged. Can you promote it to a full PR so that the build can be run? |
|
@rdsharma26 it looks like it is ready to go please advise if additional steps are need on my end |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Format: Five digit U.S. Zip code and an optional four digit code separated by a hyphen (-). | ||
| val POSTAL_CODE_US: Regex = """\b\d{5}(?:-\d{4})?\b""".r | ||
|
|
||
| // Format: U.S. phone number with optional country code, area code, and extension. |
There was a problem hiding this comment.
The PHONE_NUMBER_US regex uses ^ and $ anchors, which will not work correctly with Spark's regexp function (which uses java.util.regex partial matching). In the PatternMatch analyzer, the pattern is applied via RegexAnalysis.evaluateFilteredRow which wraps it in a regexp_extract or similar — but ^/$ anchors will prevent matching when the value is embedded in a larger string or when Spark applies the regex per-row differently than expected. The existing patterns (e.g., CREDITCARD, EMAIL, URL, SOCIAL_SECURITY_NUMBER_US) do not use ^/$ anchors; they use \b word boundaries instead. For consistency and correctness, replace ^ and $ with \b or remove them entirely.
| withSpark { session => | ||
|
|
||
| // Create sample data with SSN, PostalCode, and PhoneNumber columns | ||
| val data = itemsAsDataframe(session, |
There was a problem hiding this comment.
This example uses itemsAsDataframe from ExampleUtils, but that function expects the existing Item case class defined in ExampleUtils (which has fields like id: Long, productName: String, description: String, priority: String, numViews: Long). The Item case class defined at line 68 of this file shadows it locally, but itemsAsDataframe likely calls Seq(...).toDF() with the ExampleUtils.Item encoder, not this local one. This will fail at runtime with a schema mismatch. You should use Seq(...).toDF("id", "SSN", "PostalCode", "PhoneNumber") directly instead.
| data | ||
| .withColumn("new", metric.fullColumn.get) | ||
| .collect() | ||
| .map(_.getAs[Any]("new")) shouldBe |
There was a problem hiding this comment.
The references SSN_US, PHONE_NUMBER_US, and POSTAL_CODE_US are used without qualification. These are defined in Patterns object. You need to either import Patterns._ or qualify them as Patterns.SOCIAL_SECURITY_NUMBER_US, Patterns.PHONE_NUMBER_US, Patterns.POSTAL_CODE_US. As written, this will not compile.
| } | ||
|
|
||
|
|
||
| def getDfWithPatternMatch(sparkSession: SparkSession): DataFrame = { |
There was a problem hiding this comment.
Indentation issue: the body of getDfWithPatternMatch is not indented relative to the method definition. The import statement and Seq(...) block should be indented by 2 or 4 spaces to match the rest of the file's style.
| ("", "", "") // Empty row, testing blanks | ||
| ).toDF("SSN", "PostalCode", "PhoneNumber") | ||
| } | ||
| def getFakeNumericColumnProfileWithMinMaxMeanAndStdDev( |
There was a problem hiding this comment.
Missing blank line between the closing } of getDfWithPatternMatch and the next method getFakeNumericColumnProfileWithMinMaxMeanAndStdDev.
| val patternMatchCountry = PatternMatch( | ||
| "Address Line 2", | ||
| """\w""".r, | ||
| where = Option("id < 5"), |
There was a problem hiding this comment.
The expected results for SSN matching look incorrect. "078-05-1120" is explicitly excluded in the SOCIAL_SECURITY_NUMBER_US regex (it's a known invalid SSN), yet the expected result at index 1 is true. Similarly, verify that "123-45-0000" (serial 0000 is excluded) and "123-00-6789" (group 00 is excluded) are correctly expected as false. Please double-check all expected values against the regex.
| analyzerOptions = | ||
| Option(AnalyzerOptions(filteredRow = FilteredRowOutcome.TRUE)) | ||
| ) | ||
| val state = patternMatchSSN.computeStateFrom(data) |
There was a problem hiding this comment.
The expected results for phone number matching need verification. "1234567890" (index 3) — this is 10 digits without separators where the first digit is 1, which would fail the [2-9] requirement for the area code start. Yet the expected value is true. Also "123-45-6789" (index 6) starts with 1 in the exchange, which should fail [2-9] check, but it's expected as... actually it's true in the expected. Please carefully re-validate all expected phone number match results against the regex.
| Follow these steps to install AWS Deequ in your Azure Synapse environment: | ||
|
|
||
| ## 1. Download the Deequ JAR | ||
| Download the Deequ JAR that matches the Spark version of your Synapse Spark pool. As of September 2024, the required version is Deequ 2.0.7 for Spark 3.4. |
There was a problem hiding this comment.
This documentation references a specific version ("Deequ 2.0.7 for Spark 3.4") and date ("As of September 2024"). This will become stale quickly. Consider referencing the Maven Central page generically or noting that users should pick the version matching their Spark pool version without hardcoding a specific version.
| ``` | ||
| libraryDependencies += "com.amazon.deequ" % "deequ" % "2.0.0-spark-3.1" | ||
| ``` | ||
| __synapse__ |
There was a problem hiding this comment.
Grammar: "an synapse" should be "a Synapse" (Synapse starts with a consonant sound and should be capitalized as a proper noun).
|
|
||
| // Define the item case class to match the schema used in the DataFrame | ||
| case class Item(id: Int, SSN: String, PostalCode: String, PhoneNumber: String) | ||
| } No newline at end of file |
There was a problem hiding this comment.
Missing newline at end of file.
|
@jasonhorner are you still working on resolving the comments and make this PR merge ready? |
3 participants
Issue #, if available:
#579
Description of changes:
PatternMatchanalyzer by incorporating the following regex patterns:PatternMatchexamples and unit tests to include these new patterns for validation.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.