PARQUET-2042: Add support for unwrapping common Protobuf wrappers and… by mwong38 · Pull Request #900 · apache/parquet-java

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

+    @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.

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.

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

+      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?

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

  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

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

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

Comment thread parquet-protobuf/pom.xml

emkornfield reviewed Mar 28, 2022

View reviewed changes

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

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

0x26res mentioned this pull request Jan 6, 2025

GH-3112: Fix reading of proto Uint32Value #3113

Merged

Conversation

mwong38 commented May 3, 2021

Jira

Tests

Commits

Documentation

Uh oh!

shangxinli commented Nov 12, 2021

Uh oh!

sheinbergon commented Mar 5, 2022

Uh oh!

mwong38 commented Mar 6, 2022

Uh oh!

sheinbergon commented Mar 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shangxinli commented Mar 7, 2022

Uh oh!

shangxinli commented Mar 19, 2022

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!

Uh oh!

sheinbergon commented Mar 21, 2022

Uh oh!

dossett commented Mar 22, 2022

Uh oh!

dossett commented Mar 22, 2022

Uh oh!

sheinbergon commented Mar 25, 2022

Uh oh!

shangxinli commented Mar 25, 2022

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shangxinli commented Apr 20, 2022

Uh oh!

sheinbergon commented Jun 13, 2022

Uh oh!

shangxinli commented Jul 24, 2022

Uh oh!

mwong38 commented Sep 4, 2022

Uh oh!

sheinbergon commented Oct 17, 2022

Uh oh!

zhaolihe commented Oct 17, 2023

Uh oh!

wgtmac commented Oct 18, 2023

Uh oh!

mwong38 commented Nov 26, 2023

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

shangxinli commented Nov 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

sheinbergon commented Mar 6, 2022 •

edited

Loading