Glue catalog commit table #140

HonahX · 2023-11-12T08:01:25Z

Implements _commit_table for GlueCatalog. This PR depends on:

Table metadata update: ( Update table metadata #139 )
Table requirements validation: (Table Requirements Validation #200)

Thanks in advance for your help!

…lue_commit

# Conflicts: # pyiceberg/table/__init__.py # tests/conftest.py

…lue_commit

# Conflicts: # pyiceberg/catalog/glue.py # pyiceberg/table/__init__.py # pyiceberg/table/metadata.py # tests/conftest.py # tests/table/test_init.py

…nd_glue_commit # Conflicts: # pyiceberg/table/__init__.py

…nd_glue_commit

pyiceberg/catalog/glue.py

HonahX · 2023-12-14T07:16:47Z

I'm quite busy at the moment, but I'm aiming to complete this and mark this ready by the end of the week.

pyiceberg/catalog/glue.py

HonahX · 2023-12-16T02:05:29Z

Thanks @nicor88 and @jackye1995 for the initial review. I think this PR is ready.

cc: @Fokko @pdames @svdimchenko

pyiceberg/catalog/__init__.py

jackye1995

Mostly looks good to me, just a nit regarding the doc

pyiceberg/catalog/__init__.py

Fokko

One small comment to make the regex a bit more robust, apart from that this looks good! Thanks for working on this!

pdames

LGTM - thanks @HonahX! Just a few minor questions/nits. Looking forward to trying this out w/ Glue for Ray later this week and will let you know if anything comes up (if any issues pop up, we can address them in a follow-up PR)!

pdames · 2023-12-26T22:07:16Z

pyiceberg/catalog/__init__.py

@@ -74,6 +75,8 @@
 LOCATION = "location"
 EXTERNAL_TABLE = "EXTERNAL_TABLE"

+TABLE_METADATA_FILE_NAME_REGEX = re.compile(r"""(\d+)-.*\.json""", re.X)


To better avoid potential false positives, should we change this to:

Suggested change

TABLE_METADATA_FILE_NAME_REGEX = re.compile(r"""(\d+)-.*\.json""", re.X)

TABLE_METADATA_FILE_NAME_REGEX = re.compile(r"""(\d+)-(.*)\.metadata\.json""", re.X)

Where the 2nd capturing group will also allow us to assert UUID validity later. I'm assuming we don't want to match non-UUID strings or arbitrary JSON files, but let me know if you think this is too restrictive.

Also, since we've specified the re.X flag anyway, we can make use of it by adding some comments here like:

Suggested change

TABLE_METADATA_FILE_NAME_REGEX = re.compile(r"""(\d+)-.*\.json""", re.X)

TABLE_METADATA_FILE_NAME_REGEX = re.compile(

r"""

(\d+) # version number

- # separator

(.*) # UUID

\.metadata\.json # file extension

""",

re.X

)

Thanks for the suggestion. I've updated the regex to capture the UUID. Although in java we do not check the UUID validity: https://github.com/apache/iceberg/blob/81bf8d30766b1b129b87abde15239645cb127046/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java#L382-L404
I think it make sense to add the check here

pdames · 2023-12-26T22:14:24Z

pyiceberg/catalog/__init__.py

+        if new_version < 0:
+            raise ValueError(f"Table metadata version: {new_version} cannot be negative")


Should we also return a more friendly error in the off chance that new_version is None?

Suggested change

if new_version < 0:

raise ValueError(f"Table metadata version: {new_version} cannot be negative")

if new_version is None or new_version < 0:

raise ValueError(f"Table metadata version: `{new_version}` must be a non-negative integer")

Thanks for the suggestion! Since this is an internal method, we can rely on the type check to ensure new_version is never None. I think one benefit of doing type-checking is that it reduces the number of None checks in our code. Also, I've incorporated your suggested change to the error message.

Please let me know if you think the type-check is not enough and we'd better do the None-check here. Thanks!

pdames · 2023-12-26T22:16:43Z

pyiceberg/catalog/__init__.py

+        Returns:
+            int: The version of the metadata file. -1 if the file name does not have valid version string
+        """
+        file_name = metadata_location.split("/")[-1]


Should we also gracefully return -1 if metadata_location happens to be undefined?

Suggested change

file_name = metadata_location.split("/")[-1]

file_name = metadata_location.split("/")[-1] if metadata_location else ""

Similar reason as above, since this is an internal method, I think we can rely on the type-check to ensure metadata_location is not None. But please let me know if you think type-check is not enough.

pyiceberg/catalog/glue.py

pdames · 2023-12-26T23:02:51Z

pyiceberg/catalog/__init__.py

+        file_name = metadata_location.split("/")[-1]
+        if file_name_match := TABLE_METADATA_FILE_NAME_REGEX.fullmatch(file_name):
+            return int(file_name_match.group(1))
+        else:
+            return -1


If we take the above change to add a capturing group around the UUID, we can assert its validity here with something like:

Suggested change

file_name = metadata_location.split("/")[-1]

if file_name_match := TABLE_METADATA_FILE_NAME_REGEX.fullmatch(file_name):

return int(file_name_match.group(1))

else:

return -1

file_name = metadata_location.split("/")[-1] if metadata_location else ""

if file_name_match := TABLE_METADATA_FILE_NAME_REGEX.fullmatch(file_name):

try:

UUID(file_name_match.group(2))

except ValueError:

return -1

return int(file_name_match.group(1))

return -1

(Again, assuming this type of stricter assertion against table metadata file names is actually desirable - not sure if @Fokko or @jackye1995 have any opinions or additional context here).

HonahX

Thanks @pdames for reviewing! Apologies for the delay; I was on vacation last week.

HonahX · 2024-01-01T08:54:30Z

pyiceberg/catalog/__init__.py

+        if new_version < 0:
+            raise ValueError(f"Table metadata version: {new_version} cannot be negative")


Thanks for the suggestion! Since this is an internal method, we can rely on the type check to ensure new_version is never None. I think one benefit of doing type-checking is that it reduces the number of None checks in our code. Also, I've incorporated your suggested change to the error message.

Please let me know if you think the type-check is not enough and we'd better do the None-check here. Thanks!

HonahX · 2024-01-01T08:58:27Z

pyiceberg/catalog/__init__.py

+        Returns:
+            int: The version of the metadata file. -1 if the file name does not have valid version string
+        """
+        file_name = metadata_location.split("/")[-1]


Similar reason as above, since this is an internal method, I think we can rely on the type-check to ensure metadata_location is not None. But please let me know if you think type-check is not enough.

pyiceberg/catalog/glue.py

HonahX · 2024-01-01T10:04:25Z

pyiceberg/catalog/__init__.py

@@ -74,6 +75,8 @@
 LOCATION = "location"
 EXTERNAL_TABLE = "EXTERNAL_TABLE"

+TABLE_METADATA_FILE_NAME_REGEX = re.compile(r"""(\d+)-.*\.json""", re.X)


Thanks for the suggestion. I've updated the regex to capture the UUID. Although in java we do not check the UUID validity: https://github.com/apache/iceberg/blob/81bf8d30766b1b129b87abde15239645cb127046/core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java#L382-L404
I think it make sense to add the check here

HonahX · 2024-01-01T10:10:32Z

tests/conftest.py

@@ -1586,7 +1586,7 @@ def fixture_aws_credentials() -> Generator[None, None, None]:
    os.environ.pop("AWS_DEFAULT_REGION")


-MOTO_SERVER = ThreadedMotoServer(port=5000)
+MOTO_SERVER = ThreadedMotoServer(ip_address="localhost", port=5000)


The default ip address of the moto server is 0.0.0.0. Since we only use it for unit test, I think it make more sense to start it at localhost

Fokko · 2024-01-01T18:27:38Z

Thanks @HonahX for working on this, and thanks @pdames, @nicor88, and @jackye1995 for the review 🙌

* Implement table metadata updater first draft * fix updater error and add tests * implement _commit_table for glue * implement apply_metadata_update which is simpler * remove old implementation * re-organize method place * fix nit * fix test * add another test * clear TODO * add a combined test * Fix merge conflict * update table metadata merged * implement requirements validation * change the exception to CommitFailedException * add docstring * use regex to parse the metadata version * fix lint issue * fix CI issue * make base_metadata optional and add null check * make base_metadata optional and add null check * add integration test * default skip-archive to true and comments * refactor tests * add doc and fix test after merge * make regex more robust, thanks Fokko! * Fix review comments, thanks Patrick!

HonahX added 15 commits November 5, 2023 00:32

Implement table metadata updater first draft

d53785a

fix updater error and add tests

274b91b

implement _commit_table for glue

8e8d39d

implement apply_metadata_update which is simpler

c3e1311

remove old implementation

2b7a7d1

re-organize method place

4fc25df

fix nit

facb43b

Merge branch 'table_metadata_update' into table_metadata_update_and_g…

ce4311f

…lue_commit

fix test

116c6fd

add another test

66a4f46

clear TODO

2882d0d

add a combined test

8a8d4ff

Merge remote-tracking branch 'origin/main' into table_metadata_update

70b64d8

# Conflicts: # pyiceberg/table/__init__.py # tests/conftest.py

Fix merge conflict

1cfe9d2

Merge branch 'table_metadata_update' into table_metadata_update_and_g…

252dc36

…lue_commit

HonahX mentioned this pull request Nov 12, 2023

Update table metadata #139

Merged

HonahX changed the title ~~[WIP] Table metadata update, table requirement validation, and glue catalog commit~~ WIP： Table metadata update, table requirement validation, and glue catalog commit Nov 12, 2023

HonahX changed the title ~~WIP： Table metadata update, table requirement validation, and glue catalog commit~~ WIP: Table metadata update, table requirement validation, and glue catalog commit Nov 12, 2023

HonahX added 5 commits December 7, 2023 21:32

Merge branch 'main' into table_metadata_update_and_glue_commit

501e7a9

# Conflicts: # pyiceberg/catalog/glue.py # pyiceberg/table/__init__.py # pyiceberg/table/metadata.py # tests/conftest.py # tests/table/test_init.py

update table metadata merged

7afe318

implement requirements validation

f769101

change the exception to CommitFailedException

4282d37

add docstring

94cfc69

HonahX mentioned this pull request Dec 10, 2023

Table Requirements Validation #200

Merged

HonahX added 6 commits December 10, 2023 15:37

Merge branch 'table_update_requirements' into table_metadata_update_a…

bb58e09

…nd_glue_commit # Conflicts: # pyiceberg/table/__init__.py

use regex to parse the metadata version

6d4efc8

fix lint issue

5efb155

fix CI issue

413935e

Merge branch 'table_update_requirements' into table_metadata_update_a…

fe7da26

…nd_glue_commit

make base_metadata optional and add null check

e8666dc

nicor88 reviewed Dec 12, 2023

View reviewed changes

pyiceberg/catalog/glue.py Show resolved Hide resolved

Fokko added this to the PyIceberg 0.6.0 release milestone Dec 13, 2023

add integration test

ccb787c

jackye1995 reviewed Dec 14, 2023

View reviewed changes

pyiceberg/catalog/glue.py Show resolved Hide resolved

HonahX mentioned this pull request Dec 15, 2023

GlueCatalog: Set Glue table input information based on Iceberg table metadata #216

Closed

HonahX added 3 commits December 15, 2023 16:24

Merge branch 'main' into table_metadata_update_and_glue_commit

8a29796

default skip-archive to true and comments

5e78af9

refactor tests

93f2cec

HonahX changed the title ~~WIP: Glue catalog commit~~ Glue catalog commit table Dec 16, 2023

HonahX marked this pull request as ready for review December 16, 2023 02:05

jackye1995 reviewed Dec 19, 2023

View reviewed changes

pyiceberg/catalog/__init__.py Show resolved Hide resolved

jackye1995 approved these changes Dec 19, 2023

View reviewed changes

HonahX added 2 commits December 20, 2023 01:56

Merge branch 'main' into table_metadata_update_and_glue_commit

6932cee

add doc and fix test after merge

0d38337

Fokko reviewed Dec 21, 2023

View reviewed changes

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

Fokko reviewed Dec 21, 2023

View reviewed changes

HonahX added 2 commits December 21, 2023 17:56

Merge branch 'main' into table_metadata_update_and_glue_commit

0ad4909

make regex more robust, thanks Fokko!

dccad75

pdames reviewed Dec 26, 2023

View reviewed changes

HonahX added 2 commits January 1, 2024 00:08

Merge branch 'main' into table_metadata_update_and_glue_commit

b967284

Fix review comments, thanks Patrick!

bf06d26

HonahX commented Jan 1, 2024

View reviewed changes

Fokko approved these changes Jan 1, 2024

View reviewed changes

Fokko merged commit 2272e20 into apache:main Jan 1, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Glue catalog commit table #140

Glue catalog commit table #140

HonahX commented Nov 12, 2023 •

edited

HonahX commented Dec 14, 2023 •

edited

HonahX commented Dec 16, 2023 •

edited

jackye1995 left a comment

Fokko left a comment

pdames left a comment

pdames Dec 26, 2023

HonahX Jan 1, 2024

pdames Dec 26, 2023 •

edited

HonahX Jan 1, 2024

pdames Dec 26, 2023

HonahX Jan 1, 2024

pdames Dec 26, 2023

HonahX left a comment

HonahX Jan 1, 2024

HonahX Jan 1, 2024

HonahX Jan 1, 2024

HonahX Jan 1, 2024

Fokko commented Jan 1, 2024

	TABLE_METADATA_FILE_NAME_REGEX = re.compile(r"""(\d+)-.*\.json""", re.X)
	TABLE_METADATA_FILE_NAME_REGEX = re.compile(r"""(\d+)-(.*)\.metadata\.json""", re.X)

-TABLE_METADATA_FILE_NAME_REGEX = re.compile(r"""(\d+)-.*\.json""", re.X)
+TABLE_METADATA_FILE_NAME_REGEX = re.compile(
+    r"""
+    (\d+)             # version number
+    -                 # separator
+    (.*)              # UUID
+    \.metadata\.json  # file extension
+    """,
+    re.X
+)

		if new_version < 0:
		raise ValueError(f"Table metadata version: {new_version} cannot be negative")

	file_name = metadata_location.split("/")[-1]
	file_name = metadata_location.split("/")[-1] if metadata_location else ""

Glue catalog commit table #140

Glue catalog commit table #140

Conversation

HonahX commented Nov 12, 2023 • edited

HonahX commented Dec 14, 2023 • edited

HonahX commented Dec 16, 2023 • edited

jackye1995 left a comment

Choose a reason for hiding this comment

Fokko left a comment

Choose a reason for hiding this comment

pdames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdames Dec 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HonahX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fokko commented Jan 1, 2024

HonahX commented Nov 12, 2023 •

edited

HonahX commented Dec 14, 2023 •

edited

HonahX commented Dec 16, 2023 •

edited

pdames Dec 26, 2023 •

edited