Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add TableSerializable and mysql_serializer #459

Merged
merged 8 commits into from Mar 22, 2021
Merged

feat: Add TableSerializable and mysql_serializer #459

merged 8 commits into from Mar 22, 2021

Conversation

xuan616
Copy link
Member

@xuan616 xuan616 commented Mar 16, 2021

Summary of Changes

The PR added TableSerializable and mysql_serializer, which is the implementation of RFC adding an abstract serializable to support mysql backend

Main changes:

  • Add the abstract layer: TableSerializable
  • Add record iterators in applicable models inheriting TableSerializable
  • Add mysql_serializer
  • Add unit tests for models accordingly

Tests

Unit tests added to test the new TableSerializable, mysql_serializer and table record iterators of databuilder models

Documentation

What documentation did you add or modify and why? Add any relevant links then remove this line

CheckList

Make sure you have checked all steps below to ensure a timely review.

  • PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • PR includes a summary of changes.
  • PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
  • PR passes make test

xuan616 and others added 6 commits March 12, 2021 16:29
@xuan616
Add TableSerializable and implement record_iterators in applicable mo…
7cda0ac 
…dels

Signed-off-by: xuans <xuan_shen@outlook.com>
Add mysql serializer
30c7c53 
Signed-off-by: Sahithi Velma <sahithiv@wepay.com>
Add unit tests for dashboard and schema models
1df79df 
Signed-off-by: Sahithi Velma <sahithiv@wepay.com>
@xuan616
Add unit tests for table and user models
1fb0af2 
Signed-off-by: xuans <xuan_shen@outlook.com>
@xuan616
ignore type for Base in test_table_serializable.py
04b62e9 
Signed-off-by: xuans <xuan_shen@outlook.com>
@xuan616
Add comments and rds model package in requirements
b536e07 
Signed-off-by: xuans <xuan_shen@outlook.com>
@xuan616
Address the unit test failure
a9b38b7 
Signed-off-by: xuans <xuan_shen@outlook.com>
@feng-tao
Copy link
Member

will take a look

feng-tao
feng-tao reviewed Mar 17, 2021
View reviewed changes
Copy link
Member

@feng-tao feng-tao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crazy-2020 great work! I assume the the file_system_rds_csv_loader or publisher will be in a different pr?

also how's the test so far, are you able to run it locally to load the local mysql. Also i wonder whether we could force every model (or future model) to be subclass of both (GraphSerializable, TableSerializable)

Put a few comments as I just skim through, will take another look tomorrow.

databuilder/models/schema/schema_constant.py Outdated
@@ -9,3 +9,5 @@
SCHEMA_REVERSE_RELATION_TYPE = 'SCHEMA_OF'

DATABASE_SCHEMA_KEY_FORMAT = '{db}://{cluster}.{schema}'

SCHEMA_PATTERN_REGEX = '([a-z]+://[a-zA-Z0-9_-]+).[a-zA-Z0-9_.-]+'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a comment on this regex? (e.g like what is the example value, what we are matching for)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

databuilder/models/table_metadata.py Outdated
node = self.get_node()
yield node

def _create_relation_iterator(self) -> Iterator[RDSModel]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be Iterator[GraphRelationship] as type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, nice catch. I'll update it.

databuilder/models/table_metadata.py
@@ -99,13 +138,13 @@ def __init__(self,
:param source: The unique source of what is populating this description.
:param text: the description text. Markdown supported.
"""
self._source = source
self._text = text
self.source = source
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason we remove the underscore? I think we try to use _var to indicate it is private.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rds models do not have a generic description model. There is only description model for each entity(e.g., TableDescription, SchemaDescription). The code change is going to create description table records from databuilder Table/Schema/Column models, namely, access some attributes outside of DescriptionMetadata class. Should I change it back to protected vars?(I think it may bring some syntax highlight warning). Thanks

databuilder/models/user.py
record = RDSUser(rk=User.get_user_model_key(email=self.email))
for attr, value in record_attr_map.items():
if value or not self.do_not_update_empty_attribute:
record.__setattr__(attr.key, value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a comment on this part? Are you trying to set the user record for optional attribute?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I'll add a comment. Yes, it is going to set the empty attribute based on the flag

tests/unit/extractor/test_bigquery_metadata_extractor.py
@@ -249,7 +249,7 @@ def test_table_without_schema(self, mock_build: Any) -> None:
self.assertEqual(result.cluster, 'your-project-here')
self.assertEqual(result.schema, 'fdgdfgh')
self.assertEqual(result.name, 'nested_recs')
self.assertEqual(result.description._text, '')
self.assertEqual(result.description.text, '')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment on the rename

@xuan616
Copy link
Member Author

xuan616 commented Mar 17, 2021 •
edited

@crazy-2020 great work! I assume the the file_system_rds_csv_loader or publisher will be in a different pr?

@feng-tao Thanks for the comments. Yes, there will be a separate pr for the features(e.g., mysql_data_loader/publisher) mentioned in another RFC

also how's the test so far, are you able to run it locally to load the local mysql. Also i wonder whether we could force every model (or future model) to be subclass of both (GraphSerializable, TableSerializable)

The test was mainly based on BigQuery and csv samples in databulider. Yes, I am able to push BQ and csv sample data into mysql backend with a rough implementation of mysql_dataloader/publisher on my local.
Currently, I think I leave table_lineage to only inherit one serializable, GraphSerializable, which seems to be in development per our offline discussion. I am not sure if we will have any model that would work for only graph db or only mysql. If we won't have that case, then yes, we could force subclass to inherit both serializable.

@feng-tao
Copy link
Member

@crazy-2020 feel free to ping once you update the pr

@xuan616
Address comments
7d4fb19 
Signed-off-by: xuans <xuan_shen@outlook.com>
feng-tao
feng-tao approved these changes Mar 19, 2021
View reviewed changes
Copy link
Member

@feng-tao feng-tao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but will merge it next week in case others have comment

@feng-tao feng-tao merged commit 4bb4452 into amundsen-io:master Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feng-tao feng-tao feng-tao approved these changes

@allisonsuarez allisonsuarez Awaiting requested review from allisonsuarez allisonsuarez is a code owner

@dikshathakur3119 dikshathakur3119 Awaiting requested review from dikshathakur3119 dikshathakur3119 is a code owner

@jinhyukchang jinhyukchang Awaiting requested review from jinhyukchang jinhyukchang is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@xuan616 @feng-tao