PARQUET-1798: [C++] Review logic around automatic assignment of field_id's #10289

westonpace · 2021-05-10T17:54:56Z

Questions:

This is my first PR in the parquet namespace, I'm not sure of all the special rules.
The field ID generation doesn't happen on the parquet::schema -> arrow::schema phase but on the parquet::format::schema -> parquet::schema phase. So in order to test I had to add #include "generated/parquet_types.h" to arrow_schema_test.cc and I wasn't sure if I was allowed to reference the generated/* files like that.
This PR simply allows user specified field id's to be persisted. Is that sufficient for PARQUET-1798 (the title is rather general) or should I open up a dedicated JIRA?

github-actions · 2021-05-10T17:55:16Z

https://issues.apache.org/jira/browse/PARQUET-1798

westonpace · 2021-05-10T17:56:03Z

CC @emkornfield Are you able to review this? Not sure who I should ping for a parquet change.

emkornfield · 2021-05-10T18:20:55Z

I'll try to look tonight or tomorrow morning. Otherwise, Antoine is likely the best person.

wesm · 2021-05-10T18:31:14Z

cc @TGooch44

pitrou · 2021-05-12T09:42:36Z

I'm not even sure what field_ids are supposed to be for. The parquet spec only has this to say:

  /** When the original schema supports field ids, this will save the
   * original field id in the parquet schema
   */
  9: optional i32 field_id;

I suppose "original schema" means something non-Parquet, but what? Is it just some kind of arbitrary application-defined id?

cpp/src/parquet/schema.cc

pitrou · 2021-05-12T09:47:20Z

Based on my understanding, it seems that we should:

when reading from Parquet, reflect Parquet field_ids (if any) under the PARQUET:field_id metadata key
when writing to Parquet, generate Parquet field_ids from the PARQUET:field_id metadata key (if present)
not attempt to auto-generate any field_ids if they are not present in metadata

emkornfield · 2021-05-12T15:27:34Z

https://issues.apache.org/jira/browse/PARQUET-951 informs field IDs a little bit better. It is from other systems, in this case protobuf (and I imagine thrift might also have something similar) has each field in a message annotated with a unique ID. Based on this I agree with Antoine's assessment, haven't actually looked at the code (is this not what is done?).

westonpace · 2021-05-12T18:33:40Z

not attempt to auto-generate any field_ids if they are not present in metadata
@pitrou

That should simplify things. Just to clarify, this will be a bit of a regression as we currently auto-generate field IDs today.

https://issues.apache.org/jira/browse/PARQUET-951 informs field IDs a little bit better. It is from other systems, in this case protobuf (and I imagine thrift might also have something similar) has each field in a message annotated with a unique ID. Based on this I agree with Antoine's assessment, haven't actually looked at the code (is this not what is done?).
@emkornfield

Correct. We already pulled the field id out of thrift and into Arrow metadata. The only problem was that the logic to do the reverse was missing. This PR is only adding that.

There could be some follow-up work for integrating with other parts of the Arrow ecosystem. I will send some questions to the ML.

westonpace · 2021-05-13T21:32:49Z

Per @pitrou 's suggestion I have removed the logic auto-generating field_id entirely. I also added a python test to ensure things are working full path.

This is ready for review again.

westonpace · 2021-05-18T04:40:04Z

Ok, looks like I jumped the gun with the last comment. Removing the old auto-generation behavior broke some tests I wasn't looking at. They are fixed now. I believe the CI failures are unrelated at this point. I may force-push tomorrow just for good measure.

Review is welcome.

westonpace · 2021-05-24T17:34:29Z

I've rebased (and verified again that the build failures are unrelated). Gentle ping for review @emkornfield / @pitrou

pitrou

Sorry for the delay, some comments and questions below.

pitrou · 2021-05-26T09:35:42Z

python/pyarrow/tests/parquet/test_metadata.py

-    #   }
-    #   optional binary field_id=5 f2;
-    # }
-
    field_name = b'PARQUET:field_id'


You already have it named field_id above.

Thanks, fixed to use the existing variable.

pitrou · 2021-05-26T09:38:26Z

cpp/src/parquet/schema_test.cc

@@ -171,17 +167,16 @@ TEST_F(TestPrimitiveNode, Attrs) {
 }

 TEST_F(TestPrimitiveNode, FromParquet) {
-  SchemaElement elt =
-      NewPrimitive(name_, FieldRepetitionType::OPTIONAL, Type::INT32, field_id_);
+  SchemaElement elt = NewPrimitive(name_, FieldRepetitionType::OPTIONAL, Type::INT32);


I'm not sure I understand this change in the tests. The user should still be able to pass an explicit field_id when creating a schema node, no?

No, the user does not need this capability. See comment below.

Well, let's say I want to create a Node with a given field id as was done in this test. Would you I do that?

Ah, I see your point. These tests were not testing FromParquet so the field id setting was still valid. I have restored them.

cpp/src/parquet/schema.h

pitrou · 2021-05-26T09:44:07Z

cpp/src/parquet/arrow/arrow_schema_test.cc

+TEST_F(TestConvertRoundTrip, GroupIdMissingIfNotSpecified) {
+  std::vector<std::shared_ptr<Field>> arrow_fields;
+  arrow_fields.push_back(::arrow::field("simple", ::arrow::int32(), false));
+  /// { "nested": { "outer": { "inner" }, "sibling" }


Missing closing brace here?

pitrou · 2021-05-26T09:45:49Z

cpp/src/parquet/arrow/arrow_schema_test.cc

+  return field_ids;
+}
+
+TEST_F(TestConvertRoundTrip, GroupIdMissingIfNotSpecified) {


Why "GroupId"? Should this be "FieldId"?

Yes, I'm not sure where Group came from. Fixed.

pitrou · 2021-05-26T09:47:06Z

cpp/src/parquet/arrow/arrow_schema_test.cc

+  return ::arrow::key_value_metadata({"PARQUET:field_id"}, {std::to_string(field_id)});
+}
+
+TEST_F(TestConvertRoundTrip, GroupIdPreserveExisting) {


Well... this test only checks that Arrow metadata is preserved, right? It doesn't test that the metadata is actually converted into a field_id on the Parquet schema node.

Perhaps TestConvertRoundTrip::ConvertSchema should be renamed to RoundTripSchema. It does the following transformations...

vector<Field> -> arrow::Schema

arrow::Schema -> parquet::SchemaDescriptor

parquet::SchemaDescriptor -> vector<parquet::format::SchemaElement>

vector<parquet::format::SchemaElement> -> parquet::SchemaDescriptor

parquet::SchemaDescriptor -> arrow::Schema

So I believe it does indeed test the Parquet schema node.

It doesn't seem to test that the PARQUET:field_id Arrow annotation ends up in the Parquet field_id member, does it?

I now verify the field IDs at all three levels (round-tripped arrow, parquet, and thrift).

… to parquet file. On read these existing field ids will be preferred to the auto-generating (dfs-assignment) algorithm.

… in my test but thrift_internal is what I was looking for

…behavior

…metadata that I had missed. Now that the old behavior changed it was invalid. So I removed my new test and updated the existing test.

westonpace · 2021-05-27T13:40:35Z

As an extra level of sanity-checking I created a parquet file with Arrow and then read it in with fastparquet and verified the field_id is correct (both for a missing field_id and a valid field_id).

westonpace · 2021-05-27T13:41:35Z

Provided this passes CI (I'll check in the morning) I believe I have addressed all concerns.

pitrou · 2021-05-27T13:41:55Z

Thanks a lot for checking!

pitrou · 2021-05-27T14:57:54Z

AppVeyor build at https://ci.appveyor.com/project/pitrou/arrow/builds/39351310

…_id's Questions: - This is my first PR in the parquet namespace, I'm not sure of all the special rules. - The field ID generation doesn't happen on the `parquet::schema` -> `arrow::schema` phase but on the `parquet::format::schema` -> `parquet::schema` phase. So in order to test I had to add `#include "generated/parquet_types.h"` to `arrow_schema_test.cc` and I wasn't sure if I was allowed to reference the `generated/*` files like that. - This PR simply allows user specified field id's to be persisted. Is that sufficient for PARQUET-1798 (the title is rather general) or should I open up a dedicated JIRA? Closes apache#10289 from westonpace/feature/PARQUET-1798-field-id-assignment Lead-authored-by: Weston Pace <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

github-actions bot added Component: C++ Component: Parquet labels May 10, 2021

westonpace force-pushed the feature/PARQUET-1798-field-id-assignment branch from 49c3060 to bd4a8fb Compare May 10, 2021 19:57

pitrou reviewed May 12, 2021

View reviewed changes

cpp/src/parquet/schema.cc Outdated Show resolved Hide resolved

westonpace force-pushed the feature/PARQUET-1798-field-id-assignment branch from bd4a8fb to 6b372d0 Compare May 13, 2021 21:27

github-actions bot added the Component: Python label May 13, 2021

westonpace requested a review from pitrou May 13, 2021 21:32

westonpace force-pushed the feature/PARQUET-1798-field-id-assignment branch 2 times, most recently from 6e80fe9 to 9349f6e Compare May 17, 2021 18:18

emkornfield mentioned this pull request May 18, 2021

ARROW-12424: [Go][Parquet] Adding Schema Package for Go Parquet #10071

Closed

westonpace force-pushed the feature/PARQUET-1798-field-id-assignment branch from c11452d to 85a860b Compare May 24, 2021 16:20

pitrou reviewed May 26, 2021

View reviewed changes

westonpace added 7 commits May 27, 2021 02:30

PARQUET-1798: Allow field IDs present in Arrow metadata to be written…

ecb6e4f

… to parquet file. On read these existing field ids will be preferred to the auto-generating (dfs-assignment) algorithm.

PARQUET-1798: Indeed it seems I cannot reference the generated folder…

1f1ee94

… in my test but thrift_internal is what I was looking for

PARQUET-1798: Removed autogeneration of field ids. Added python test

9a0cdc3

PARQUET-1798: Missed a test check in my cleanup

01c094b

PARQUET-1798: Python lint

400eb6a

PARQUET-1798: Remove unused field_id_ from tests

ae4fc85

PARQUET-1798: Fixed test that was relying on old field id generation …

4950f30

…behavior

westonpace added 2 commits May 27, 2021 02:30

PARQUET-1798: Turns out there was a python test for field id in test_…

edd422d

…metadata that I had missed. Now that the old behavior changed it was invalid. So I removed my new test and updated the existing test.

PARQUET-1798: Addressing PR comments

7a93325

westonpace force-pushed the feature/PARQUET-1798-field-id-assignment branch from 85a860b to 7a93325 Compare May 27, 2021 13:28

Remove outdated comment

af22ece

pitrou approved these changes May 27, 2021

View reviewed changes

pitrou closed this in d0de88d May 27, 2021

westonpace deleted the feature/PARQUET-1798-field-id-assignment branch January 6, 2022 08:17

levaja mentioned this pull request May 19, 2022

parquet::schema::Node's field_id not being auto-generated #13194

Open

jorisvandenbossche mentioned this pull request Feb 3, 2023

GH-33954: [C++][Parquet] Preserve field-id for nested type #33955

Merged

asfimport mentioned this pull request Jun 23, 2024

[C++][Parquet] Review logic around automatic assignment of field_id's #42935

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PARQUET-1798: [C++] Review logic around automatic assignment of field_id's #10289

PARQUET-1798: [C++] Review logic around automatic assignment of field_id's #10289

westonpace commented May 10, 2021

github-actions bot commented May 10, 2021

westonpace commented May 10, 2021

emkornfield commented May 10, 2021

wesm commented May 10, 2021

pitrou commented May 12, 2021

pitrou commented May 12, 2021

emkornfield commented May 12, 2021 •

edited

Loading

westonpace commented May 12, 2021

westonpace commented May 13, 2021

westonpace commented May 18, 2021

westonpace commented May 24, 2021

pitrou left a comment

pitrou May 26, 2021

westonpace May 27, 2021

pitrou May 26, 2021

westonpace May 26, 2021

pitrou May 27, 2021

westonpace May 27, 2021

pitrou May 26, 2021

westonpace May 27, 2021

pitrou May 26, 2021

westonpace May 27, 2021

pitrou May 26, 2021

westonpace May 26, 2021 •

edited

Loading

pitrou May 27, 2021

westonpace May 27, 2021

westonpace commented May 27, 2021

westonpace commented May 27, 2021

pitrou commented May 27, 2021

pitrou commented May 27, 2021

PARQUET-1798: [C++] Review logic around automatic assignment of field_id's #10289

PARQUET-1798: [C++] Review logic around automatic assignment of field_id's #10289

Conversation

westonpace commented May 10, 2021

github-actions bot commented May 10, 2021

westonpace commented May 10, 2021

emkornfield commented May 10, 2021

wesm commented May 10, 2021

pitrou commented May 12, 2021

pitrou commented May 12, 2021

emkornfield commented May 12, 2021 • edited Loading

westonpace commented May 12, 2021

westonpace commented May 13, 2021

westonpace commented May 18, 2021

westonpace commented May 24, 2021

pitrou 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

westonpace May 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonpace commented May 27, 2021

westonpace commented May 27, 2021

pitrou commented May 27, 2021

pitrou commented May 27, 2021

emkornfield commented May 12, 2021 •

edited

Loading

westonpace May 26, 2021 •

edited

Loading