PARQUET-2042: Add support for unwrapping common Protobuf wrappers and… #900

mwong38 · 2021-05-03T07:53:59Z

… logical Timestamps, Date, TimeOfDay

Make sure you have checked all steps below.

Jira

My PR addresses the following Parquet Jira issues and references them in the PR title. For example, "PARQUET-1234: My Parquet PR"
- https://issues.apache.org/jira/browse/PARQUET-XXX
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does

shangxinli · 2021-11-12T17:22:56Z

@mwong38, can you put more information in the Jira on why/what is changed? This is pretty big change and it would help people to review your code.

sheinbergon · 2022-03-05T12:45:51Z

@shangxinli @mwong38 any way I can help with merge? Currently, proto-to-parquet conversions do not support proper timestamp handling, and it looks like this work addresses the issue.

Let me know.

mwong38 · 2022-03-06T08:31:59Z

After proto3 made everything optional, there is no way to know whether a primitive has been set or not. That is, you could no longer represent a "nullable" primitive. (They later brought optional keyword back, but the damage is done). The solution was to wrap a primitive in a message. For example, to represent a nullable double, there is the DoubleValue. It's analogous to Java's boxed Double.

Parquet supports nullable primitives natively. If we were to represent these well-known Protobuf types directly, it will be nested inside a deeper data structure; very inconvenient and a waste of space. What I did here is "unwrap" these primitives and make use of Parquet's nullaibility.

Additionally, I convert other well-known types such as Timestamp and Date to Parquet's. Again, rather than represent the data structure in its raw nested form (seconds/nanos or year, month, day, etc), they are converted to Parquet's native or logical representation of Timestamp and Date. You can turn on or off this features in the ProtoSchemaConverter constructor.

sheinbergon · 2022-03-06T14:05:52Z

@shangxinli is there any reason this work shouldn't be merged, given @mwong38 explanation? Is there something I can help with?

shangxinli · 2022-03-07T16:31:33Z

I don't see a reason why this cannot be merge. I will have a look soon.

shangxinli · 2022-03-19T20:14:49Z

Can you squash all the commits?

shangxinli · 2022-03-20T18:03:45Z

parquet-protobuf/src/main/java/org/apache/parquet/proto/ProtoMessageConverter.java

+    @Override
+    public void addInt(int value) {
+      LocalDate localDate = LocalDate.ofEpochDay(value);
+      com.google.type.Date date = com.google.type.Date.newBuilder()


Can we use import?

I think in this particular case it's more clear to be explicit that this is the Google Protobuf Date, rather than, say, the Java built-in Date.

If there is no collision on the imports, we should use imports generally.

parquet-protobuf/src/main/java/org/apache/parquet/proto/ProtoMessageConverter.java

sheinbergon · 2022-03-21T10:04:48Z

@mwong38 let me know if you want me to help in any way

dossett · 2022-03-22T13:13:41Z

I am not an expert here by any means, but I have made (very small) changes related to default values in protobuf parquet before and the goal of this change seem good. I assume that the behavior for unwrapped primitives remains unchanged?

@belugabehr might have thoughts too.

dossett · 2022-03-22T13:13:41Z

I am not an expert here by any means, but I have made (very small) changes related to default values in protobuf parquet before and the goal of this change seem good. I assume that the behavior for unwrapped primitives remains unchanged?

@belugabehr might have thoughts too.

sheinbergon · 2022-03-25T13:25:11Z

@shangxinli any news about merging this version? Are there still any blockers?

shangxinli · 2022-03-25T19:28:00Z

Will have another look soon.

parquet-protobuf/pom.xml

emkornfield · 2022-03-28T04:12:22Z

parquet-protobuf/src/main/java/org/apache/parquet/proto/ProtoSchemaConverter.java

+      if (unwrapProtoWrappers) {
+        String typeName = descriptor.getMessageType().getFullName();
+        if (typeName.equals(PROTOBUF_TIMESTAMP_TYPE)) {
+          return builder.primitive(INT64, getRepetition(descriptor)).as(timestampType(true, TimeUnit.NANOS));


should TimeUnit be something configurable? I know protobuf timestamps support nanosecond resolution but might not be what applications want to store.

I don't think it's worth complicating the API. The Timestamp common Proto stores time in nanoseconds. There's no good reason to deviate from that or to truncate the resolution. If the user wishes to do more manipulation, it can be done downstream.

In that case, you can use the default 'TimeUnit.NANOS' while having that configured.

Isn't that outside the scope of ProtoSchemaConverter/ParquetWriter? I don't think it should go into the business of doing transformations. If the point of ProtoSchemaConverter is to give the closest representation of the Protobuf object in Parquet, then it should be NANOS and nothing more.

@emkornfield Do you still have a comment on this?

parquet-protobuf/src/main/java/org/apache/parquet/proto/ProtoSchemaConverter.java

emkornfield · 2022-03-28T04:27:06Z

parquet-protobuf/src/main/java/org/apache/parquet/proto/ProtoSchemaConverter.java

@@ -97,6 +127,46 @@ public MessageType convert(Class<? extends Message> protobufClass) {

  private <T> Builder<? extends Builder<?, GroupBuilder<T>>, GroupBuilder<T>> addField(FieldDescriptor descriptor, final GroupBuilder<T> builder) {
    if (descriptor.getJavaType() == JavaType.MESSAGE) {
+      if (unwrapProtoWrappers) {
+        String typeName = descriptor.getMessageType().getFullName();


is there a reason you are using type equality vs descriptor equality? (I haven't checked but I would think .Equals would work? Maybe add a comment if this is intentional.

That's a good point. I'll change it to compare the Descriptor directly to the Wrapper's Descriptor rather than the name.

shangxinli · 2022-04-20T15:07:39Z

@mwong38 Can you address the feedback from @emkornfield before we can merge?

sheinbergon · 2022-06-13T07:19:04Z

@mwong38 anyway I can help with finalizing this PR?

shangxinli · 2022-07-24T20:11:37Z

I think we are close to merge this PR. Resolve the conflict and use the imports , then we can merge.

mwong38 · 2022-09-04T11:40:57Z

I think we are close to merge this PR. Resolve the conflict and use the imports , then we can merge.

Done

sheinbergon · 2022-10-17T22:04:21Z

@shangxinli any reason why this PR hasn't been merged yet?

zhaolihe · 2023-10-17T10:37:55Z

any reason why this PR hasn't been merged yet?

wgtmac · 2023-10-18T09:28:08Z

I just noticed this PR and sorry to see it does not check in. @mwong38 Could you try rebase it one last time? Thanks!

… logical Timestamps, Date, TimeOfDay

mwong38 · 2023-11-26T23:57:07Z

I just noticed this PR and sorry to see it does not check in. @mwong38 Could you try rebase it one last time? Thanks!

Done. I really hope it's the last time.

wgtmac

Thanks @mwong38!

@shangxinli @emkornfield I see you have reviewed this PR and there is no more comment to resolve. Do you need to take a look for another pass?

shangxinli · 2023-11-27T01:39:17Z

Merged

mwong38 force-pushed the PARQUET-2042 branch from efbe1d7 to dbaa368 Compare March 20, 2022 11:28

shangxinli reviewed Mar 20, 2022

View reviewed changes

parquet-protobuf/src/main/java/org/apache/parquet/proto/ProtoMessageConverter.java Outdated Show resolved Hide resolved

mwong38 force-pushed the PARQUET-2042 branch from dbaa368 to e6b6e95 Compare March 21, 2022 10:21

emkornfield reviewed Mar 28, 2022

View reviewed changes

parquet-protobuf/pom.xml Show resolved Hide resolved

emkornfield reviewed Mar 28, 2022

View reviewed changes

parquet-protobuf/src/main/java/org/apache/parquet/proto/ProtoSchemaConverter.java Show resolved Hide resolved

emkornfield reviewed Mar 28, 2022

View reviewed changes

mwong38 force-pushed the PARQUET-2042 branch from e6b6e95 to 9bf1961 Compare July 3, 2022 23:27

mwong38 force-pushed the PARQUET-2042 branch from 9bf1961 to 9672ee7 Compare September 4, 2022 11:40

PARQUET-2042: Add support for unwrapping common Protobuf wrappers and…

95b45d3

… logical Timestamps, Date, TimeOfDay

mwong38 force-pushed the PARQUET-2042 branch from 9672ee7 to 95b45d3 Compare November 26, 2023 23:54

wgtmac approved these changes Nov 27, 2023

View reviewed changes

shangxinli approved these changes Nov 27, 2023

View reviewed changes

shangxinli merged commit c8487c7 into apache:master Nov 27, 2023
9 checks passed

0x26res mentioned this pull request Jan 6, 2025

Fix reading of proto Uint32Value #3113

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PARQUET-2042: Add support for unwrapping common Protobuf wrappers and… #900

PARQUET-2042: Add support for unwrapping common Protobuf wrappers and… #900

mwong38 commented May 3, 2021

shangxinli commented Nov 12, 2021

sheinbergon commented Mar 5, 2022

mwong38 commented Mar 6, 2022

sheinbergon commented Mar 6, 2022 •

edited

Loading

shangxinli commented Mar 7, 2022

shangxinli commented Mar 19, 2022

shangxinli Mar 20, 2022

mwong38 Mar 21, 2022

shangxinli Jul 24, 2022

sheinbergon commented Mar 21, 2022

dossett commented Mar 22, 2022

dossett commented Mar 22, 2022

sheinbergon commented Mar 25, 2022

shangxinli commented Mar 25, 2022

emkornfield Mar 28, 2022

mwong38 Jul 3, 2022

shangxinli Jul 4, 2022

mwong38 Jul 4, 2022

shangxinli Jul 24, 2022

emkornfield Mar 28, 2022

mwong38 Jul 3, 2022

shangxinli commented Apr 20, 2022

sheinbergon commented Jun 13, 2022

shangxinli commented Jul 24, 2022

mwong38 commented Sep 4, 2022

sheinbergon commented Oct 17, 2022

zhaolihe commented Oct 17, 2023

wgtmac commented Oct 18, 2023

mwong38 commented Nov 26, 2023

wgtmac left a comment

shangxinli commented Nov 27, 2023

PARQUET-2042: Add support for unwrapping common Protobuf wrappers and… #900

PARQUET-2042: Add support for unwrapping common Protobuf wrappers and… #900

Conversation

mwong38 commented May 3, 2021

Jira

Tests

Commits

Documentation

shangxinli commented Nov 12, 2021

sheinbergon commented Mar 5, 2022

mwong38 commented Mar 6, 2022

sheinbergon commented Mar 6, 2022 • edited Loading

shangxinli commented Mar 7, 2022

shangxinli commented Mar 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sheinbergon commented Mar 21, 2022

dossett commented Mar 22, 2022

dossett commented Mar 22, 2022

sheinbergon commented Mar 25, 2022

shangxinli commented Mar 25, 2022

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

shangxinli commented Apr 20, 2022

sheinbergon commented Jun 13, 2022

shangxinli commented Jul 24, 2022

mwong38 commented Sep 4, 2022

sheinbergon commented Oct 17, 2022

zhaolihe commented Oct 17, 2023

wgtmac commented Oct 18, 2023

mwong38 commented Nov 26, 2023

wgtmac left a comment

Choose a reason for hiding this comment

shangxinli commented Nov 27, 2023

sheinbergon commented Mar 6, 2022 •

edited

Loading