Add `update_column` and `add_column` to `SingleTableMetadata` #915

pvk-developer · 2022-07-20T17:56:05Z

Resolves #877

amontanez24

A few comments but I think this is looking really good so far!

amontanez24 · 2022-07-20T20:48:25Z

sdv/metadata/single_table.py

+            matches = re.findall('(%.)|(%)', formated_date)
+            if matches:
+                matches = ', '.join(item for match in matches for item in match if item)
+                matches = ''.join(char if not char.isalnum() else '.' for char in matches)


what is this regex doing? Can we just use the same logic as in the actual transformer? As in try to parse the datetime and if it comes back with an error than we catch that here?

There is no such way to tell if a strftime is valid (or at least I didn't find one). This regex detects if there are % left on the formated_date, which is a way to tell whether or not the user passed an extra % which is the character that is usually used to specify any datetime like formating string.
The last line maybe I can remove it since it's just to clear the string a bit. I don't want ot confuse the end user since it can be a bit confusing if we find a % next to a 1 that comes from the datetime.now().

amontanez24 · 2022-07-20T20:55:55Z

tests/unit/metadata/test_single_table.py

+        instance._validate_unexpected_kwargs('age', 'numerical')
+        instance._validate_unexpected_kwargs('age', 'numerical', representation='int')
+        instance._validate_unexpected_kwargs('start_date', 'datetime')
+        instance._validate_unexpected_kwargs('start_date', 'datetime', datetime_format='%Y-%d')
+        instance._validate_unexpected_kwargs('name', 'categorical')
+        instance._validate_unexpected_kwargs('name', 'categorical', order_by='alphabetical')
+        instance._validate_unexpected_kwargs('name', 'categorical', order=['a', 'b', 'c'])
+        instance._validate_unexpected_kwargs('synthetic', 'boolean')
+        instance._validate_unexpected_kwargs('phrase', 'text')
+        instance._validate_unexpected_kwargs('phrase', 'text', regex_format='[A-z]')
+        instance._validate_unexpected_kwargs('phone', 'phone_number')


Can we parameterize this test instead of calling all these different cases like this? We can make the passed kwargs, and the error message parameters

🤔 yes let me re-do it

codecov-commenter · 2022-07-21T09:59:12Z

Codecov Report

Merging #915 (214a504) into V1.0.0.dev (fd26ffd) will increase coverage by 0.68%.
The diff coverage is 97.46%.

@@              Coverage Diff               @@
##           V1.0.0.dev     #915      +/-   ##
==============================================
+ Coverage       69.48%   70.16%   +0.68%     
==============================================
  Files              39       39              
  Lines            2949     3023      +74     
==============================================
+ Hits             2049     2121      +72     
- Misses            900      902       +2

Impacted Files	Coverage Δ
sdv/metadata/single_table.py	`98.70% <97.46%> (-1.30%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd26ffd...214a504. Read the comment docs.

katxiao · 2022-07-21T15:34:52Z

sdv/metadata/single_table.py

@@ -22,6 +32,68 @@ class SingleTableMetadata:
    KEYS = ['columns', 'primary_key', 'alternate_keys', 'constraints', 'SCHEMA_VERSION']
    SCHEMA_VERSION = 'SINGLE_TABLE_V1'

+    @staticmethod
+    def _validate_numerical(column_name, **kwargs):
+        representations = [


Can we pull this out as a constant?

katxiao · 2022-07-21T15:41:18Z

tests/unit/metadata/test_single_table.py

+        """
+        # Setup
+        instance = SingleTableMetadata()
+        representations = [


You could parameterize the unit test with the list of representations. That way if one fails, it's easy to tell which representation failed.

amontanez24

A couple of minor comments but I think this is ready!

amontanez24 · 2022-07-21T17:18:37Z

tests/unit/metadata/test_single_table.py

@@ -47,6 +47,12 @@ class TestSingleTableMetadata:
         re.escape("Invalid values '(anonymization, order_by)' for phone_number column 'phone'."))
    ]

+    NUMERICAL_REPRESENTATIONS = [


can you just access this from the class like SingleTableMetadata._NUMERICAL_REPRESENTATIONS

amontanez24 · 2022-07-21T17:19:45Z

sdv/metadata/single_table.py

@@ -29,18 +29,18 @@ class SingleTableMetadata:
        'b': 'boolean',
        'M': 'datetime',
    }
+
+    _NUMERICAL_REPRESENTATIONS = [


can we make this a frozenset instead of a list? That way it is immutable

amontanez24 · 2022-07-21T17:21:16Z

sdv/metadata/single_table.py

@@ -12,16 +14,86 @@
 class SingleTableMetadata:
    """Single Table Metadata class."""

+    _EXPECTED_KWARGS = {


can we make this a frozendict as well?

katxiao

lgtm!

* Implement update / add columns. *WIP * Add docstrings * Fix call functions * WIP: Unit tests in progress * Finish unit tests * Address comments * Fix multiple python version erroring * fix exception * Fix error msg * Address comments * Address comments about frozensets * Bump macos version.

pvk-developer added 5 commits July 20, 2022 16:39

Implement update / add columns. *WIP

c5dc211

Add docstrings

3f8ccf0

Fix call functions

2126d3d

WIP: Unit tests in progress

31b2c47

Finish unit tests

c9aaf44

pvk-developer marked this pull request as ready for review July 20, 2022 19:29

pvk-developer requested a review from a team as a code owner July 20, 2022 19:29

pvk-developer requested review from katxiao and amontanez24 and removed request for a team July 20, 2022 19:29

amontanez24 reviewed Jul 20, 2022

View reviewed changes

Address comments

8e262fd

pvk-developer added 3 commits July 21, 2022 12:07

Fix multiple python version erroring

169020a

fix exception

cd6829e

Fix error msg

1626b89

katxiao reviewed Jul 21, 2022

View reviewed changes

Address comments

72913f2

amontanez24 approved these changes Jul 21, 2022

View reviewed changes

pvk-developer added 2 commits July 21, 2022 20:43

Address comments about frozensets

7272d89

Bump macos version.

214a504

katxiao approved these changes Jul 21, 2022

View reviewed changes

pvk-developer merged commit 106621f into V1.0.0.dev Jul 22, 2022

pvk-developer deleted the issue-877-add-update-column-and-add-column-to-stm branch July 22, 2022 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `update_column` and `add_column` to `SingleTableMetadata` #915

Add `update_column` and `add_column` to `SingleTableMetadata` #915

pvk-developer commented Jul 20, 2022

amontanez24 left a comment

amontanez24 Jul 20, 2022

pvk-developer Jul 20, 2022

amontanez24 Jul 20, 2022

pvk-developer Jul 20, 2022

codecov-commenter commented Jul 21, 2022 •

edited

Loading

katxiao Jul 21, 2022

katxiao Jul 21, 2022

amontanez24 left a comment

amontanez24 Jul 21, 2022

amontanez24 Jul 21, 2022

amontanez24 Jul 21, 2022

katxiao left a comment

Add update_column and add_column to SingleTableMetadata #915

Add update_column and add_column to SingleTableMetadata #915

Conversation

pvk-developer commented Jul 20, 2022

amontanez24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 21, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amontanez24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katxiao left a comment

Choose a reason for hiding this comment

Add `update_column` and `add_column` to `SingleTableMetadata` #915

Add `update_column` and `add_column` to `SingleTableMetadata` #915

codecov-commenter commented Jul 21, 2022 •

edited

Loading