Python: Implement Support for DynamoDB Catalog #6646

arminnajafi · 2023-01-23T03:10:41Z

Fixes #6541

arminnajafi · 2023-01-26T05:50:49Z

@Fokko @jackye1995
Ready for review

python/mkdocs/docs/configuration.md

python/pyiceberg/catalog/base_aws_catalog.py

python/pyiceberg/catalog/hive.py

JonasJ-ap

@arminnajafi Thank you for the implementation of DynamoDB Catalog and the refactor to other catalogs.

python/tests/catalog/__init__.py

python/pyiceberg/catalog/dynamodb.py

python/pyiceberg/catalog/__init__.py

arminnajafi · 2023-01-31T06:36:40Z

@JonasJ-ap
Thanks for reviewing this PR and also your Glue Catalog PR. This work is mostly based on that PR.

Fokko

Hey @arminnajafi! First of all, sorry for my super late reply to this. I was traveling last week, and this PR needs some concentration. I also had to set up a dynamodb catalog, and got it working:

➜  arrow git:(fd-fix-drop-docstring) ✗ python3
Python 3.10.9 (main, Dec 15 2022, 17:11:09) [Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from pyiceberg.catalog import load_catalog
>>> from pyiceberg.expressions import GreaterThanOrEqual, And, LessThan
>>> 
>>> catalog = load_catalog('dynamo')
>>> 
>>> tbl = catalog.load_table('nyc.taxis')
>>> 
>>> tbl.scan(row_filter=And(
...     GreaterThanOrEqual("tpep_pickup_datetime", "2021-04-01T00:00:00.000000+00:00"),
...     LessThan("tpep_pickup_datetime", "2021-05-01T00:00:00.000000+00:00")
... )).to_arrow().to_pandas()
Empty DataFrame
Columns: [VendorID, tpep_pickup_datetime, tpep_dropoff_datetime, passenger_count, trip_distance, RatecodeID, store_and_fwd_flag, PULocationID, DOLocationID, payment_type, fare_amount, extra, mta_tax, tip_amount, tolls_amount, improvement_surcharge, total_amount, congestion_surcharge]
Index: []
>>> tbl = catalog.load_table('nyc.taxis')
>>> tbl.scan().to_arrow().to_pandas()
       VendorID tpep_pickup_datetime tpep_dropoff_datetime passenger_count  ... tolls_amount improvement_surcharge total_amount congestion_surcharge
0             1  2020-04-01 00:41:22   2020-04-01 01:01:53               1  ...            0                   0.3          6.8                    0
1             1  2020-04-01 00:56:00   2020-04-01 01:09:25               1  ...            0                   0.3        16.55                    0
2             1  2020-04-01 00:00:26   2020-04-01 00:09:25               1  ...            0                   0.3         14.8                  2.5
3             1  2020-04-01 00:24:38   2020-04-01 00:34:38               0  ...            0                   0.3         14.8                  2.5
4             2  2020-04-01 00:13:24   2020-04-01 00:18:26               1  ...            0                   0.3         13.3                  2.5
...         ...                  ...                   ...             ...  ...          ...                   ...          ...                  ...
237988     None  2020-04-30 19:47:00   2020-04-30 20:07:00            None  ...            0                   0.3        29.62                    0
237989     None  2020-04-30 19:34:00   2020-04-30 19:39:00            None  ...            0                   0.3         5.85                    0
237990     None  2020-04-30 19:58:14   2020-04-30 20:03:47            None  ...            0                   0.3        10.89                  2.5
237991     None  2020-04-30 19:35:15   2020-04-30 19:56:51            None  ...         6.12                   0.3        31.84                  2.5
237992     None  2020-04-30 19:27:47   2020-04-30 19:45:00            None  ...            0                   0.3        17.21                    0

[237993 rows x 18 columns]

I've done some basic tests and will dig into the code in detail tomorrow. But it is looking great already

python/mkdocs/docs/configuration.md

python/pyiceberg/catalog/__init__.py

python/pyiceberg/catalog/dynamodb.py

python/pyproject.toml

python/pyiceberg/catalog/dynamodb.py

Fokko

Just a few more comments, but it is looking great! Thanks @arminnajafi for working on this, and special thanks for consolidating overlapping logic between the different catalogs 👏🏻

python/pyiceberg/catalog/dynamodb.py

python/pyiceberg/exceptions.py

arminnajafi · 2023-02-13T00:55:49Z

Thanks for the review @Fokko !

I addressed most of the comments in the new commits but also asked a few questions.
Let me know whenever you get a chance to go over them.
It might be easier to discuss some of them in Slack.

Fokko

LGTM, thanks for working on this @arminnajafi

jackye1995

looks good to me! Thanks for all the refactoring!

JonasJ-ap

Overall LGTM! Thanks for all your work. I have a followup regarding to the changes made to the list_namespaces method

JonasJ-ap · 2023-02-14T18:56:36Z

python/pyiceberg/catalog/glue.py

        # Glue does not support hierarchical namespace, therefore return an empty list
-        if namespace:
+        tuple_identifier = Catalog.identifier_to_tuple(namespace)
+        if len(tuple_identifier) > 1:


Sorry but I may not fully understand the change here. Why is the condition >1 instead of >0?

It seems now if I call GlueCatalog.list_namespaces("test_database") I will still get a list of databases instead of an empty list so that the CLI can switch to list_tables("test_database") .

Related discussion: #6646 (comment)

If this is intended, currently the test_list_namespaces in both integration_test_dynamodb.py and integration_test_glue.py may need some changes to reflect this.

For example:
If I run pyiceberg list default, now I will get all my databases:

(pyiceberg-py3.9) (base) ➜ python git:(support-ddb-catalog) pyiceberg list default default delta_lake_demo delta_playgrounddb hudi_playgrounddb migrate_delta_to_iceberg_demo migrated_to_iceberg_demo mygluedatabase mygluedatabase2 ...

but previously I got all my tables in default database:

(pyiceberg-py3.9) (base) ➜ python git:(master) pyiceberg list default default.cloudtrail_logs_aws_cloudtrail_logs_481640105715_2dd430ba default.cloudtrail_logs_aws_cloudtrail_logs_481640105715_de0525cc default.table100 default.test2 default.test21 default.test22 default.test23 default.test24 default.test27 default.test28 default.test29 default.test3 ...

Good catch. Changed it to what it was before:

if namespace: return []

JonasJ-ap · 2023-02-14T18:59:13Z

python/tests/catalog/integration_test_dynamodb.py

+    for database_name in database_list:
+        assert (database_name,) in db_list
+
+    with pytest.raises(ValidationError, match="This API is not supported for hierarchical namespaces."):


Shall we change this test to accomodate the changes made in list_namespaces discussed :#6646 (comment)

Good catch. Fixed. I completely missed checking integ tests for the previous push and GitHub actions didn't run them either.

Fokko · 2023-02-15T09:32:00Z

Merging this, thanks for the awesome work @arminnajafi! And thanks @jackye1995 @JonasJ-ap for the reviews, much appreciated!

* Implement support for DynamoDB catalog * Python: Support DynamoDB catalog apache#6541 * Fix licence * Remove BaseAwsCatalog class * Add custom DynamoDB table name * Remove __init__.py from tests/catalog/ * Apply comments from PR * Rebase and fix tests * Return empty list for hierarchical namespace * Return empty list for hierarchical namespace * Apply comments from PR * Rebase and fix circular import * Fix linter issue * Fix integ tests

github-actions bot added the python label Jan 23, 2023

arminnajafi force-pushed the support-ddb-catalog branch from c8e086e to 0f1983f Compare January 26, 2023 05:38

arminnajafi marked this pull request as ready for review January 26, 2023 05:50

jackye1995 reviewed Jan 26, 2023

View reviewed changes

python/mkdocs/docs/configuration.md Show resolved Hide resolved

jackye1995 reviewed Jan 26, 2023

View reviewed changes

python/pyiceberg/catalog/base_aws_catalog.py Outdated Show resolved Hide resolved

jackye1995 reviewed Jan 26, 2023

View reviewed changes

python/pyiceberg/catalog/hive.py Show resolved Hide resolved

arminnajafi force-pushed the support-ddb-catalog branch 2 times, most recently from 65fdfd9 to 9004de2 Compare January 27, 2023 09:25

JonasJ-ap reviewed Jan 27, 2023

View reviewed changes

python/tests/catalog/__init__.py Outdated Show resolved Hide resolved

python/pyiceberg/catalog/dynamodb.py Outdated Show resolved Hide resolved

python/pyiceberg/catalog/dynamodb.py Outdated Show resolved Hide resolved

python/pyiceberg/catalog/__init__.py Show resolved Hide resolved

Fokko added this to the Python 0.4.0 release milestone Jan 30, 2023

arminnajafi force-pushed the support-ddb-catalog branch from 9004de2 to ea453de Compare January 31, 2023 07:00

Fokko reviewed Feb 6, 2023

View reviewed changes

Fokko reviewed Feb 7, 2023

View reviewed changes

Fokko changed the title ~~Implement Support for DynamoDB Catalog~~ Python: Implement Support for DynamoDB Catalog Feb 7, 2023

arminnajafi force-pushed the support-ddb-catalog branch from ea453de to 306896f Compare February 13, 2023 00:41

arminnajafi force-pushed the support-ddb-catalog branch from 5002fa7 to 78cc8fc Compare February 14, 2023 09:24

Fokko approved these changes Feb 14, 2023

View reviewed changes

jackye1995 approved these changes Feb 14, 2023

View reviewed changes

JonasJ-ap reviewed Feb 14, 2023

View reviewed changes

Armin Najafi added 8 commits February 14, 2023 16:32

Implement support for DynamoDB catalog

4387dce

Python: Support DynamoDB catalog apache#6541

3cebe1f

Fix licence

ac1cf12

Remove BaseAwsCatalog class

530b9ef

Add custom DynamoDB table name

f4fcd67

Remove __init__.py from tests/catalog/

83d96c2

Apply comments from PR

44d0dc9

Rebase and fix tests

b4b4afd

Armin Najafi added 6 commits February 14, 2023 16:32

Return empty list for hierarchical namespace

0d4da18

Return empty list for hierarchical namespace

cf3b90a

Apply comments from PR

5e003bc

Rebase and fix circular import

c2fafc4

Fix linter issue

e657b1f

Fix integ tests

9d7be15

arminnajafi force-pushed the support-ddb-catalog branch from 78cc8fc to 9d7be15 Compare February 15, 2023 00:32

JonasJ-ap approved these changes Feb 15, 2023

View reviewed changes

Fokko merged commit 18d45b4 into apache:master Feb 15, 2023

arminnajafi deleted the support-ddb-catalog branch February 16, 2023 20:20

Python: Implement Support for DynamoDB Catalog #6646

Python: Implement Support for DynamoDB Catalog #6646

Uh oh!

Conversation

arminnajafi commented Jan 23, 2023

Uh oh!

arminnajafi commented Jan 26, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JonasJ-ap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arminnajafi commented Jan 31, 2023

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arminnajafi commented Feb 13, 2023

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

jackye1995 left a comment

Choose a reason for hiding this comment

Uh oh!

JonasJ-ap left a comment

Choose a reason for hiding this comment

Uh oh!

JonasJ-ap Feb 14, 2023

Choose a reason for hiding this comment

Uh oh!

JonasJ-ap Feb 14, 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!

arminnajafi Feb 15, 2023

Choose a reason for hiding this comment

Uh oh!

JonasJ-ap Feb 14, 2023

Choose a reason for hiding this comment

Uh oh!

arminnajafi Feb 15, 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!

Fokko commented Feb 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JonasJ-ap Feb 14, 2023 •

edited

Loading

arminnajafi Feb 15, 2023 •

edited

Loading