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: Databuilder support for nested columns #1695

Merged

dkunitsk merged 18 commits into amundsen-io:main from kristenarmes:support-complex-column-types-on-backend

Feb 24, 2022

Contributor

kristenarmes commented Feb 2, 2022 •

edited by dkunitsk

Loading

Signed-off-by: Kristen Armes karmes@lyft.com

Summary of Changes

This PR adds databuilder support for nested columns by introducing a new TypeMetadata base class, along with Map/Array/Struct/Scalar subclasses (with opportunity to add more as needed). Columns will have an empty TypeMetadata by default and Amundsen will not require it for the time being. To eliminate breaking changes, setting TypeMetadata is done via the ComplexTypeTransformer. With time this logic can easily be integrated into extractors themselves.

The TypeMetadata classes are intended to be general and reusable. Syntax-specific parsers will need to be added per-database for converting type strings into structured objects. In this PR we have implemented a Hive parser that uses pyparsing.

After this we'll work on the APIs and then the frontend.

Tests

Added test cases in test_table_metadata.py, test_type_metadata.py, test_complex_type_transformer.py, test_hive_complex_type_parser.py

Documentation

Added details about the ComplexTypeTransformer in databuilder/README.md

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"
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.
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


          Type metadata classes to represent complex column types

b3c77b0

Signed-off-by: Kristen Armes <karmes@lyft.com>

boring-cyborg bot added area:databuilder category:models labels


          Adding a few more type metadata tests

0c3072d

Signed-off-by: Kristen Armes <karmes@lyft.com>

dkunitsk reviewed

View reviewed changes

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

dkunitsk reviewed

View reviewed changes

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

dkunitsk reviewed

View reviewed changes

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

dkunitsk reviewed

View reviewed changes

dkunitsk left a comment

No major problems, looks great. A few tweaks I'd suggest, let's discuss IRL.

kristenarmes added 4 commits

February 4, 2022 13:53


          Adding __eq__ implementation for the different types for easier testing

ece1376

Signed-off-by: Kristen Armes <karmes@lyft.com>


          Addressing PR comments, bringing description into base class, removin…

fa3a6de

…g extra StructItem class and using dict for structs instead, and implemented ScalarTypeMetadata to use as the terminal type rather than string

Signed-off-by: Kristen Armes <karmes@lyft.com>


          lint

c8c6cfd

Signed-off-by: Kristen Armes <karmes@lyft.com>


          mypy

31ced31

Signed-off-by: Kristen Armes <karmes@lyft.com>

dkunitsk reviewed

View reviewed changes

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

kristenarmes added 2 commits

February 9, 2022 11:00


          lint

8c2e7ad

Signed-off-by: Kristen Armes <karmes@lyft.com>


          Changes to type metadata, adding hive parser, adding generic complex …

215d037

…type transformer

Signed-off-by: Kristen Armes <karmes@lyft.com>

kristenarmes commented

View reviewed changes

databuilder/databuilder/models/description_metadata.py

		@@ -0,0 +1,150 @@
		# Copyright Contributors to the Amundsen project.

Contributor Author

kristenarmes Feb 11, 2022

Separated DescriptionMetadata from TableMetadata due to circular import issues with TypeMetadata


          lint

1b73a15

Signed-off-by: Kristen Armes <karmes@lyft.com>

dkunitsk reviewed

View reviewed changes

databuilder/databuilder/models/table_metadata.py Show resolved Hide resolved

databuilder/databuilder/models/description_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/description_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/transformer/complex_type_transformer.py Outdated Show resolved Hide resolved

kristenarmes added 2 commits

February 16, 2022 19:02


          Updating description metadata imports

b9389c6

Signed-off-by: Kristen Armes <karmes@lyft.com>


          Adding parent and name attributes to child type metadata, base type m…

5aa6cf5

…etadata can now get own key/parent key, fixing struct node handling

Signed-off-by: Kristen Armes <karmes@lyft.com>

kristenarmes changed the title ~~feat: Type metadata classes to represent complex column types~~ feat: Ability to parse complex column types, store in ColumnMetadata, and create graph nodes for them


          Adding documentation for the transformer

be6b937

Signed-off-by: Kristen Armes <karmes@lyft.com>

boring-cyborg bot added area:all area:docs labels

dkunitsk reviewed

View reviewed changes

databuilder/databuilder/models/description_metadata.py Show resolved Hide resolved

databuilder/databuilder/models/table_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

kristenarmes added 3 commits

February 22, 2022 20:26


          Changing the type metadata and parser to be created with parent and n…

814ced2

…ame instead of data_type (test and mypy fixes remaining todo)

Signed-off-by: Kristen Armes <karmes@lyft.com>


          Fixing tests to work with changes

21139b9

Signed-off-by: Kristen Armes <karmes@lyft.com>


          mypy fixes

ed2f949

Signed-off-by: Kristen Armes <karmes@lyft.com>

dkunitsk marked this pull request as ready for review

February 23, 2022 19:23

dkunitsk requested a review from a team as a code owner

February 23, 2022 19:23

dkunitsk changed the title ~~feat: Ability to parse complex column types, store in ColumnMetadata, and create graph nodes for them~~ feat: Backend support for nested columns


          Addressing PR comments

9ba71f7

Signed-off-by: Kristen Armes <karmes@lyft.com>

dkunitsk reviewed

View reviewed changes

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

dkunitsk reviewed

View reviewed changes

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

dkunitsk reviewed

View reviewed changes

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved

databuilder/databuilder/transformer/complex_type_transformer.py Show resolved Hide resolved


          Addressing PR comments and fixing up is terminal check handling

29f4e6f

Signed-off-by: Kristen Armes <karmes@lyft.com>

dkunitsk reviewed

View reviewed changes

databuilder/README.md Outdated Show resolved Hide resolved

databuilder/databuilder/models/type_metadata.py Outdated Show resolved Hide resolved


          Ceaning up map attributes and removing info from README for now

62cc060

Signed-off-by: Kristen Armes <karmes@lyft.com>

dkunitsk approved these changes

View reviewed changes

dkunitsk left a comment

very very nice. Love the test coverage also.

dkunitsk merged commit 9854948 into amundsen-io:main

kristenarmes deleted the support-complex-column-types-on-backend branch

February 24, 2022 19:11

dkunitsk changed the title ~~feat: Backend support for nested columns~~ feat: Databuilder support for nested columns

kristenarmes mentioned this pull request

chore: Bump databuilder version for backend support for nested columns #1730

Merged

4 tasks

dkunitsk mentioned this pull request

feat: Adding TypeMetadata model in amundsen-common #1732

Merged

4 tasks

ozandogrultan pushed a commit to deliveryhero/amundsen that referenced this pull request


          feat: Backend support for nested columns (amundsen-io#1695)

0bf1675

* Type metadata classes to represent complex column types

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Adding a few more type metadata tests

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Adding __eq__ implementation for the different types for easier testing

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Addressing PR comments, bringing description into base class, removing extra StructItem class and using dict for structs instead, and implemented ScalarTypeMetadata to use as the terminal type rather than string

Signed-off-by: Kristen Armes <karmes@lyft.com>

* lint

Signed-off-by: Kristen Armes <karmes@lyft.com>

* mypy

Signed-off-by: Kristen Armes <karmes@lyft.com>

* lint

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Changes to type metadata, adding hive parser, adding generic complex type transformer

Signed-off-by: Kristen Armes <karmes@lyft.com>

* lint

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Updating description metadata imports

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Adding parent and name attributes to child type metadata, base type metadata can now get own key/parent key, fixing struct node handling

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Adding documentation for the transformer

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Changing the type metadata and parser to be created with parent and name instead of data_type (test and mypy fixes remaining todo)

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Fixing tests to work with changes

Signed-off-by: Kristen Armes <karmes@lyft.com>

* mypy fixes

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Addressing PR comments

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Addressing PR comments and fixing up is terminal check handling

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Ceaning up map attributes and removing info from README for now

Signed-off-by: Kristen Armes <karmes@lyft.com>
Signed-off-by: Ozan Dogrultan <ozan.dogrultan@deliveryhero.com>

zacr pushed a commit to SaltIO/amundsen that referenced this pull request


          feat: Backend support for nested columns (amundsen-io#1695)

fc3551c

* Type metadata classes to represent complex column types

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Adding a few more type metadata tests

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Adding __eq__ implementation for the different types for easier testing

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Addressing PR comments, bringing description into base class, removing extra StructItem class and using dict for structs instead, and implemented ScalarTypeMetadata to use as the terminal type rather than string

Signed-off-by: Kristen Armes <karmes@lyft.com>

* lint

Signed-off-by: Kristen Armes <karmes@lyft.com>

* mypy

Signed-off-by: Kristen Armes <karmes@lyft.com>

* lint

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Changes to type metadata, adding hive parser, adding generic complex type transformer

Signed-off-by: Kristen Armes <karmes@lyft.com>

* lint

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Updating description metadata imports

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Adding parent and name attributes to child type metadata, base type metadata can now get own key/parent key, fixing struct node handling

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Adding documentation for the transformer

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Changing the type metadata and parser to be created with parent and name instead of data_type (test and mypy fixes remaining todo)

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Fixing tests to work with changes

Signed-off-by: Kristen Armes <karmes@lyft.com>

* mypy fixes

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Addressing PR comments

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Addressing PR comments and fixing up is terminal check handling

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Ceaning up map attributes and removing info from README for now

Signed-off-by: Kristen Armes <karmes@lyft.com>
Signed-off-by: Zachary Ruiz <zacharyruiz@Zacharys-MacBook-Pro-2.local>

zacr pushed a commit to SaltIO/amundsen that referenced this pull request


          feat: Backend support for nested columns (amundsen-io#1695)

bc59677

* Type metadata classes to represent complex column types

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Adding a few more type metadata tests

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Adding __eq__ implementation for the different types for easier testing

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Addressing PR comments, bringing description into base class, removing extra StructItem class and using dict for structs instead, and implemented ScalarTypeMetadata to use as the terminal type rather than string

Signed-off-by: Kristen Armes <karmes@lyft.com>

* lint

Signed-off-by: Kristen Armes <karmes@lyft.com>

* mypy

Signed-off-by: Kristen Armes <karmes@lyft.com>

* lint

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Changes to type metadata, adding hive parser, adding generic complex type transformer

Signed-off-by: Kristen Armes <karmes@lyft.com>

* lint

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Updating description metadata imports

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Adding parent and name attributes to child type metadata, base type metadata can now get own key/parent key, fixing struct node handling

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Adding documentation for the transformer

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Changing the type metadata and parser to be created with parent and name instead of data_type (test and mypy fixes remaining todo)

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Fixing tests to work with changes

Signed-off-by: Kristen Armes <karmes@lyft.com>

* mypy fixes

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Addressing PR comments

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Addressing PR comments and fixing up is terminal check handling

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Ceaning up map attributes and removing info from README for now

Signed-off-by: Kristen Armes <karmes@lyft.com>

hansadriaans pushed a commit to DataChefHQ/amundsen that referenced this pull request


          feat: Backend support for nested columns (amundsen-io#1695)

df2593b

* Type metadata classes to represent complex column types

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Adding a few more type metadata tests

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Adding __eq__ implementation for the different types for easier testing

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Addressing PR comments, bringing description into base class, removing extra StructItem class and using dict for structs instead, and implemented ScalarTypeMetadata to use as the terminal type rather than string

Signed-off-by: Kristen Armes <karmes@lyft.com>

* lint

Signed-off-by: Kristen Armes <karmes@lyft.com>

* mypy

Signed-off-by: Kristen Armes <karmes@lyft.com>

* lint

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Changes to type metadata, adding hive parser, adding generic complex type transformer

Signed-off-by: Kristen Armes <karmes@lyft.com>

* lint

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Updating description metadata imports

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Adding parent and name attributes to child type metadata, base type metadata can now get own key/parent key, fixing struct node handling

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Adding documentation for the transformer

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Changing the type metadata and parser to be created with parent and name instead of data_type (test and mypy fixes remaining todo)

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Fixing tests to work with changes

Signed-off-by: Kristen Armes <karmes@lyft.com>

* mypy fixes

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Addressing PR comments

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Addressing PR comments and fixing up is terminal check handling

Signed-off-by: Kristen Armes <karmes@lyft.com>

* Ceaning up map attributes and removing info from README for now

Signed-off-by: Kristen Armes <karmes@lyft.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:all area:databuilder