PARQUET-1711: support recursive proto schemas by limiting recursion depth #995

jinyius · 2022-09-08T07:42:45Z

This is an alternative approach to supporting recursion to Proposed change in AvroSchema to handle circular references. #445 and PARQUET-1711: Break circular dependencies in proto definitions #988.
This approach could address the other recursion related issues (PARQUET-129, PARQUET-554).
TODO: ReadSupport

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-1711
- 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:
- ProtoSchemaConverterTest#test*Recursion
- ProtoWriteSupportTest#test*Recursion

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

jinyius · 2022-09-14T22:02:07Z

ping

jinyius · 2022-09-28T18:06:12Z

fixed missing dep issue. can someone approve the ci flow?

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

emkornfield · 2022-09-30T06:08:33Z

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

@@ -99,9 +139,9 @@ private Type.Repetition getRepetition(FieldDescriptor descriptor) {
    }
  }

-  private <T> Builder<? extends Builder<?, GroupBuilder<T>>, GroupBuilder<T>> addField(FieldDescriptor descriptor, final GroupBuilder<T> builder) {
+  private <T> Builder<? extends Builder<?, GroupBuilder<T>>, GroupBuilder<T>> addField(FieldDescriptor descriptor, final GroupBuilder<T> builder, ImmutableSetMultimap<String, Integer> seen, int depth) {


would it make sense to consolidate seen and depth into a single data-structure that can be passed through and abstract some of the direct access to the multimap?

?

the seen map does encode the seen fields along with their depth as a single datastructure. depth being a separate arg is important b/c it's the current depth in the traversal, and is used to update the seen data structure.

right, I was thinking of encapsulating this logic into its own class, so they can be recorded and updated together, to 1. Reduce additional parameters that have to be passed through.
2. Encapsulate the logic behind more mnemonic method names (e.g. AddRecursiveStep())

i'm not sure encapsulation helps with readability or protection in this case. they are really tracking different things, and should be understood by readers of the traversal code to know how each piece of state is used.

jinyius · 2022-09-30T18:17:56Z

thanks for the review. updated to handle the logging perf concern as well as fixing the javadoc errors.

…epth This approach could address the other recursion related issues (PARQUET-129, PARQUET-554).

jinyius · 2022-10-07T16:49:17Z

ping

emkornfield · 2022-10-09T03:23:16Z

parquet-protobuf/src/test/java/org/apache/parquet/proto/ProtoSchemaConverterTest.java

-        "  }\n" +
-        "}";
+  public void testProto3ConvertAllDatatypes() {
+    String expectedSchema = JOINER.join(


is it possible to separate this tpe of code style cleanup from functional changes?

wdym by "tpe"?

if this isn't blocking, i'd rather avoid the busy-work to undo and redo in a different branch.

emkornfield · 2022-10-09T03:29:06Z

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

+          : value instanceof Message
+          ? ((Message) value).toByteString()
+          // Worst-case, just dump as plain java string.
+          : ByteString.copyFromUtf8(value.toString());


is this actually an intended state? If not it is probably better to raise an exception then writing data that could possibly be hard to recover.

this is intended. for a real-time, production pipeline i'm working on, losing data as it passes through or killing the job b/c of an uncaught exception is problematic as it could lead to data loss and down time. this way, there's some way to know what the problematic data was and fix it properly asap.

emkornfield · 2022-10-09T03:30:00Z

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

+          ? (ByteString) value
+          // TODO: figure out a way to use MessageOrBuilder
+          : value instanceof Message
+          ? ((Message) value).toByteString()


does recordconsumer offer a stream API or something else to avoid the additional array/bytestring copies?

no, afaict: https://www.javadoc.io/doc/org.apache.parquet/parquet-column/latest/org/apache/parquet/io/api/RecordConsumer.html

emkornfield · 2022-10-09T03:38:55Z

parquet-protobuf/src/test/resources/BinaryTree.par

@@ -0,0 +1,50 @@
+message Trees.BinaryTree {
+  optional group value = 1 {


Aren't groups deprecated?

or is par not proto?

this is parquet schema, not proto. protos should/would have a .proto suffix.

emkornfield · 2022-10-09T03:44:18Z

parquet-protobuf/src/test/resources/Trees.proto

+option java_package = "org.apache.parquet.proto.test";
+
+message BinaryTree {
+    google.protobuf.Any value = 1;


it would be good to verify that something like:

message WrappedTree {
google.protobuf.Any non_recursive = 1;
BinaryTree tree = 2;
}

Also gives expected results (non_recursive doesn't accidentally trigger any of the recursio logic).
}

i think the existing non-recursive proto tests exercise the existing and newly added (the skipping behavior) code paths.

emkornfield · 2022-10-09T03:45:27Z

Mostly looks reasonable, I'm not too familiar with parquet-mr @shangxinli can you recommend someone who might be able to give a better review?

jinyius · 2022-10-17T17:31:39Z

Mostly looks reasonable, I'm not too familiar with parquet-mr @shangxinli can you recommend someone who might be able to give a better review?

pinging @shangxinli :)

shangxinli · 2022-10-17T17:48:40Z

@ggershinsky Can you have a look?

ggershinsky

I'm ok with the current state of the PR, and would like to thank its reviewers.

ggershinsky · 2022-10-19T07:38:59Z

I would also like to recommend adding @matthieun as a co-author to this PR, per the discussion in the parallel PR.

jinyius · 2022-10-19T21:32:50Z

I would also like to recommend adding @matthieun as a co-author to this PR, per the discussion in the parallel PR.

~~how do you do this?~~ i think i did it right...

Co-authored-by: matthieun <matthieu.nahoum@gmail.com>

jinyius · 2022-10-24T17:42:00Z

can someone retry the github actions? there seemed to have been a transient issue that caused one of the test/build targets to fail. i'd like to get this change in this week.

emkornfield · 2022-10-31T04:18:14Z

@ggershinsky what is the process to merge this? Does parquet-mr just use the github UI?

ggershinsky · 2022-10-31T14:41:56Z

yep, just the squash/merge button.

jinyius · 2022-10-31T15:50:32Z

@ggershinsky

i'd love to just hit the button. i don't see it. the workflow for travis ci had a failure due to a transient connection issue, and so it wasn't giving me the option to merge. the ui messaging also states that "Only those with write access to this repository can merge pull requests."

ggershinsky · 2022-11-01T07:04:20Z

@shangxinli are you ok with this PR in its current form?

jinyius · 2022-11-01T15:50:36Z

yeah, i still don't see a button to merge. it now shows everything approved, checks passed, and no conflicts.

i think a committer needs to merge.

emkornfield · 2022-11-01T18:06:36Z

@jinyius only committers can see the button. I was asking because different repos have different commit procedures. Should be able to merge this soon as long as @shangxinli doesn't express concerns.

ggershinsky

Thanks @jinyius

shangxinli · 2022-11-02T14:44:14Z

LGTM

jinyius mentioned this pull request Sep 8, 2022

PARQUET-1711: Break circular dependencies in proto definitions #988

Closed

4 tasks

emkornfield reviewed Sep 30, 2022

View reviewed changes

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

emkornfield reviewed Sep 30, 2022

View reviewed changes

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

emkornfield reviewed Sep 30, 2022

View reviewed changes

jinyius added 2 commits October 3, 2022 05:10

PARQUET-1711: support recursive proto schemas by limiting recursion d…

b1f9361

…epth This approach could address the other recursion related issues (PARQUET-129, PARQUET-554).

PARQUET-1711: write recursive proto schemas with limited recursion depth

694423d

jinyius requested a review from emkornfield October 3, 2022 12:13

emkornfield reviewed Oct 9, 2022

View reviewed changes

jinyius requested a review from emkornfield October 17, 2022 17:31

ggershinsky approved these changes Oct 18, 2022

View reviewed changes

Make description a sentence. This commit is here to set a co-author.

258c66a

Co-authored-by: matthieun <matthieu.nahoum@gmail.com>

jinyius requested review from ggershinsky and emkornfield and removed request for emkornfield and ggershinsky October 19, 2022 22:07

jinyius requested review from emkornfield and ggershinsky and removed request for ggershinsky and emkornfield October 19, 2022 22:07

jinyius requested review from emkornfield and ggershinsky and removed request for ggershinsky and emkornfield October 25, 2022 16:49

jinyius closed this Oct 31, 2022

jinyius reopened this Oct 31, 2022

ggershinsky approved these changes Nov 2, 2022

View reviewed changes

shangxinli merged commit d75596b into apache:master Nov 2, 2022

jinyius deleted the feat/protorecurse branch November 2, 2022 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PARQUET-1711: support recursive proto schemas by limiting recursion depth #995

PARQUET-1711: support recursive proto schemas by limiting recursion depth #995

jinyius commented Sep 8, 2022

jinyius commented Sep 14, 2022

jinyius commented Sep 28, 2022

emkornfield Sep 30, 2022

jinyius Sep 30, 2022 •

edited

Loading

emkornfield Oct 9, 2022

jinyius Oct 10, 2022

jinyius commented Sep 30, 2022

jinyius commented Oct 7, 2022

emkornfield Oct 9, 2022

jinyius Oct 10, 2022

emkornfield Oct 9, 2022

jinyius Oct 10, 2022

emkornfield Oct 9, 2022

jinyius Oct 10, 2022

emkornfield Oct 9, 2022

emkornfield Oct 9, 2022

jinyius Oct 10, 2022

emkornfield Oct 9, 2022

jinyius Oct 10, 2022

emkornfield commented Oct 9, 2022

jinyius commented Oct 17, 2022

shangxinli commented Oct 17, 2022

ggershinsky left a comment

ggershinsky commented Oct 19, 2022

jinyius commented Oct 19, 2022 •

edited

Loading

jinyius commented Oct 24, 2022

emkornfield commented Oct 31, 2022

ggershinsky commented Oct 31, 2022

jinyius commented Oct 31, 2022

ggershinsky commented Nov 1, 2022

jinyius commented Nov 1, 2022

emkornfield commented Nov 1, 2022

ggershinsky left a comment

shangxinli commented Nov 2, 2022

		@@ -0,0 +1,50 @@
		message Trees.BinaryTree {
		optional group value = 1 {

PARQUET-1711: support recursive proto schemas by limiting recursion depth #995

PARQUET-1711: support recursive proto schemas by limiting recursion depth #995

Conversation

jinyius commented Sep 8, 2022

Jira

Tests

Commits

Documentation

jinyius commented Sep 14, 2022

jinyius commented Sep 28, 2022

Choose a reason for hiding this comment

jinyius Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinyius commented Sep 30, 2022

jinyius commented Oct 7, 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

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

emkornfield commented Oct 9, 2022

jinyius commented Oct 17, 2022

shangxinli commented Oct 17, 2022

ggershinsky left a comment

Choose a reason for hiding this comment

ggershinsky commented Oct 19, 2022

jinyius commented Oct 19, 2022 • edited Loading

jinyius commented Oct 24, 2022

emkornfield commented Oct 31, 2022

ggershinsky commented Oct 31, 2022

jinyius commented Oct 31, 2022

ggershinsky commented Nov 1, 2022

jinyius commented Nov 1, 2022

emkornfield commented Nov 1, 2022

ggershinsky left a comment

Choose a reason for hiding this comment

shangxinli commented Nov 2, 2022

jinyius Sep 30, 2022 •

edited

Loading

jinyius commented Oct 19, 2022 •

edited

Loading