URL-encode partition field names in file locations by smaheshwar-pltr · Pull Request #1457 · apache/iceberg-python

smaheshwar-pltr · 2024-12-20T21:57:57Z

Closes #1458
Closes #175

tests/integration/test_partitioning_key.py

kevinjqliu · 2024-12-22T20:55:39Z

BTW #1462 is merged, could you rebase this PR?

smaheshwar-pltr · 2024-12-23T13:55:36Z

tests/table/test_partitioning.py

+        spec_id=3,
+    )
+
+    record = Record(**{"my#str%bucket": "my+str", "other str+bucket": "( )", "my!int:bucket": 10})  # type: ignore


mypy complains here and elsewhere but I think it's fine

smaheshwar-pltr · 2024-12-23T13:56:50Z

tests/table/test_partitioning.py

+
+    # Both partition names fields and values should be URL encoded, with spaces mapping to plus signs, to match the Java
+    # behaviour: https://github.com/apache/iceberg/blob/ca3db931b0f024f0412084751ac85dd4ef2da7e7/api/src/main/java/org/apache/iceberg/PartitionSpec.java#L198-L204
+    assert spec.partition_to_path(record, schema) == "my%23str%25bucket=my%2Bstr/other+str%2Bbucket=%28+%29/my%21int%3Abucket=10"


Cross-checked with Java implementation (integration tests will do this eventually), in particular WRT to ' ' and '+' encoding. It is consistent.

smaheshwar-pltr · 2024-12-23T13:59:06Z

Done, @kevinjqliu. Fails due to #1457 (comment) but will think over it.

FYI, am away for a little bit now so will pick this back up shortly in the new year! (Feel free to take over if v urgent 😄)

kevinjqliu · 2024-12-23T19:01:49Z

Thanks for the PR! I've dug into the test failure a bit. Heres what I found.

There's a subtle difference between PartitionKey.partition and DataFile.partition. In most cases, these are the same value. For strings with special characters, DataFile.partition is sanitized but PartitionKey.partition is not.

DataFile.partition is sanitized according to apache/iceberg/#10120 this is to match the column value stored in the underlying parquet file.
PartitionKey.partition uses the value from the PartitionSpec which stores the un-sanitized value.

You can verify this by looking up the table partition spec.

iceberg_table.metadata.spec()
iceberg_table.metadata.specs()

The integration test assumes that the value for PartitionKey.partition and DataFile.partition is the same.
One possible solution is to sanitize the given Record before comparison

After spark_path_for_justification,

        # Special characters in partition value are sanitized when written to the data file's partition field
        # Use `make_compatible_name` to match the sanitize behavior
        sanitized_record = Record(**{make_compatible_name(k): v for k, v in vars(expected_partition_record).items()})
        assert spark_partition_for_justification == sanitized_record

smaheshwar-pltr · 2024-12-23T23:30:57Z

Thanks a lot for this explanation and suggestion @kevinjqliu! It sounds good.

Had some time so I've made this change so tests pass - using make_compatible_name as a param that can be specified on a per-case basis (instead of porting over all this logic into tests). I also made it Optional so each subtest doesn't have to specify the identity transform but None.

Fokko · 2024-12-29T05:55:39Z

pyiceberg/partitioning.py

            value_str = partition_field.transform.to_human_string(field_types[pos].field_type, value=data[pos])

-            value_str = quote(value_str, safe="")
+            value_str = quote_plus(value_str, safe="")


It defaults to utf-8, so that's good 👍

Fokko · 2024-12-29T05:57:04Z

pyiceberg/partitioning.py

+            field_str = quote_plus(partition_field.name, safe="")
+            field_strs.append(field_str)


Nit, I would just collapse these:

Suggested change

field_str = quote_plus(partition_field.name, safe="")

field_strs.append(field_str)

field_strs.append(quote_plus(partition_field.name, safe=""))

Fokko

Thanks @smaheshwar-pltr for picking this up, and thanks for @kevinjqliu the review. I'll leave this one open in case Kevin has any further comments.

tests/integration/test_partitioning_key.py

kevinjqliu

LGTM! Thanks for working on this @smaheshwar-pltr
A few nit comments left on this PR but not blocking. Going to merge this PR as is and we can deal with nit comment as a followup

smaheshwar-pltr · 2025-01-08T17:20:28Z

Sounds good, thanks Kevin for your excellent review here!

smaheshwar-pltr · 2025-01-08T18:01:46Z

Going to merge this PR as is and we can deal with nit comment as a followup

I've put up #1499 for the nits.

This was referenced Dec 20, 2024

(Potential Bug) Partition field names are not URL-encoded in file locations #1458

Closed

Support Location Providers #1452

Merged

smaheshwar-pltr marked this pull request as ready for review December 20, 2024 23:36

smaheshwar-pltr commented Dec 21, 2024

View reviewed changes

tests/integration/test_partitioning_key.py Show resolved Hide resolved

smaheshwar-pltr requested a review from kevinjqliu December 21, 2024 19:41

Sreesh Maheshwar added 8 commits December 22, 2024 22:30

URL-encode partition field names in file locations

4b139ee

Separate into variable

638a43f

Add test

3126952

Revert to main

65e2c39

Failing test

64e7748

Disable justication from test

18c7674

Use quote_plus instead of quote to match Java behaviour

3756e4e

Temporarily update test to pass

f5a35de

smaheshwar-pltr force-pushed the encode-partition-names branch from 999d753 to f5a35de Compare December 23, 2024 03:31

Sreesh Maheshwar added 2 commits December 22, 2024 22:32

Uncomment test

f1f5f4c

Add unit test

8a106e6

smaheshwar-pltr commented Dec 23, 2024

View reviewed changes

Fix typo in comment

1bb379b

Add make_name_compatible suggestion so test passes

61cdd08

Fix typo in schema field name

f32b3aa

Fokko reviewed Dec 29, 2024

View reviewed changes

Fokko approved these changes Dec 29, 2024

View reviewed changes

kevinjqliu reviewed Jan 2, 2025

View reviewed changes

tests/integration/test_partitioning_key.py Show resolved Hide resolved

kevinjqliu approved these changes Jan 2, 2025

View reviewed changes

kevinjqliu merged commit 5da1f4d into apache:main Jan 2, 2025
7 checks passed

smaheshwar-pltr deleted the encode-partition-names branch January 8, 2025 17:18

smaheshwar-pltr mentioned this pull request Jan 8, 2025

Nit fixes to URL-encoding of partition field names #1499

Merged

		field_str = quote_plus(partition_field.name, safe="")
		field_strs.append(field_str)

	field_str = quote_plus(partition_field.name, safe="")
	field_strs.append(field_str)
	field_strs.append(quote_plus(partition_field.name, safe=""))

Conversation

smaheshwar-pltr commented Dec 20, 2024 • edited by Fokko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kevinjqliu commented Dec 22, 2024

Uh oh!

smaheshwar-pltr Dec 23, 2024 • 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!

smaheshwar-pltr Dec 23, 2024 • 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!

smaheshwar-pltr commented Dec 23, 2024

Uh oh!

kevinjqliu commented Dec 23, 2024

Uh oh!

smaheshwar-pltr commented Dec 23, 2024

Uh oh!

Fokko Dec 29, 2024

Choose a reason for hiding this comment

Uh oh!

Fokko Dec 29, 2024 • 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 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smaheshwar-pltr commented Jan 8, 2025

Uh oh!

smaheshwar-pltr commented Jan 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

smaheshwar-pltr commented Dec 20, 2024 •

edited by Fokko

Loading

smaheshwar-pltr Dec 23, 2024 •

edited

Loading

smaheshwar-pltr Dec 23, 2024 •

edited

Loading

Fokko Dec 29, 2024 •

edited

Loading