feat(parquet): add schema projection to parquet by wgtmac · Pull Request #159 · apache/iceberg-cpp

wgtmac · 2025-08-04T15:35:27Z

No description provided.

lidavidm

Hmm, not sure what that lint failure is talking about

wgtmac · 2025-08-05T01:46:02Z

Hmm, not sure what that lint failure is talking about

Me neither :/

lidavidm · 2025-08-05T01:48:16Z

I'm guessing one of the GTest or GMock macros expands to something weird.

google/googletest#4531

src/iceberg/parquet/parquet_schema_util.cc

test/parquet_schema_test.cc

src/iceberg/parquet/parquet_schema_util.cc

test/parquet_schema_test.cc

src/iceberg/parquet/parquet_schema_util.cc

zhjwpku

LGTM, thanks for working on this.

wgtmac · 2025-08-07T07:01:22Z

@Fokko @zeroshade Could you help review this? Thanks!

Fokko · 2025-08-08T13:16:45Z

src/iceberg/parquet/parquet_schema_util.cc

+      }
+      break;
+    case TypeId::kTime:
+      if (arrow_type->id() == ::arrow::Type::TIME64) {


Should we also check for ::arrow::TimeUnit::MICRO here?

Good catch! I have added an exhaustive test case to make sure I don't miss any primitive type.

Fokko · 2025-08-08T13:17:54Z

src/iceberg/parquet/parquet_schema_util.cc

+      }
+      break;
+    case TypeId::kDecimal:
+      if (arrow_type->id() == ::arrow::Type::DECIMAL128) {


Do we also accept the other types?

This is still an open PR: apache/arrow#45351

dongxiao1198

lgtm

zeroshade · 2025-08-11T19:24:41Z

src/iceberg/parquet/parquet_schema_util.cc

+      if (arrow_type->id() == ::arrow::Type::FIXED_SIZE_BINARY) {
+        const auto& fixed_binary =
+            internal::checked_cast<const ::arrow::FixedSizeBinaryType&>(*arrow_type);
+        if (fixed_binary.byte_width() == 16) {
+          return {};
+        }
+      }


We should probably also allow https://github.com/apache/arrow/blob/main/cpp/src/arrow/extension/uuid.h#L35

You can validate via arrow_type->id() == ::arrow::Type::EXTENSION and the extension_name() == "arrow.uuid"

Good catch! Fixed.

zeroshade · 2025-08-11T19:25:59Z

src/iceberg/parquet/parquet_schema_util.cc

+    case TypeId::kString:
+      if (arrow_type->id() == ::arrow::Type::STRING) {
+        return {};
+      }
+      break;
+    case TypeId::kBinary:
+      if (arrow_type->id() == ::arrow::Type::BINARY) {
+        return {};
+      }
+      break;


What about LargeString, LargeBinary, StringView and BinaryView?

I don't think parquet-cpp has supported these types.

zeroshade · 2025-08-11T19:28:48Z

src/iceberg/parquet/parquet_schema_util.cc

+// TODO(gangwu): support v3 unknown type
+Status ValidateParquetSchemaEvolution(
+    const Type& expected_type, const ::parquet::arrow::SchemaField& parquet_field) {
+  const auto& arrow_type = parquet_field.field->type();


I forget offhand if this will return a DictionaryType for dictionary encoded columns, if so then you need to check for the DictionaryType and then switch on the ValueType of it.

No it won't. Reading dictionary is supported via an option to create a RecordReader: https://github.com/apache/arrow/blob/2dd3ccda6437f79aa34641bd3197dd7392ae4aec/cpp/src/parquet/column_reader.h#L266

zeroshade · 2025-08-11T19:29:11Z

src/iceberg/parquet/parquet_schema_util.cc

+      }
+      break;
+    case TypeId::kList:
+      if (arrow_type->id() == ::arrow::Type::LIST) {


What about LargeList and ListView?

ListView is not supported by parquet-cpp yet. I think we should just support simple list and binary type variants in the early versions of iceberg-cpp. Once parquet-cpp has full support, we can leverage them later.

Xuanwo

Thank you for working on this, let's move!

wgtmac force-pushed the parquet_projection branch from 8968c66 to 80a629b Compare August 4, 2025 15:37

feat(parquet): add schema projection for parquet

316f42a

wgtmac force-pushed the parquet_projection branch from 80a629b to 316f42a Compare August 4, 2025 16:16

lidavidm approved these changes Aug 4, 2025

View reviewed changes

mapleFU reviewed Aug 5, 2025

View reviewed changes

src/iceberg/parquet/parquet_schema_util.cc Outdated Show resolved Hide resolved

src/iceberg/parquet/parquet_schema_util.cc Show resolved Hide resolved

wgtmac force-pushed the parquet_projection branch from b24dd93 to b287a6f Compare August 5, 2025 05:41

wgtmac commented Aug 5, 2025

View reviewed changes

test/parquet_schema_test.cc Outdated Show resolved Hide resolved

wgtmac force-pushed the parquet_projection branch 4 times, most recently from ceb4b1c to 6af2b8f Compare August 5, 2025 09:07

lidavidm reviewed Aug 5, 2025

View reviewed changes

test/parquet_schema_test.cc Outdated Show resolved Hide resolved

wgtmac force-pushed the parquet_projection branch 2 times, most recently from 1311ee6 to a94e6a0 Compare August 5, 2025 09:48

mapleFU reviewed Aug 5, 2025

View reviewed changes

wgtmac force-pushed the parquet_projection branch 5 times, most recently from 472d2ee to ad10678 Compare August 5, 2025 16:08

wgtmac commented Aug 5, 2025

View reviewed changes

test/parquet_schema_test.cc Show resolved Hide resolved

wgtmac force-pushed the parquet_projection branch 7 times, most recently from a4531f0 to ca51b68 Compare August 6, 2025 05:50

wgtmac force-pushed the parquet_projection branch 2 times, most recently from c2f5b56 to 2ea602a Compare August 6, 2025 06:42

zhjwpku reviewed Aug 6, 2025

View reviewed changes

src/iceberg/parquet/parquet_schema_util.cc Outdated Show resolved Hide resolved

src/iceberg/parquet/parquet_schema_util.cc Outdated Show resolved Hide resolved

bump clang-tidy version for cpp-linter

b42bda5

wgtmac force-pushed the parquet_projection branch from 2ea602a to b42bda5 Compare August 6, 2025 15:10

mapleFU approved these changes Aug 7, 2025

View reviewed changes

zhjwpku approved these changes Aug 7, 2025

View reviewed changes

Fokko reviewed Aug 8, 2025

View reviewed changes

fix time64 unit and add more test

36b01c5

wgtmac requested a review from Fokko August 9, 2025 06:34

dongxiao1198 approved these changes Aug 11, 2025

View reviewed changes

zeroshade reviewed Aug 11, 2025

View reviewed changes

wgtmac force-pushed the parquet_projection branch 2 times, most recently from 89410b3 to b686ccb Compare August 12, 2025 08:07

fix arrow uuid extension type

bc9d8be

wgtmac force-pushed the parquet_projection branch from b686ccb to bc9d8be Compare August 12, 2025 09:40

wgtmac changed the title ~~feat(parquet): add schema projection for parquet~~ feat(parquet): add schema projection to parquet Aug 13, 2025

Xuanwo approved these changes Aug 13, 2025

View reviewed changes

Xuanwo merged commit 7063f2b into apache:main Aug 13, 2025
7 checks passed

Conversation

wgtmac commented Aug 4, 2025

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Aug 5, 2025

Uh oh!

lidavidm commented Aug 5, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhjwpku left a comment

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Aug 7, 2025

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!

dongxiao1198 left a comment

Choose a reason for hiding this comment

Uh oh!

zeroshade Aug 11, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Xuanwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

zeroshade Aug 11, 2025 •

edited

Loading