Add a utility to drop unknown references (and enforce referential integrity) #1800

R-Palazzo · 2024-02-20T16:13:59Z

CU-86azc5bc2
Resolve #1792

sdv-team · 2024-02-20T16:14:02Z

Task linked: CU-86azc5bc2 SDV - Add a utility to drop unknown references (and enforce referential integrity) #1792

codecov-commenter · 2024-02-20T16:28:47Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (ea4c2cc) 97.26% compared to head (d1777fc) 97.27%.

❗ Current head d1777fc differs from pull request most recent head f110a87. Consider uploading reports for the commit f110a87 to get more accurate results

Files	Patch %	Lines
sdv/utils.py	95.23%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1800      +/-   ##
==========================================
+ Coverage   97.26%   97.27%   +0.01%     
==========================================
  Files          49       50       +1     
  Lines        4679     4733      +54     
==========================================
+ Hits         4551     4604      +53     
- Misses        128      129       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sdv/_utils.py

frances-h · 2024-02-21T20:52:40Z

sdv/_utils.py

+
+
+def _get_relationship_idx_for_child(relationships, child_table):
+    return [idx for idx, rel in enumerate(relationships) if rel['child_table_name'] == child_table]


Do we actually need the index here or can we pass the relationship itself?

Yes, done in 995a684

rwedge · 2024-02-21T22:22:55Z

The issue mentions doing input validation on the provided data / metadata. It doesn't look like this is present.

R-Palazzo · 2024-02-22T10:22:05Z

@rwedge thanks for the comment. Done in 462ecf2

frances-h

Looks good! Just a couple of small nitpicks

frances-h · 2024-02-22T14:19:04Z

sdv/_utils.py

@@ -200,3 +201,80 @@ def _format_invalid_values_string(invalid_values, num_values):
            return f'{invalid_values[:num_values] + extra_missing_values}'

    return f'{invalid_values}'
+
+
+def _find_root_tables(relationships):


Can we rename this to _get_root_tables?

Yes, done in 3c640b1

frances-h · 2024-02-22T14:19:19Z

sdv/_utils.py

+
+def _get_relationship_idx_for_parent(relationships, parent_table):
+    return [
+        idx for idx, rel in enumerate(relationships) if rel['parent_table_name'] == parent_table


Same thing, do we need the index here or can we just pass the relationship?

Yes, done in f110a87

frances-h · 2024-02-22T14:28:36Z

sdv/_utils.py

+            relationship_idx = _get_relationship_idx_for_parent(relationships, root)
+            for idx in relationship_idx:
+                relationship = relationships[idx]
+                parent_table = relationship['parent_table_name']


Since this is just the root table, can we move this outside of the for loop over the children?

Yes, done in bff9b0a

frances-h · 2024-02-22T14:28:55Z

sdv/_utils.py

+                relationship = relationships[idx]
+                parent_table = relationship['parent_table_name']
+                child_table = relationship['child_table_name']
+                parent_column = relationship['parent_primary_key']


Same as above, this shouldn't change for any of the children.

Yes, done in bff9b0a

frances-h · 2024-02-22T14:30:01Z

sdv/_utils.py

+                valid_parent_idx = [
+                    idx for idx in data[parent_table].index if idx not in table_to_idx_to_drop.get(
+                        parent_table, set()
+                    )
+                ]
+                valid_parent_values = set(data[parent_table].loc[valid_parent_idx, parent_column])


Can we move this to doing it once outside of the for loop as well? Once we're evaluating it as a root table, the valid parent values shouldn't change again, right?

Yes, done in d1777fc

frances-h · 2024-02-22T14:46:42Z

sdv/_utils.py

+                if child_table not in table_to_idx_to_drop:
+                    table_to_idx_to_drop[child_table] = set()


Instead of setting this here you could make table_to_idx_to_drop a defaultdict(set), then you could also change the table_to_idx_to_drop.get below to just indexing in directly.

Good point, thanks, done in 8b7c528

frances-h

Looks good! Thanks for addressing

frances-h · 2024-02-25T22:47:42Z

sdv/_utils.py

+def _get_relationship_for_child(relationships, child_table):
+    return [rel for rel in relationships if rel['child_table_name'] == child_table]
+
+
+def _get_relationship_for_parent(relationships, parent_table):
+    return [rel for rel in relationships if rel['parent_table_name'] == parent_table]


nitpick: these probably don't need to be helper functions anymore.

frances-h · 2024-02-25T22:50:59Z

sdv/utils.py

+        metadata.validate_data(data)
+        return data


Can we also just add a unit test to make sure that valid data is returned as-is?

R-Palazzo requested review from amontanez24 and frances-h February 20, 2024 16:13

R-Palazzo requested a review from a team as a code owner February 20, 2024 16:14

R-Palazzo removed the request for review from a team February 20, 2024 16:14

R-Palazzo force-pushed the issue-1792-add-drop_unknown_references branch from 1b2c7f2 to cb8a21b Compare February 21, 2024 13:58

amontanez24 requested review from rwedge and removed request for amontanez24 February 21, 2024 16:43

frances-h reviewed Feb 21, 2024

View reviewed changes

sdv/_utils.py Outdated Show resolved Hide resolved

frances-h reviewed Feb 21, 2024

View reviewed changes

Base automatically changed from issue-1793-functions-private to main February 22, 2024 09:00

R-Palazzo added 10 commits February 22, 2024 09:03

def

a984492

unit tests

2966f96

integration tests

afa6fa4

update error message

ba92d3d

error message for empty data

af759f9

docstring

4d34771

refactor between _utils and utils

e1820a4

rebase

a11d54a

add data/metadata validation

462ecf2

_get_relationship_for_child

995a684

R-Palazzo force-pushed the issue-1792-add-drop_unknown_references branch from f3ddf79 to 995a684 Compare February 22, 2024 10:17

frances-h reviewed Feb 22, 2024

View reviewed changes

R-Palazzo added 4 commits February 22, 2024 15:12

_get_root_tables instead of _find_root_tables

3c640b1

move parent table name and primary key outside of the loop

bff9b0a

use defaultdict

8b7c528

move valid parent computation outside the loop

d1777fc

get_relationship_for_parent

f110a87

R-Palazzo requested a review from frances-h February 22, 2024 17:34

frances-h approved these changes Feb 25, 2024

View reviewed changes

frances-h reviewed Feb 25, 2024

View reviewed changes

add unit test

ecf88a8

rwedge approved these changes Feb 26, 2024

View reviewed changes

R-Palazzo merged commit c314751 into main Feb 26, 2024
37 checks passed

R-Palazzo deleted the issue-1792-add-drop_unknown_references branch February 26, 2024 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a utility to drop unknown references (and enforce referential integrity) #1800

Add a utility to drop unknown references (and enforce referential integrity) #1800

R-Palazzo commented Feb 20, 2024

sdv-team commented Feb 20, 2024

codecov-commenter commented Feb 20, 2024 •

edited

Loading

frances-h Feb 21, 2024

R-Palazzo Feb 22, 2024

rwedge commented Feb 21, 2024

R-Palazzo commented Feb 22, 2024

frances-h left a comment

frances-h Feb 22, 2024

R-Palazzo Feb 22, 2024

frances-h Feb 22, 2024

R-Palazzo Feb 22, 2024

frances-h Feb 22, 2024

R-Palazzo Feb 22, 2024

frances-h Feb 22, 2024

R-Palazzo Feb 22, 2024 •

edited

Loading

frances-h Feb 22, 2024

R-Palazzo Feb 22, 2024

frances-h Feb 22, 2024

R-Palazzo Feb 22, 2024

frances-h left a comment

frances-h Feb 25, 2024

frances-h Feb 25, 2024



		def _get_relationship_idx_for_child(relationships, child_table):
		return [idx for idx, rel in enumerate(relationships) if rel['child_table_name'] == child_table]

		if child_table not in table_to_idx_to_drop:
		table_to_idx_to_drop[child_table] = set()

Add a utility to drop unknown references (and enforce referential integrity) #1800

Add a utility to drop unknown references (and enforce referential integrity) #1800

Conversation

R-Palazzo commented Feb 20, 2024

sdv-team commented Feb 20, 2024

codecov-commenter commented Feb 20, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwedge commented Feb 21, 2024

R-Palazzo commented Feb 22, 2024

frances-h 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

R-Palazzo Feb 22, 2024 • edited Loading

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

frances-h left a 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 Feb 20, 2024 •

edited

Loading

R-Palazzo Feb 22, 2024 •

edited

Loading