GH-46371: [C++][Parquet] Parquet Variant decoding tools #46372

mapleFU · 2025-05-09T10:37:43Z

Rationale for this change

This patch supports tool to decode the parquet variant.

What changes are included in this PR?

This patch supports tool to decode the parquet variant.

Are these changes tested?

Yes. I uses parquet-testings. Some problems is listed here: apache/parquet-testing#79

I can also add some hand-written tests after interface is agreed.

Are there any user-facing changes?

Yes, this adds interfaces for decode variant.

GitHub Issue: [C++][Parquet] Variant Type common decoding visitor #46371

github-actions · 2025-05-09T10:38:08Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

github-actions · 2025-05-09T11:34:24Z

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

…pp-decoder-tools

cpp/src/parquet/variant_test.cpp

cpp/src/parquet/variant.h

mapleFU · 2025-05-13T11:50:05Z

cpp/src/parquet/variant.h

+  /// \defgroup ValueAccessors
+  /// @{
+
+  // Note: Null doesn't need visitor.


I don't know should we just return an arrow's Scalar, it would be easy to use but in-efficient.

cpp/src/parquet/variant.h

cpp/src/parquet/variant.cc

mapleFU · 2025-05-14T08:06:51Z

cpp/src/parquet/variant.h

+  int8_t getInt8() const;
+  int16_t getInt16() const;
+  int32_t getInt32() const;
+  int64_t getInt64() const;


Currently, getInt64 only supports read from int64, which is too strict for integer. I think we can also uses some way to allow getInt64 to get some "smaller types" like int32, int16, int8.

mapleFU · 2025-05-14T08:07:05Z

cpp/src/parquet/variant.h

+  int32_t getInt32() const;
+  int64_t getInt64() const;
+  /// Include short_string optimization and primitive string type
+  std::string_view getString() const;


Currently I didn't check utf-8 here.

mapleFU · 2025-05-14T08:07:50Z

cpp/src/parquet/variant.h

+  std::string_view getString() const;
+  std::string_view getBinary() const;
+  float getFloat() const;
+  double getDouble() const;


Currently, getDouble only supports read from getFloat, which is too strict for. Maybe we can also uses some way to allow getDouble get other types

mapleFU · 2025-05-14T08:08:56Z

cpp/src/parquet/variant.cc

+  }
+
+  // checking the element is incremental.
+  // TODO(mwish): Remove this or encapsulate this range check to function


I think should we use a extra function here like "Validate", or just checks them here?

cpp/src/parquet/variant.cc

mapleFU · 2025-05-14T08:20:07Z

@emkornfield @wgtmac @pitrou @zeroshade

This patch add some basic variant decoding tools. Some thoughts:

How would the interface for visiting variant like? The simplist way is cast <metadata, value> pairs to a ptr<::arrow::Scalar>, but this is too slow and needs to read whole data. We can also wraps a std::variant, but I think it's also slow and needs to dynamic dispatch the visitor. Here I just add visitor for every type. And currently, getInt64 would only supports read int64. Any idea is welcome

xxubai

Thanks for the contribution, I have some questions please help me out!

xxubai · 2025-05-14T09:40:10Z

cpp/src/parquet/variant.cc

+  if (metadata.size() < 2) {
+    throw ParquetException("Invalid Variant metadata: too short: " +
+                           std::to_string(metadata.size()));
+  }


I think there are some conditions that should be checked first when creating metadata?

byte order

whether the header version is 1

About (1) I assume it's lsb format here. (2) is a good point

I've check it's 1 now

cpp/src/parquet/variant.cc

cpp/src/parquet/variant_test.cc

cpp/src/parquet/variant.h

mapleFU · 2025-05-14T11:52:24Z

cpp/src/parquet/variant.cc

+  if ((dict_size + 1) * offset_size > metadata_.size()) {
+    throw ParquetException("Invalid Variant metadata: offset out of range");
+  }
+  // TODO(mwish): This can be optimized by using binary search if the metadata is sorted.


This can be optimized by sorting, I'll optimize this later

mapleFU · 2025-05-14T11:53:02Z

cpp/src/parquet/variant.cc

+
+  uint32_t field_offset = std::numeric_limits<uint32_t>::max();
+  // Get the field offset
+  // TODO(mwish): Using binary search to optimize it.


This can be optimized by binary search

xxubai · 2025-05-15T03:41:57Z

Is it possible to manually add unit tests covering scenarios such as null values, UUID, etc.?

mapleFU · 2025-05-15T05:03:07Z

Is it possible to manually add unit tests covering scenarios such as null values, UUID, etc.?

Sure, I just leave it for review, I think the interfaces might be modified a lot after review, and parquet-java(which can write the correspond data) haven't release yet

wgtmac

I just took a glimpse of the API. Let me know what you think.

wgtmac · 2025-05-16T03:45:47Z

cpp/src/parquet/variant.h

+
+enum class VariantBasicType {
+  /// One of the primitive types
+  Primitive = 0,


Suggested change

Primitive = 0,

kPrimitive = 0,

We need to add k-prefix to be consistent in this codebase.

Actually this codebase doesn't have this style? Like:

enum class ParquetDataPageVersion { V1, V2 }; /// Controls the level of size statistics that are written to the file. enum class SizeStatisticsLevel : uint8_t { // No size statistics are written. None = 0, // Only column chunk size statistics are written. ColumnChunk, // Both size statistics in the column chunk and page index are written. PageAndColumnChunk };

wgtmac · 2025-05-16T03:45:55Z

cpp/src/parquet/variant.h

+
+enum class VariantPrimitiveType : int8_t {
+  /// Equivalent Parquet Type: UNKNOWN
+  NullType = 0,


cpp/src/parquet/variant.h

wgtmac · 2025-05-16T06:18:08Z

cpp/src/parquet/variant.h

+  /// @{
+
+  // Note: Null doesn't need visitor.
+  bool getBool() const;


What is the expectation if the basic type or primitive type does not match?

Is it acceptable to use std::variant to replace these getters? The caveat is that we need to define view types for object, array as well.

I don't think variant is a good idea, VariantValue is a "variant" type, casting it to std::variant and get from it is (1) not lazy, needs to deserialize whole structure (2) it decode twice

Fair enough. std::variant pays for a full deserialization.

wgtmac · 2025-05-16T06:19:33Z

cpp/src/parquet/variant.h

+  int64_t getTimestamp() const;
+  int64_t getTimestampNTZ() const;
+  // 16 bytes UUID
+  std::array<uint8_t, 16> getUuid() const;


Why not std::string_view? Is it due to big endianness?

wgtmac · 2025-05-16T06:30:08Z

cpp/src/parquet/variant.h

+    std::string toDebugString() const;
+  };
+  ObjectInfo getObjectInfo() const;
+  std::optional<VariantValue> getObjectValueByKey(std::string_view key) const;


Do we need functions to iterate each key/value?

wgtmac · 2025-05-16T06:30:29Z

cpp/src/parquet/variant.h

+  };
+  ArrayInfo getArrayInfo() const;
+  // Would throw ParquetException if index is out of range.
+  VariantValue getArrayValueByIndex(uint32_t index) const;


Same question for iterator over the elements

wgtmac · 2025-05-16T06:36:36Z

cpp/src/parquet/variant.h

+
+    std::string toDebugString() const;
+  };
+  ObjectInfo getObjectInfo() const;


Is it for reuse? Shouldn't we cache it internally?

mapleFU · 2025-05-16T16:12:06Z

@wgtmac I'm think of keep some elements inside the VariantValue, the way below adds 24B or 28B overheads on VariantValue. It adds some cost when constructing the VariantValue but maybe more friendly for user.

    VariantBasicType type_; // (maybe we don't need store this)
    uint32_t num_elements_; // exists when type_ is array or object
    uint32_t id_size_;   // exists when type_ is object
    uint32_t offset_size_; // exists when type_ is array or object
    uint32_t id_start_offset_; // exists when type_ is object
    uint32_t offset_start_offset_; // exists when type_ is array or object
    uint32_t data_start_offset_;   // exists when type_ is array or object

Another problem is the error style for decoding tools. If we just leave VariantValue/VariantMetadata a tool in parquet, and uses it in arrow compute or extension type, we also needs cast the handling value to status...

mapleFU · 2025-05-16T16:19:24Z

Also cc @pitrou for interfaces because you're interface expert here

mapleFU · 2025-05-16T21:15:13Z

cpp/src/parquet/variant.h

+  std::string_view metadata_;
+  uint32_t dictionary_size_{0};


I wonder if it's worth to change this to below case, which could make this 24B -> 16B

const uint8_t* metadata_ptr_; const uint32_t metadata_size_; uint32_t dictionary_size_

Variant tools

5c43465

github-actions bot added Component: Parquet Component: C++ awaiting review Awaiting review labels May 9, 2025

mapleFU changed the title ~~[C++][Parquet] Parquet Variant decoding tools~~ GH-46371: [C++][Parquet] Parquet Variant decoding tools May 9, 2025

mapleFU added 4 commits May 12, 2025 16:12

Merge branch 'main' of https://github.com/apache/arrow into variant-c…

ec7d9e0

…pp-decoder-tools

metadata logic impl

a4599d8

add some value interfaces

34c1d2c

Merge branch 'main' into variant-cpp-decoder-tools

1083a16

mapleFU commented May 13, 2025

View reviewed changes

cpp/src/parquet/variant_test.cpp Outdated Show resolved Hide resolved

cpp/src/parquet/variant.h Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 13, 2025

mapleFU added 5 commits May 13, 2025 16:28

Add basic tests (will failed)

ad585f2

Fix all primitive type tests( time, uuid not included)

225b39b

skeleton for object parsing ( bug might exists )

80a37ba

eliminate duplicate code

fc4b721

Basic implement array (test not runned)

e8cdad7

mapleFU commented May 13, 2025

View reviewed changes

mapleFU added 2 commits May 14, 2025 14:44

Finish code of uuid, complete some tests

34f546e

Cleanup for review

3566dfd

mapleFU commented May 14, 2025

View reviewed changes

mapleFU marked this pull request as ready for review May 14, 2025 08:10

mapleFU requested a review from wgtmac as a code owner May 14, 2025 08:10

mapleFU force-pushed the variant-cpp-decoder-tools branch from fb59842 to da142a6 Compare May 14, 2025 08:10

continue cleanup

54681c4

mapleFU force-pushed the variant-cpp-decoder-tools branch from da142a6 to 54681c4 Compare May 14, 2025 08:20

Try to fix lint

7759b03

xxubai reviewed May 14, 2025

View reviewed changes

mapleFU added 2 commits May 14, 2025 18:41

Address some comments, fix and test NestedObject problem

31fc7df

Fix object handling, and refactor methods ( needs rethink it )

15b12f2

mapleFU commented May 14, 2025

View reviewed changes

Minor comments

bbce69c

mapleFU force-pushed the variant-cpp-decoder-tools branch from 51f02d9 to bbce69c Compare May 14, 2025 12:16

Merge branch 'main' into variant-cpp-decoder-tools

0c96946

mapleFU force-pushed the variant-cpp-decoder-tools branch 3 times, most recently from 99a59ee to c0526da Compare May 14, 2025 14:44

Trying to using scope resolution operator to fix ci

6454bb7

mapleFU force-pushed the variant-cpp-decoder-tools branch from c0526da to 6454bb7 Compare May 14, 2025 14:51

Try to fix CI

9560023

mapleFU added 4 commits May 15, 2025 16:14

Revert stupid file name change

365329f

refactor metadata and read u32le (bug exists)

cf9c4ad

VariantMetadata use u32 rather than i32; minor bug fix

9b52d0b

optimize the case when metadata key is unique

6c9c66d

wgtmac reviewed May 16, 2025

View reviewed changes

mapleFU added 3 commits May 17, 2025 03:23

Apply cr suggestions

5361500

Refactor with heavier value

7628110

More comments for the code interface

1b31d42

mapleFU force-pushed the variant-cpp-decoder-tools branch from 7bfb57d to 1b31d42 Compare May 16, 2025 20:11

change str to view

a83f8cc

mapleFU commented May 16, 2025

View reviewed changes

mapleFU added 2 commits May 17, 2025 11:52

Revert toString return view firstly

17a0637

Fix ci

01ec5ea

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-46371: [C++][Parquet] Parquet Variant decoding tools #46372

GH-46371: [C++][Parquet] Parquet Variant decoding tools #46372

mapleFU commented May 9, 2025 •

edited

Loading

github-actions bot commented May 9, 2025

github-actions bot commented May 9, 2025

mapleFU May 13, 2025

mapleFU May 14, 2025

mapleFU May 14, 2025

mapleFU May 14, 2025

mapleFU May 14, 2025

mapleFU commented May 14, 2025

xxubai left a comment

xxubai May 14, 2025

mapleFU May 14, 2025

mapleFU May 14, 2025

mapleFU May 14, 2025

mapleFU May 14, 2025

xxubai commented May 15, 2025

mapleFU commented May 15, 2025

wgtmac left a comment

wgtmac May 16, 2025

mapleFU May 16, 2025

wgtmac May 16, 2025

wgtmac May 16, 2025

wgtmac May 16, 2025

mapleFU May 16, 2025

wgtmac May 16, 2025

wgtmac May 16, 2025

mapleFU May 16, 2025

wgtmac May 16, 2025

wgtmac May 16, 2025

wgtmac May 16, 2025

mapleFU commented May 16, 2025 •

edited

Loading

mapleFU commented May 16, 2025

mapleFU May 16, 2025

GH-46371: [C++][Parquet] Parquet Variant decoding tools #46372

Are you sure you want to change the base?

GH-46371: [C++][Parquet] Parquet Variant decoding tools #46372

Conversation

mapleFU commented May 9, 2025 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented May 9, 2025

github-actions bot commented May 9, 2025

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

mapleFU commented May 14, 2025

xxubai 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

Choose a reason for hiding this comment

xxubai commented May 15, 2025

mapleFU commented May 15, 2025

wgtmac 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU commented May 16, 2025 • edited Loading

mapleFU commented May 16, 2025

Choose a reason for hiding this comment

mapleFU commented May 9, 2025 •

edited

Loading

mapleFU commented May 16, 2025 •

edited

Loading