fix: "PostgresMetadataExtractor doesn't discover Redshift late binding views"

[nathanlawrence-asana]

Summary of Changes

Splits PostgresMetadataExtractor into BasePostgresMetadataExtractor, PostgresMetadataExtractor and RedshiftMetadataExtractor (the latter two inherit from BasePostgresMetadataExtractor).

This allows us to safely modify the SQL_QUERY constant for Redshift-based deployments to pull late binding views into Amundsen.

See amundsen-io/amundsen#672

Tests

I'm happy to write tests once folks are happy with the general structure of the PR (this is my first time contributing :) ).

My understanding is that I should be adding a new test for the RedshiftMetadataExtractor similar to what we have currently for the PostgresMetadataExtractor.

Documentation

I'm happy to update documentation once folks are happy with the general structure of the PR (this is my first time contributing :) ).

My understanding is that the main place that needs to be updated is adding a new section to 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
• PR passes make test

[feng-tao]

some checks are failing

[feng-tao]

thanks for the contribution. Doe the refactor / new feature cover by the existing unit test?

[nathanlawrence-asana]

thanks for the contribution. Doe the refactor / new feature cover by the existing unit test?

It's not, and I'm not sure what the best way to test this is. The underlying issue is that the SQL we're running to pull tables and views in Redshift misses a category of views.

From looking at the unit tests currently in Amundsen, none of them actually run any SQL (they just mock the results from SQL Alchemy). I could write some tests for the new RedshiftMetadataExtractor that fit that pattern, but they wouldn't really be testing the underlying logic of the change.

P.S. I'm also on the Amundsen Slack (Nathan Lawrence) if you'd like to chat synchronously sometime tomorrow. I work 9-5pm pacific and I'm mostly free tomorrow.

[nathanlawrence-asana]

I'll also add that I've tested this on a local installation and verified that the changes work as expected.

[feng-tao]

yeah, I think it is fine, if it is a SQL change. I will take a look at the pr in detail. thanks for the clarification.

[feng-tao]

the change lgtm (I don't know much about the actual SQL, but based on your local testing, I assume it should work as expected), if you could add some basic unit tests, we are good to go!

[nathanlawrence-asana]

Thanks. Updated to copy over some of the Postgres tests and ensure that they work with the similar Redshift extractor.

In the process, I made some updates to the Postgres tests. The scope that we were passing the extractor was wrong, and the tests were really just using the default config parameters for most tests. This only showed up when I copied the tests over because the database key check started to fail (checking against the passed redshift vs. the default postgres).

[feng-tao]

thanks @nathanlawrence-asana