[FEATURE] Experimental Splitter connection testing #7051

NathanFarmer · 2023-02-05T01:59:10Z

Changes proposed in this pull request:

GREAT-1490
Add connection_test for ColumnSplitter
Leverage Generic TypeVar ColumnSplitterType in anticipation of additional Splitter classes/methods and the need for connection testing for TableAsset splitters, but not QueryAsset splitters
Refactor Sqlite splitters to bring in line with other splitter patterns and remove isinstance/type ignores
Introduce Generic TypeVar ExecutionEngineType for passing along Datasource.get_execution_engine() typing information
Make Datasource test_connection error message unit test less brittle

Definition of Done

My code follows the Great Expectations style guide
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added unit tests where applicable and made sure that new and existing tests are passing.
I have run any local integration tests and made sure that nothing is broken.

netlify · 2023-02-05T01:59:16Z

✅ Deploy Preview for niobium-lead-7998 ready!

Name	Link
🔨 Latest commit	`54eb852`
🔍 Latest deploy log	https://app.netlify.com/sites/niobium-lead-7998/deploys/63e16fd99eed300008278b2a
😎 Deploy Preview	https://deploy-preview-7051--niobium-lead-7998.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

ghost · 2023-02-05T02:01:14Z

👇 Click on the image for a new way to code review

Make big changes easier — review code in small groups of related files
Know where to start — see the whole change at a glance
Take a code tour — explore the change with an interactive tour
Make comments and review — all fully sync’ed with github

Try it now!

Legend

…:great-expectations/great_expectations into f/GREAT-1490/splitter-connection-testing

NathanFarmer · 2023-02-06T15:31:46Z

great_expectations/experimental/datasources/sqlite_datasource.py

 )


-class _SplitterMixin:
+class SqliteTableAsset(TableAsset):


Hopefully I'm not missing anything regarding the benefits of these mixins, but it doesn't seem as scalable as we will likely continue to require dialect-specific SQLAssets.

Also, moving to inheritance gets rid of the isinstance checks and type: ignores.

It was about removing verbosity but this looks better to me.

…:great-expectations/great_expectations into f/GREAT-1490/splitter-connection-testing

great_expectations/experimental/datasources/interfaces.py

great_expectations/experimental/datasources/sql_datasource.py

alexsherstinsky · 2023-02-06T16:56:03Z

great_expectations/experimental/datasources/sql_datasource.py

@@ -278,7 +303,9 @@ def get_batch_list_from_batch_request(
                )
            # Creating the batch_spec is our hook into the execution engine.
            batch_spec = SqlAlchemyDatasourceBatchSpec(**batch_spec_kwargs)
-            execution_engine: ExecutionEngine = self.datasource.get_execution_engine()
+            execution_engine: SqlAlchemyExecutionEngine = (


❤️ for type narrowing here as well as in the file-path (Pandas, Spark) cases!

alexsherstinsky

@NathanFarmer I am approving, but please do not merge until someone else with more experience in the ZEP world provides feedback as well. I left a note for consideration. For a slightly longer term (within a couple of days), I am also working on a hypothesis for leveraging Data Connectors within the new Asset in the file path modules (Pandas, Spark), which, if works, might entail some beneficial refactoring opportunities in the SQL ZEP modules as well. Thanks!

great_expectations/experimental/datasources/sql_datasource.py

Kilo59 · 2023-02-06T17:49:55Z

Everything looks good.
I especially like all the improved generics work 🚀 .

The only thing I might consider blocking is the SQLAsset type-alias, which I'd like to handle differently.
#7051 (comment)

…:great-expectations/great_expectations into f/GREAT-1490/splitter-connection-testing

billdirks · 2023-02-06T22:06:07Z

great_expectations/experimental/datasources/interfaces.py

@@ -335,22 +338,22 @@ class Datasource(
    }
    # Setting this in a Datasource subclass will override the execution engine type.
    # The primary use case is to inject an execution engine for testing.
-    execution_engine_override: ClassVar[Optional[Type[ExecutionEngine]]] = None
+    execution_engine_override: ClassVar[Optional[Type[_ExecutionEngineT]]] = None  # type: ignore[misc]


What are the details behind this new type ignore? That is, I am curious what the error message is if you don't have this.

Message: ClassVar cannot contain type variables. I added a comment in this PR: #7010

great_expectations/experimental/datasources/sql_datasource.py

billdirks · 2023-02-06T22:18:53Z

great_expectations/experimental/datasources/sqlite_datasource.py

 )


-class _SplitterMixin:
+class SqliteTableAsset(TableAsset):


It was about removing verbosity but this looks better to me.

… of https://github.com/great-expectations/great_expectations into f/dx-244/dx-247/sql-column-pair-and-multi-column-id-pk * 'f/dx-244/dx-247/sql-column-pair-and-multi-column-id-pk' of https://github.com/great-expectations/great_expectations: [FEATURE] ID/PK `ColumnPairExpectations` and `MultiColumnMapExpectations` - Spark (#7001) [FEATURE] Experimental Splitter connection testing (#7051) [DOCS] Add new `DataContext` CRUD to public API (#7058) [FEATURE] ZEP - generate pandas assets (#7044) [MAINTENANCE] fix - `FutureWarning: pandas.Float64Index is deprecated` when importing `great_expectations` (#7055) [MAINTENANCE] finish linting `great_expectations` (#7050) [MAINTENANCE] ruff `0.0.241` (#7048) [MAINTENANCE] Standardize `Checkpoint` CRUD (#6962) [MAINTENANCE] replace `isort` with `ruff` sorting rules (#6907) [MAINTENANCE] Minor Cleanup (#7047) [FEATURE] Experimental `DataAsset` `test_connection` (#7019) [FEATURE] Add SQL query data asset for new experimental datasources (#6999)

NathanFarmer added 7 commits February 3, 2023 12:27

Add test_connection stub to ColumnSplitter

377f220

raise NotImplementedError on abstract test_connection method

597ab09

Add test_connection logic to ColumnSplitter

063b1a4

Allow table_name to be a sqlalchemy.sql.quoted_name

c0b091f

Merge branch 'develop' into f/GREAT-1490/splitter-connection-testing

5f68260

Use qualified name in TableAsset.test_connection() error messages

03d146d

Add splitter testing and improve types

8ddc5eb

github-actions bot added core-team platform labels Feb 5, 2023

NathanFarmer and others added 19 commits February 4, 2023 22:41

Remove test helper method for column splitters

a26ca2b

Unused imports

2354a12

Merge branch 'develop' into f/GREAT-1490/splitter-connection-testing

3945bac

Fix bug due to wrong variable name

355c3a6

Merge branch 'f/GREAT-1490/splitter-connection-testing' of github.com…

4f76772

…:great-expectations/great_expectations into f/GREAT-1490/splitter-connection-testing

Incorrect mock return value for get_columns

5be5e67

Correct type hint in mock

31fbe58

Use built-in annotations

2c0f2da

Remove unneccessary field in mixin

c943794

Introduce Generic TypeVar ColumnSplitterType

16ab5b5

Replace YearMonthSplitters with generic ColumnSplitter

ea7f0f1

Ruff import sorting

0363b87

Merge branch 'develop' into f/GREAT-1490/splitter-connection-testing

fbd6204

Remove unused TypeVar

188e82a

Make error message unit test less brittle

3086cd3

Unused type ignore

b7da47e

Merge branch 'f/GREAT-1490/splitter-connection-testing' of github.com…

081ff70

…:great-expectations/great_expectations into f/GREAT-1490/splitter-connection-testing

Lose unneccessary variable

a4e13ca

Update schemas

d29e456

NathanFarmer commented Feb 6, 2023

View reviewed changes

NathanFarmer and others added 3 commits February 6, 2023 10:34

Merge branch 'develop' into f/GREAT-1490/splitter-connection-testing

dd1e6f4

One more comment

b449121

Merge branch 'f/GREAT-1490/splitter-connection-testing' of github.com…

0e0bd2b

…:great-expectations/great_expectations into f/GREAT-1490/splitter-connection-testing

NathanFarmer marked this pull request as ready for review February 6, 2023 16:45

NathanFarmer requested review from billdirks, cdkini, Kilo59 and alexsherstinsky February 6, 2023 16:45

Kilo59 reviewed Feb 6, 2023

View reviewed changes

great_expectations/experimental/datasources/interfaces.py Outdated Show resolved Hide resolved

alexsherstinsky reviewed Feb 6, 2023

View reviewed changes

great_expectations/experimental/datasources/sql_datasource.py Show resolved Hide resolved

alexsherstinsky reviewed Feb 6, 2023

View reviewed changes

alexsherstinsky approved these changes Feb 6, 2023

View reviewed changes

Merge branch 'develop' into f/GREAT-1490/splitter-connection-testing

4faec79

Kilo59 reviewed Feb 6, 2023

View reviewed changes

great_expectations/experimental/datasources/sql_datasource.py Outdated Show resolved Hide resolved

NathanFarmer and others added 4 commits February 6, 2023 13:49

Revert SQLAsset TypeAlias change

d81d162

Revert SQLAsset TypeAlias change

106ab1a

Merge branch 'f/GREAT-1490/splitter-connection-testing' of github.com…

8dd8fd4

…:great-expectations/great_expectations into f/GREAT-1490/splitter-connection-testing

Merge branch 'develop' into f/GREAT-1490/splitter-connection-testing

374c701

Kilo59 added the zep Zero Entry Pool work label Feb 6, 2023

Kilo59 approved these changes Feb 6, 2023

View reviewed changes

NathanFarmer and others added 3 commits February 6, 2023 16:22

Rename TypeVars in experimental

e30728c

Merge branch 'f/GREAT-1490/splitter-connection-testing' of github.com…

8f3a1c2

…:great-expectations/great_expectations into f/GREAT-1490/splitter-connection-testing

Merge branch 'develop' into f/GREAT-1490/splitter-connection-testing

54eb852

NathanFarmer enabled auto-merge (squash) February 6, 2023 21:23

NathanFarmer merged commit 69ad938 into develop Feb 6, 2023

NathanFarmer deleted the f/GREAT-1490/splitter-connection-testing branch February 6, 2023 22:19

billdirks reviewed Feb 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[FEATURE] Experimental Splitter connection testing #7051

[FEATURE] Experimental Splitter connection testing #7051

Uh oh!

NathanFarmer commented Feb 5, 2023 •

edited

Loading

Uh oh!

netlify bot commented Feb 5, 2023 •

edited

Loading

Uh oh!

ghost commented Feb 5, 2023 •

edited by ghost

Loading

Uh oh!

NathanFarmer Feb 6, 2023 •

edited

Loading

Uh oh!

NathanFarmer Feb 6, 2023

Uh oh!

billdirks Feb 6, 2023

Uh oh!

Uh oh!

Uh oh!

alexsherstinsky Feb 6, 2023

Uh oh!

alexsherstinsky left a comment

Uh oh!

Uh oh!

Kilo59 commented Feb 6, 2023

Uh oh!

billdirks Feb 6, 2023

Uh oh!

NathanFarmer Feb 7, 2023

Uh oh!

Uh oh!

billdirks Feb 6, 2023

Uh oh!

Uh oh!

[FEATURE] Experimental Splitter connection testing #7051

[FEATURE] Experimental Splitter connection testing #7051

Uh oh!

Conversation

NathanFarmer commented Feb 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Definition of Done

Uh oh!

netlify bot commented Feb 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for niobium-lead-7998 ready!

Uh oh!

ghost commented Feb 5, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Legend

Uh oh!

NathanFarmer Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NathanFarmer Feb 6, 2023

Choose a reason for hiding this comment

Uh oh!

billdirks Feb 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alexsherstinsky Feb 6, 2023

Choose a reason for hiding this comment

Uh oh!

alexsherstinsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Kilo59 commented Feb 6, 2023

Uh oh!

billdirks Feb 6, 2023

Choose a reason for hiding this comment

Uh oh!

NathanFarmer Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

billdirks Feb 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NathanFarmer commented Feb 5, 2023 •

edited

Loading

netlify bot commented Feb 5, 2023 •

edited

Loading

ghost commented Feb 5, 2023 •

edited by ghost

Loading

NathanFarmer Feb 6, 2023 •

edited

Loading