fix: add metadata_properties to _construct_parameters when update hive table #2013

kadai0308 · 2025-05-18T13:49:33Z

Closes: #2010

Rationale for this change

This change adds metadata_properties to the _construct_parameters function to ensure metadata properties are included in the parameters.
I'm not entirely confident about the changes, so please let me know if my understanding is correct—if so, I’ll proceed to add tests.
Thanks you!

Are these changes tested?

Not yet.

Are there any user-facing changes?

Not sure.

Fokko

@kadai0308 thanks for working on this. Would it be possible to add a test? We have Hive container running that we use for tests. This way we don't break it in the future.

Maybe we can add a test somewhere here:

iceberg-python/tests/integration/test_reads.py

Line 87 in da88b8d

def test_table_properties(catalog: Catalog) -> None:

The session_catalog_hive is a HiveCatalog.

pyiceberg/catalog/hive.py

kadai0308 · 2025-05-20T14:28:16Z

@kadai0308 thanks for working on this. Would it be possible to add a test? We have Hive container running that we use for tests. This way we don't break it in the future.

Maybe we can add a test somewhere here:

iceberg-python/tests/integration/test_reads.py

Line 87 in da88b8d

def test_table_properties(catalog: Catalog) -> None:

The session_catalog_hive is a HiveCatalog.

sure

pyiceberg/catalog/hive.py

…e table

kadai0308 · 2025-06-02T12:52:52Z

@Fokko Can you help me review the PR? Thank you.

kevinjqliu

Generally LGTM! I added a few comments. Thanks for working on this!

kevinjqliu · 2025-06-03T04:42:09Z

pyiceberg/catalog/hive.py

    properties = {PROP_EXTERNAL: "TRUE", PROP_TABLE_TYPE: "ICEBERG", PROP_METADATA_LOCATION: metadata_location}
    if previous_metadata_location:
        properties[PROP_PREVIOUS_METADATA_LOCATION] = previous_metadata_location

+    if metadata_properties:
+        for key, value in metadata_properties.items():
+            if key not in properties:


👍 this is fine, it helps with not re-setting PROP_EXTERNAL, PROP_TABLE_TYPE, PROP_METADATA_LOCATION, and PROP_PREVIOUS_METADATA_LOCATION

kevinjqliu · 2025-06-03T04:59:26Z

tests/integration/test_reads.py

@@ -111,6 +112,23 @@ def test_table_properties(catalog: Catalog) -> None:
        table.transaction().set_properties(property_name=None).commit_transaction()
    assert "None type is not a supported value in properties: property_name" in str(exc_info.value)

+    if isinstance(catalog, HiveCatalog):


this is a great test! could you move this into its own test function?

with just hive catalog

@pytest.mark.integration @pytest.mark.parametrize("catalog", [pytest.lazy_fixture("session_catalog_hive")])

kevinjqliu · 2025-06-03T05:04:38Z

tests/integration/test_reads.py

@@ -111,6 +112,23 @@ def test_table_properties(catalog: Catalog) -> None:
        table.transaction().set_properties(property_name=None).commit_transaction()


I was wondering why the rest of these tests pass since we're not setting the properties in the HMS. Turns out the table properties are saved in the table metadata using its properties field.

This is not what the table metadata's properties field should be used for,

properties A string to string map of table properties. This is used to control settings that affect reading and writing and is not intended to be used for arbitrary metadata. For example, commit.retry.num-retries is used to control the number of commit retries.

This is a side affect of

iceberg-python/pyiceberg/table/__init__.py

Lines 1131 to 1134 in a67c559

@property

def properties(self) -> Dict[str, str]:

"""Properties of the table."""

return self.metadata.properties

and

iceberg-python/pyiceberg/catalog/hive.py

Lines 338 to 344 in a67c559

return Table(

identifier=(table.dbName, table.tableName),

metadata=metadata,

metadata_location=metadata_location,

io=self._load_file_io(metadata.properties, metadata_location),

catalog=self,

)

We should fix this behavior and read/write properties using the HMS's table parameters. We can fix this separately from the current issue.

I opened #2064 to track this

This PR will save the properties in both the HMS's table parameter and table metadata's properties field

Thanks for your review and suggestion.

Yes, this is also confuse me when I develop this PR. Then I found:
https://github.com/kadai0308/iceberg-python/blob/ff149e8e9d8e0b8dd9e74158b1fb89724833b5b4/pyiceberg/catalog/hive.py#L342-L344

So to make sure it did write to the HMS properties, I need to new a hive_client to get the hive_table.parameters:

hive_client: _HiveClient = _HiveClient(catalog.properties["uri"]) with hive_client as open_client: hive_table = open_client.get_table(*TABLE_NAME) assert hive_table.parameters.get("abc") == "def" assert hive_table.parameters.get("p1") == "123"

instead of just test like:

table = create_table(catalog) assert table.properties == dict(p1="123", **DEFAULT_PROPERTIES)

I think I can also help with #2064.

Fokko

This looks good to me @kadai0308, thanks for adding the test 👍

Fokko · 2025-06-08T18:43:58Z

tests/integration/test_reads.py

@@ -112,6 +113,27 @@ def test_table_properties(catalog: Catalog) -> None:
    assert "None type is not a supported value in properties: property_name" in str(exc_info.value)


+@pytest.mark.integration
+@pytest.mark.parametrize("catalog", [pytest.lazy_fixture("session_catalog_hive")])


Nit: we don't need to use parameterize with a single argument

Fokko · 2025-06-08T18:45:32Z

@kadai0308, there is an issue with the code formatting, can you run make lint? Thanks!

Fokko · 2025-06-13T12:00:32Z

Gentle ping @kadai0308

Fokko · 2025-06-13T13:41:16Z

@kadai0308 It looks like some relevant tests are failing:

tests/catalog/test_hive.py::test_create_table[True] FAILED               [ 46%]
tests/catalog/test_hive.py::test_create_table[False] FAILED              [ 46%]
tests/catalog/test_hive.py::test_create_table_with_given_location_removes_trailing_slash[True] FAILED [ 46%]
tests/catalog/test_hive.py::test_create_table_with_given_location_removes_trailing_slash[False] FAILED [ 46%]

kadai0308 · 2025-06-13T13:42:10Z

@kadai0308 It looks like some relevant tests are failing:

tests/catalog/test_hive.py::test_create_table[True] FAILED               [ 46%]
tests/catalog/test_hive.py::test_create_table[False] FAILED              [ 46%]
tests/catalog/test_hive.py::test_create_table_with_given_location_removes_trailing_slash[True] FAILED [ 46%]
tests/catalog/test_hive.py::test_create_table_with_given_location_removes_trailing_slash[False] FAILED [ 46%]

Yeah, I am looking on it.

Fokko · 2025-06-13T13:47:52Z

@kadai0308 Thanks, appreciate it. I think apart from that this PR looks good to go in 👍

…ties

kevinjqliu

LGTM

kevinjqliu · 2025-06-13T15:53:25Z

@kadai0308 looks like linter failed, can you run make lint locally?

ruff-format..............................................................Failed
- hook id: ruff-format
- files were modified by this hook

1 file reformatted, 153 files left unchanged

kadai0308 · 2025-06-13T16:46:29Z

@kadai0308 looks like linter failed, can you run make lint locally?

ruff-format..............................................................Failed
- hook id: ruff-format
- files were modified by this hook

1 file reformatted, 153 files left unchanged

Sorry for forgetting to run lint before pushing the commit.

kevinjqliu · 2025-06-14T18:09:21Z

Thanks for the fix @kadai0308 and thanks @Fokko @geruh for the review :)

# Rationale for this change See #2013 Closes #2064 Continuing the trend, but with glue. # Are these changes tested? See test below # Are there any user-facing changes? When a user specifies property update on commit table, those parameters will be passed to the glue client.

…e table (apache#2013)   Closes: apache#2010 # Rationale for this change This change adds metadata_properties to the _construct_parameters function to ensure metadata properties are included in the parameters. I'm not entirely confident about the changes, so please let me know if my understanding is correct—if so, I’ll proceed to add tests. Thanks you! # Are these changes tested? Not yet. # Are there any user-facing changes? Not sure.

# Rationale for this change See apache#2013 Closes apache#2064 Continuing the trend, but with glue. # Are these changes tested? See test below # Are there any user-facing changes? When a user specifies property update on commit table, those parameters will be passed to the glue client.

kadai0308 force-pushed the fix/hive-client-does-not-update-table-properties branch from fa27421 to df294d1 Compare May 18, 2025 13:50

Fokko reviewed May 19, 2025

View reviewed changes

pyiceberg/catalog/hive.py Show resolved Hide resolved

geruh reviewed May 23, 2025

View reviewed changes

pyiceberg/catalog/hive.py Show resolved Hide resolved

fix: add metadata_properties to _construct_parameters when update hiv…

ff149e8

…e table

kadai0308 force-pushed the fix/hive-client-does-not-update-table-properties branch from df294d1 to ff149e8 Compare May 25, 2025 13:17

kevinjqliu reviewed Jun 3, 2025

View reviewed changes

kevinjqliu mentioned this pull request Jun 3, 2025

iceberg table properties are saved in table metadata's properties field #2064

Closed

3 tasks

fix: separate the test_hive_properties test

230d088

Fokko approved these changes Jun 8, 2025

View reviewed changes

fix lint

7e754d5

kadai0308 added 2 commits June 13, 2025 22:52

fix: test_hive missing default parameters

d5a26e0

Merge branch 'main' into fix/hive-client-does-not-update-table-proper…

985d77d

…ties

kadai0308 force-pushed the fix/hive-client-does-not-update-table-properties branch from 5273b95 to 985d77d Compare June 13, 2025 14:54

kevinjqliu approved these changes Jun 13, 2025

View reviewed changes

fix lint

16f562e

kevinjqliu merged commit 4f0d7ef into apache:main Jun 14, 2025
10 checks passed

kadai0308 deleted the fix/hive-client-does-not-update-table-properties branch June 15, 2025 04:50

summermousa-vendia mentioned this pull request Jun 16, 2025

fix: add metadata_properties to _construct_parameters when updating glue table #2097

Merged

		@@ -111,6 +112,23 @@ def test_table_properties(catalog: Catalog) -> None:
		table.transaction().set_properties(property_name=None).commit_transaction()

	@property
	def properties(self) -> Dict[str, str]:
	"""Properties of the table."""
	return self.metadata.properties

	return Table(
	identifier=(table.dbName, table.tableName),
	metadata=metadata,
	metadata_location=metadata_location,
	io=self._load_file_io(metadata.properties, metadata_location),
	catalog=self,
	)

fix: add metadata_properties to _construct_parameters when update hive table #2013

fix: add metadata_properties to _construct_parameters when update hive table #2013

Uh oh!

Conversation

kadai0308 commented May 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kadai0308 commented May 20, 2025

Uh oh!

Uh oh!

kadai0308 commented Jun 2, 2025

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Jun 3, 2025 • 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!

kadai0308 Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

Fokko Jun 8, 2025

Choose a reason for hiding this comment

Uh oh!

Fokko commented Jun 8, 2025

Uh oh!

Fokko commented Jun 13, 2025

Uh oh!

Fokko commented Jun 13, 2025

Uh oh!

kadai0308 commented Jun 13, 2025

Uh oh!

Fokko commented Jun 13, 2025

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

kevinjqliu commented Jun 13, 2025

Uh oh!

kadai0308 commented Jun 13, 2025

Uh oh!

Uh oh!

kevinjqliu commented Jun 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kadai0308 commented May 18, 2025 •

edited

Loading

kevinjqliu Jun 3, 2025 •

edited

Loading

kevinjqliu commented Jun 14, 2025 •

edited

Loading