Add protobuf codegen decoder #12980

rseetham · 2024-04-22T04:36:23Z

resolves #12679

Design doc: https://docs.google.com/document/d/186mVLISgYEmljla3-IHrFNJ2dGMJ21qJ-MWFtsx-ayI/

Test resources have the generated code we expect for the different test proto files.

codecov-commenter · 2024-04-22T05:14:21Z

Codecov Report

Attention: Patch coverage is 0% with 324 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (59551e4) to head (6ed7642).
Report is 1278 commits behind head on master.

Files with missing lines	Patch %	Lines
...n/inputformat/protobuf/codegen/MessageCodeGen.java	0.00%	178 Missing ⚠️
...ugin/inputformat/protobuf/ProtoBufSchemaUtils.java	0.00%	54 Missing ⚠️
...not/plugin/inputformat/protobuf/ProtoBufUtils.java	0.00%	54 Missing ⚠️
...format/protobuf/ProtoBufCodeGenMessageDecoder.java	0.00%	38 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (59551e4) and HEAD (6ed7642). Click for more details.

HEAD has 43 uploads less than BASE

Flag BASE (59551e4) HEAD (6ed7642)

integration 7 3

temurin 12 3

java-21 7 2

skip-bytebuffers-true 3 1

skip-bytebuffers-false 7 2

unittests 5 0

unittests1 2 0

java-11 5 1

unittests2 3 0

integration1 2 0

custom-integration1 2 0

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #12980       +/-   ##
=============================================
- Coverage     61.75%    0.00%   -61.76%     
=============================================
  Files          2436     2443        +7     
  Lines        133233   134535     +1302     
  Branches      20636    20824      +188     
=============================================
- Hits          82274        0    -82274     
- Misses        44911   134535    +89624     
+ Partials       6048        0     -6048

Flag	Coverage Δ
custom-integration1	`?`
integration	`0.00% <0.00%> (-0.01%)`	⬇️
integration1	`?`
integration2	`0.00% <0.00%> (ø)`
java-11	`0.00% <0.00%> (-61.71%)`	⬇️
java-21	`0.00% <0.00%> (-61.63%)`	⬇️
skip-bytebuffers-false	`0.00% <0.00%> (-61.75%)`	⬇️
skip-bytebuffers-true	`0.00% <0.00%> (-27.73%)`	⬇️
temurin	`0.00% <0.00%> (-61.76%)`	⬇️
unittests	`?`
unittests1	`?`
unittests2	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rseetham · 2024-04-24T20:18:29Z

@KKcorps Could you take a look.

chenboat · 2024-05-01T02:23:54Z

Can you link the overall design doc for this in the summary section?

...not-input-format/pinot-protobuf/src/main/java/com/google/protobuf/ProtobufInternalUtils.java

...pinot-protobuf/src/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufUtils.java

...src/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufCodeGenMessgeDecoder.java

...pinot-protobuf/src/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufUtils.java

deemoliu · 2024-05-02T23:00:53Z

...uf/src/test/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufTestDataGenerator.java

+  public static final String NULLABLE_DOUBLE_FIELD = "nullable_double_field";
+  public static final String NULLABLE_FLOAT_FIELD = "nullable_float_field";
+  public static final String NULLABLE_BOOL_FIELD = "nullable_bool_field";
+  public static final String NULLABLE_BYTES_FIELD = "nullable_bytes_field";


what are the supported data types if pinot doesn't support the type natively.

See: We support all the protobuf descriptor types at present in the version we are using. If in the future new types get added, the codegen will skip that filed and log an error.
https://github.com/apache/pinot/pull/12980/files#diff-84564c2ff3bb09e23abe31143340f4e5a2aea0613588e1131311cb3ea17d4a5eR159-R177

...rc/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufCodeGenMessageDecoder.java

...pinot-protobuf/src/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufUtils.java

deemoliu · 2024-05-10T19:38:57Z

...pinot-protobuf/src/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufUtils.java

+          Descriptors.FieldDescriptor keyFd =
+              fd.getMessageType().findFieldByName("key");
+          Descriptors.FieldDescriptor valueFd =
+              fd.getMessageType().findFieldByName("value");


...pinot-protobuf/src/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufUtils.java

deemoliu · 2024-05-10T19:45:35Z

...protobuf/src/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufSchemaUtils.java

+    }
+  }
+
+  private static void addFieldToPinotSchema(Schema pinotSchema, FieldSpec.DataType dataType, String name,


is this code re-used from somewhere?

Yes. This is from AvroUtils and JsonUtils. They both have this function:
AvroUtils
JsonUtils

We should add it to a common package and use it. I can do that in a follow up pr.

deemoliu

LGTM. can you ask for another stamp from Ting Chen

…upport for complex type schema

Jackie-Jiang · 2024-05-19T00:40:38Z

pinot-plugins/pinot-input-format/pinot-protobuf/src/test/resources/complex_type_schema.json

+    "format" : "1:MILLISECONDS:EPOCH",
+    "granularity" : "1:MILLISECONDS"
+  } ]
+}


(format) Please add a extra empty line

Jackie-Jiang · 2024-05-19T00:41:59Z

pinot-plugins/pinot-input-format/pinot-protobuf/pom.xml

@@ -50,6 +50,11 @@
      <groupId>com.google.protobuf</groupId>
      <artifactId>protobuf-java</artifactId>
    </dependency>
+    <dependency>


Any specific reason why we need to override this version? This can potentially cause version conflict.

No. Only pinot-query-planner is using the org.codehaus.janino dependency. I's also using 3.1.6 version. There's no common version so I specified it here as well

We have pinned its version to 3.1.12 in #12724. Maybe you didn't pull the latest code when merging this

* Add protobuf codegen decoder with unit tests * Add utils functions to convert protobuf schema to pinot schema with support for complex type schema

rseetham force-pushed the protbuf_codegen branch 2 times, most recently from 4615a6c to b64cfa1 Compare April 22, 2024 15:08

Jackie-Jiang added feature ingestion labels Apr 22, 2024

rseetham marked this pull request as ready for review April 22, 2024 16:57

rseetham force-pushed the protbuf_codegen branch from b64cfa1 to 9ca47ac Compare April 24, 2024 05:52

rseetham changed the title ~~Add protobuf codegen decoder with unit tests~~ Add protobuf codegen decoder Apr 24, 2024

chenboat reviewed May 1, 2024

View reviewed changes

...not-input-format/pinot-protobuf/src/main/java/com/google/protobuf/ProtobufInternalUtils.java Outdated Show resolved Hide resolved

chenboat reviewed May 1, 2024

View reviewed changes

...pinot-protobuf/src/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufUtils.java Outdated Show resolved Hide resolved

deemoliu reviewed May 2, 2024

View reviewed changes

...src/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufCodeGenMessgeDecoder.java Outdated Show resolved Hide resolved

deemoliu reviewed May 2, 2024

View reviewed changes

rseetham force-pushed the protbuf_codegen branch 4 times, most recently from 003bf3f to 8bf1d6d Compare May 10, 2024 18:34

deemoliu reviewed May 10, 2024

View reviewed changes

...rc/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufCodeGenMessageDecoder.java Outdated Show resolved Hide resolved

deemoliu reviewed May 10, 2024

View reviewed changes

...pinot-protobuf/src/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufUtils.java Outdated Show resolved Hide resolved

deemoliu reviewed May 10, 2024

View reviewed changes

...pinot-protobuf/src/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufUtils.java Outdated Show resolved Hide resolved

deemoliu reviewed May 10, 2024

View reviewed changes

rseetham force-pushed the protbuf_codegen branch 2 times, most recently from 4ce5008 to 2ccb920 Compare May 13, 2024 17:21

deemoliu approved these changes May 14, 2024

View reviewed changes

rseetham force-pushed the protbuf_codegen branch 4 times, most recently from 80bad7c to 47b279b Compare May 15, 2024 15:54

Add protobuf codegen decoder with unit tests

d4a189d

rseetham force-pushed the protbuf_codegen branch from 47b279b to 11dd49b Compare May 15, 2024 15:56

Add utils functions to convert protobuf schema to pinot schema with s…

6ed7642

…upport for complex type schema

rseetham force-pushed the protbuf_codegen branch from 11dd49b to 6ed7642 Compare May 15, 2024 16:58

rseetham requested a review from chenboat May 15, 2024 22:39

chenboat approved these changes May 17, 2024

View reviewed changes

chenboat merged commit 5489d5a into apache:master May 17, 2024
22 checks passed

Jackie-Jiang reviewed May 19, 2024

View reviewed changes

gortiz pushed a commit to gortiz/pinot that referenced this pull request Jun 14, 2024

Add protobuf codegen decoder (apache#12980)

ef19a1f

* Add protobuf codegen decoder with unit tests * Add utils functions to convert protobuf schema to pinot schema with support for complex type schema

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add protobuf codegen decoder #12980

Add protobuf codegen decoder #12980

rseetham commented Apr 22, 2024 •

edited

Loading

codecov-commenter commented Apr 22, 2024 •

edited

Loading

rseetham commented Apr 24, 2024

chenboat commented May 1, 2024

deemoliu May 2, 2024

rseetham May 8, 2024

deemoliu May 10, 2024

deemoliu May 10, 2024

rseetham May 10, 2024

rseetham May 13, 2024

deemoliu left a comment

Jackie-Jiang May 19, 2024

Jackie-Jiang May 19, 2024

rseetham May 20, 2024

Jackie-Jiang May 20, 2024 •

edited

Loading

Add protobuf codegen decoder #12980

Add protobuf codegen decoder #12980

Conversation

rseetham commented Apr 22, 2024 • edited Loading

codecov-commenter commented Apr 22, 2024 • edited Loading

Codecov Report

rseetham commented Apr 24, 2024

chenboat commented May 1, 2024

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

deemoliu 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

Jackie-Jiang May 20, 2024 • edited Loading

Choose a reason for hiding this comment

rseetham commented Apr 22, 2024 •

edited

Loading

codecov-commenter commented Apr 22, 2024 •

edited

Loading

Jackie-Jiang May 20, 2024 •

edited

Loading