GH-43807: [C++][Python] Add UUID extension type conversion support to/from Parquet #45866

paleolimbot · 2025-03-20T04:07:27Z

Rationale for this change

After #37298, the UUID canonical extension type is supported in Arrow C++ and PyArrow; however, it is not converted to the Parquet UUID type on write and is not inferred when on Parquet read.

What changes are included in this PR?

Infer the Parquet UUID type from the Arrow UUID extension type
Infer the Arrow UUID type from Parquet UUID when arrow_extensions_enabled is set (like the JSON extension type)
Wire up arrow_extensions_enabled to pyarrow and add Python tests

Are these changes tested?

Yes!

Are there any user-facing changes?

Yes! (Documentation will be added)

GitHub Issue: [C++][Python] (De)serialize Arrow UUID to/from Parquet UUID #43807

github-actions · 2025-03-20T04:07:54Z

⚠️ GitHub issue #43807 has been automatically assigned in GitHub to PR creator.

wgtmac

The C++ part looks good to me (I can't speak for the python part though). Thanks!

cpp/src/parquet/arrow/arrow_schema_test.cc

wgtmac · 2025-03-26T14:51:02Z

cpp/src/parquet/arrow/schema.cc

+      // Apply metadata recursively to storage type
+      RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));
+      inferred->field = inferred->field->WithType(origin_type);
+    } else if (inferred_type->id() == ::arrow::Type::FIXED_SIZE_BINARY &&


These branches are growing longer while they look pretty similar. Not sure if we can refactor it a little bit to look nicer.

I spent quite a bit of time rewriting the logic here...I think it's better than it was?

pitrou · 2025-03-27T15:15:40Z

cpp/src/parquet/arrow/arrow_schema_test.cc

+
+  {
+    // Parquet file contains Arrow schema.
+    // uuid will be interpreted as uuid() field even though extensions are not enabled.


They are below.

pitrou · 2025-03-27T15:17:50Z

cpp/src/parquet/arrow/schema_internal.cc

+      return ::arrow::fixed_size_binary(physical_length);
    case LogicalType::Type::UUID:
+      if (reader_properties.get_arrow_extensions_enabled()) {
+        return ::arrow::extension::uuid();


Do we need to check physical_length here?

pitrou · 2025-03-27T15:18:18Z

python/pyarrow/_dataset_parquet.pyx

    page_checksum_verification : bool, default False
        If True, verify the page checksum for each page read from the file.
+    arrow_extensions_enabled : bool, default False
+        If True, read Parquet logical types as Arrow Extension Types where possible,


Please see comments I posted on this on the Geometry PR.

pitrou · 2025-03-27T15:19:11Z

python/pyarrow/tests/parquet/test_data_types.py

+        pa.table({"ext": pa.array(data, pa.string())}),
+        store_schema=False)
+
+    # With arrow_extensions_enabled=True on read, we get a arrow.uuid back


This the JSON test, not UUID.

paleolimbot · 2025-04-10T18:01:09Z

Thanks for the reminder! I see the MacOS 13 tests failing on main as well, but I think this is ready for another review whenever time allows 🙂

wgtmac

Thanks for the update! I have left some comments to the refactoring part in schema.cc. Other parts look good to me!

cpp/src/parquet/arrow/arrow_schema_test.cc

cpp/src/parquet/arrow/schema_internal.cc

wgtmac · 2025-04-11T05:28:08Z

cpp/src/parquet/arrow/schema.cc

+    } else if (origin_extension_name == "arrow.uuid") {
+      extension_supports_inferred_storage =
+          arrow_extension_inferred ||
+          (inferred_type->id() == ::arrow::Type::FIXED_SIZE_BINARY &&


Is it better to add UuidType::IsSupportedStorageType(const std::shared_ptr<::arrow::DataType>&) for this?

wgtmac · 2025-04-11T05:33:16Z

cpp/src/parquet/arrow/schema.cc

+      // i.e., arrow_extensions_enabled is true or arrow_extensions_enabled is false but
+      // we still restore the extension type because Arrow is the source of truth if we
+      // are asked to apply the original metadata
+      auto origin_storage_field =


auto origin_storage_field = origin_field.WithType(origin_extension_type.storage_type()); RETURN_NOT_OK(ApplyOriginalStorageMetadata(*origin_storage_field, inferred));

These lines are the same in the branches. Should we move them out?

cpp/src/parquet/arrow/schema.cc

wgtmac · 2025-04-11T05:43:37Z

cpp/src/parquet/arrow/schema.cc

+          arrow_extension_inferred ||
+          VariantExtensionType::IsSupportedStorageType(inferred_type);
+    } else if (arrow_extension_inferred) {
+      extension_supports_inferred_storage = origin_extension_type.Equals(*inferred_type);


Why do we need this branch? It will for sure fall into the if statement at line 1100.

wgtmac · 2025-04-11T05:45:51Z

cpp/src/parquet/arrow/schema.cc

+    } else if (arrow_extension_inferred) {
+      extension_supports_inferred_storage = origin_extension_type.Equals(*inferred_type);
+    } else {
+      extension_supports_inferred_storage =


Similar to the above comment, do we need to check this? It will go to the else branch at line 1108 and the same check is performed there too.

Co-authored-by: Gang Wu <[email protected]>

… both if branches

paleolimbot · 2025-04-18T19:59:28Z

Apologies for taking a week to circle back here...I think the main thing I had been missing was that ApplyOriginalStorageMetadata(*origin_storage_field, inferred) invalidated auto& inferred_type, so I had been getting crashes when I tried to make the logic simpler. I think the result is much cleaner! I tried to overload the comments with the things I learned in the process.

wgtmac

It looks great to me! Thanks!

paleolimbot · 2025-04-21T04:32:45Z

Thanks! I'll merge tomorrow if there are no objections!

conbench-apache-arrow · 2025-04-22T04:21:49Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 75acf37.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 9 possible false positives for unstable benchmarks that are known to sometimes produce them.

pitrou · 2025-04-22T07:10:06Z

I forgot, do we have an issue open to enable extension types by default?

wgtmac · 2025-04-22T14:00:12Z

I didn't find one through a quick search. Should I create one? You mean ArrowReaderProperties::arrow_extensions_enabled_, right? @pitrou

pitrou · 2025-04-22T15:44:08Z

I didn't find one through a quick search. Should I create one? You mean ArrowReaderProperties::arrow_extensions_enabled_, right? @pitrou

Yes, and its Python counterpart. I think we discussed enabling it by default before @rok ?

rok · 2025-04-22T15:48:18Z

We did #44070 (comment), here's the corresponding issue: #44500. I'm happy to open a PR later this week.

github-actions bot added Component: Parquet Component: C++ awaiting committer review Awaiting committer review labels Mar 20, 2025

paleolimbot mentioned this pull request Mar 21, 2025

GH-45522: [Parquet][C++] Parquet GEOMETRY and GEOGRAPHY logical type implementations #45459

Merged

paleolimbot added 5 commits March 24, 2025 14:27

add uuid as opt in

a365b61

convert in other direction

4cee020

one more convert change

3d1a4ec

enable arrow_extensions_enabled in pyarrow

dc696cf

Add Python test

300f5eb

paleolimbot force-pushed the parquet-uuid branch from 149bbe8 to 300f5eb Compare March 24, 2025 15:29

github-actions bot added the Component: Python label Mar 24, 2025

paleolimbot added 5 commits March 24, 2025 10:46

Add C++ test

af8227c

format

bd9c9a9

fix comment that was lying

f187586

clean up schema.cc

d11fcd8

fix python lint

f7a965e

paleolimbot marked this pull request as ready for review March 25, 2025 14:47

paleolimbot requested a review from wgtmac as a code owner March 25, 2025 14:47

wgtmac approved these changes Mar 26, 2025

View reviewed changes

pitrou reviewed Mar 27, 2025

View reviewed changes

wgtmac mentioned this pull request Mar 31, 2025

[C++][Dataset] Parquet schema lost on dataset write #45969

Open

paleolimbot added 7 commits March 31, 2025 11:54

apply improvement to arrow_extensions_enabled

b4d0730

Check physical length before inferring Arrow UUID type

2193fd4

fix test comments in arrow_schema_test.cc

a81d5c1

Attempt to reduce duplication in extension type restoration

62f92b3

remove unused header

d1b37e8

support nested extension components

4bd607b

remove unneeded branch

7529947

github-actions bot removed the awaiting committer review Awaiting committer review label Apr 1, 2025

fix merge

d2d98f2

wgtmac reviewed Apr 11, 2025

View reviewed changes

Update cpp/src/parquet/arrow/arrow_schema_test.cc

f3894b8

Co-authored-by: Gang Wu <[email protected]>

paleolimbot requested review from AlenkaF, raulcd and rok as code owners April 18, 2025 15:47

paleolimbot and others added 8 commits April 18, 2025 10:47

Update cpp/src/parquet/arrow/arrow_schema_test.cc

a101c8b

Co-authored-by: Gang Wu <[email protected]>

Update cpp/src/parquet/arrow/schema_internal.cc

4c66a75

Co-authored-by: Gang Wu <[email protected]>

Update cpp/src/parquet/arrow/schema.cc

234475e

Co-authored-by: Gang Wu <[email protected]>

Update cpp/src/parquet/arrow/arrow_schema_test.cc

8e8d5ee

Co-authored-by: Gang Wu <[email protected]>

Update cpp/src/parquet/arrow/arrow_schema_test.cc

26afc40

Co-authored-by: Gang Wu <[email protected]>

clang-format

4b63ec7

move supported storage type test to UuidType, move common code out of…

b48f7f5

… both if branches

maybe simplify it all again

555ed75

AlenkaF removed their request for review April 18, 2025 17:45

wgtmac approved these changes Apr 19, 2025

View reviewed changes

paleolimbot merged commit 75acf37 into apache:main Apr 21, 2025
37 checks passed

paleolimbot mentioned this pull request Apr 21, 2025

[C++][Python] (De)serialize Arrow UUID to/from Parquet UUID #43807

Closed

paleolimbot mentioned this pull request Apr 22, 2025

[Python][Parquet] Parquet logical types should be mapped to corresponding extension types at read time #44500

Closed

wgtmac mentioned this pull request May 16, 2025

Writing UUID using PyArrow does not set the UUID logical type on Parquet #46469

Closed

Fokko mentioned this pull request May 16, 2025

fix: correct UUIDType partition representation for BucketTransform apache/iceberg-python#2003

Closed

pitrou mentioned this pull request May 27, 2025

[C++][Doc][Parquet] Update doc for JSON, UUID #46599

Closed

GH-43807: [C++][Python] Add UUID extension type conversion support to/from Parquet #45866

GH-43807: [C++][Python] Add UUID extension type conversion support to/from Parquet #45866

Uh oh!

Conversation

paleolimbot commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Mar 20, 2025

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paleolimbot commented Apr 10, 2025

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paleolimbot commented Apr 18, 2025

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

paleolimbot commented Apr 21, 2025

Uh oh!

Uh oh!

conbench-apache-arrow bot commented Apr 22, 2025

Uh oh!

pitrou commented Apr 22, 2025

Uh oh!

wgtmac commented Apr 22, 2025

Uh oh!

pitrou commented Apr 22, 2025

Uh oh!

rok commented Apr 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

paleolimbot commented Mar 20, 2025 •

edited

Loading