CSV Realtime Decoder #8658

suddendust · 2022-05-07T06:20:47Z

codecov-commenter · 2022-05-07T21:15:06Z

Codecov Report

Merging #8658 (22a0b01) into master (4e14101) will increase coverage by 36.76%.
The diff coverage is 13.70%.

❗ Current head 22a0b01 differs from pull request most recent head 568a68d. Consider uploading reports for the commit 568a68d to get more accurate results

@@              Coverage Diff              @@
##             master    #8658       +/-   ##
=============================================
+ Coverage     29.36%   66.13%   +36.76%     
- Complexity        0     4385     +4385     
=============================================
  Files          1691     1277      -414     
  Lines         89308    64445    -24863     
  Branches      13530    10018     -3512     
=============================================
+ Hits          26225    42620    +16395     
+ Misses        60671    18806    -41865     
- Partials       2412     3019      +607

Flag	Coverage Δ
integration1	`?`
integration2	`?`
unittests1	`66.13% <13.70%> (?)`

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

Impacted Files	Coverage Δ
...common/config/provider/AccessControlUserCache.java	`0.00% <0.00%> (ø)`
...ache/pinot/common/metadata/ZKMetadataProvider.java	`14.52% <0.00%> (-64.67%)`	⬇️
...not/common/metadata/segment/SegmentZKMetadata.java	`53.74% <0.00%> (-28.58%)`	⬇️
...a/org/apache/pinot/common/metrics/BrokerMeter.java	`93.02% <ø> (-6.98%)`	⬇️
...t/common/response/broker/BrokerResponseNative.java	`92.48% <ø> (+6.15%)`	⬆️
...ava/org/apache/pinot/common/utils/BcryptUtils.java	`0.00% <0.00%> (ø)`
.../main/java/org/apache/pinot/common/utils/Pair.java	`60.00% <ø> (ø)`
...mon/utils/config/AccessControlUserConfigUtils.java	`0.00% <0.00%> (ø)`
...g/apache/pinot/common/utils/helix/HelixHelper.java	`13.30% <0.00%> (-37.50%)`	⬇️
...pache/pinot/common/utils/request/RequestUtils.java	`81.57% <ø> (+49.60%)`	⬆️
... and 1584 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e14101...568a68d. Read the comment docs.

Jackie-Jiang · 2022-05-09T17:45:42Z

...ormat/pinot-csv/src/main/java/org/apache/pinot/plugin/inputformat/csv/CSVMessageDecoder.java

+
+public class CSVMessageDecoder implements StreamMessageDecoder<byte[]> {
+
+  private static final String CONFIG_FILE_FORMAT = "csv.fileFmt";


Since the properties is under the context of the decoder, we may remove the csv. prefix for simplicity and consistency with other decoders. Also, suggest using the full name for the key, e.g. fileFormat, header, delimiter etc.

Updated. My thought process was that

...decoder.props.csv.delimiter

is more idiomatic than:

...decoder.props.delimiter

, as the former tells the users that this property configures the delimiter of the CSV record. But I think this prefix is not really needed as the user already configures the decoder class in the config, so he would already know what these props are about even without the prefix. Thanks!

Jackie-Jiang · 2022-05-09T17:46:41Z

...ormat/pinot-csv/src/main/java/org/apache/pinot/plugin/inputformat/csv/CSVMessageDecoder.java

+          format = CSVFormat.TDF;
+          break;
+        default:
+          format = CSVFormat.DEFAULT;


Log a warning here stating that we cannot recognize the format, and fall back to the default

Jackie-Jiang · 2022-05-09T17:47:32Z

...t/pinot-csv/src/test/java/org/apache/pinot/plugin/inputformat/csv/CSVMessageDecoderTest.java

+    String incomingRecord = "Alice;18;F";
+    GenericRow destination = new GenericRow();
+    messageDecoder.decode(incomingRecord.getBytes(StandardCharsets.UTF_8), destination);
+    Assert.assertNotNull(destination.getValue("name"));


(minor) We may static import these Assert methods in tests

suddendust · 2022-05-10T14:36:50Z

@Jackie-Jiang Requesting re-review, thanks :)

suddendust · 2022-05-13T04:39:49Z

@Jackie-Jiang @npawar Just a reminder, thanks :)

Jackie-Jiang

LGTM with some minor comments. Thanks for contributing the feature!

Jackie-Jiang · 2022-05-10T20:57:13Z

...ormat/pinot-csv/src/main/java/org/apache/pinot/plugin/inputformat/csv/CSVMessageDecoder.java

+          format = CSVFormat.TDF;
+          break;
+        default:
+          LOGGER.info("Could not recognise the configured CSV file format: {}, falling back to DEFAULT format",


Log warning for unrecognized format

Ohh missed it, done.

Jackie-Jiang · 2022-05-10T20:57:36Z

...ormat/pinot-csv/src/main/java/org/apache/pinot/plugin/inputformat/csv/CSVMessageDecoder.java

+    if (csvFormat == null) {
+      format = CSVFormat.DEFAULT;
+    } else {
+      switch (csvFormat.toUpperCase()) {


Add a case for the "DEFAULT"

Jackie-Jiang · 2022-05-10T21:00:37Z

...ormat/pinot-csv/src/main/java/org/apache/pinot/plugin/inputformat/csv/CSVMessageDecoder.java

+    } else {
+      format = format.withHeader(StringUtils.split(csvHeader, csvDelimiter));
+    }
+    if (props.containsKey(CONFIG_COMMENT_MARKER)) {


(minor) Follow the same way aw csvDelimiter to reduce one map lookup (get and check if it is null), same for other configs

Jackie-Jiang · 2022-05-10T21:01:40Z

...ormat/pinot-csv/src/main/java/org/apache/pinot/plugin/inputformat/csv/CSVMessageDecoder.java

+      format = format.withHeader(StringUtils.split(csvHeader, csvDelimiter));
+    }
+    if (props.containsKey(CONFIG_COMMENT_MARKER)) {
+      Character commentMarker = props.get(CONFIG_COMMENT_MARKER).charAt(0);


(minor) no need to boxing, same for other configs

Inlined all of the configs.

suddendust added 5 commits May 4, 2022 00:00

Initial commit: Decoder

6cf9291

Decoder test

c8ab7c7

pop me

29a3719

More test cases

3c9948a

Fix property names

ce9ad4e

suddendust mentioned this pull request May 7, 2022

Add CSV Decoder for realtime #8617

Closed

suddendust added 2 commits May 7, 2022 11:58

Linter: License headers

2333934

Checkstyle

fb8ae4e

suddendust added 2 commits May 8, 2022 11:21

JDK 8

5783086

Additional test cases

f59a80f

Jackie-Jiang reviewed May 9, 2022

View reviewed changes

suddendust added 3 commits May 10, 2022 12:58

Addressed PR comments

6632c82

Lint fix

13445bd

Fix test

9c746f7

Jackie-Jiang approved these changes May 13, 2022

View reviewed changes

Jackie-Jiang added the feature label May 13, 2022

Addressed comments

568a68d

Jackie-Jiang merged commit 7548b67 into apache:master May 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSV Realtime Decoder #8658

CSV Realtime Decoder #8658

suddendust commented May 7, 2022 •

edited

Loading

codecov-commenter commented May 7, 2022 •

edited

Loading

Jackie-Jiang May 9, 2022

suddendust May 10, 2022

Jackie-Jiang May 9, 2022

suddendust May 10, 2022

Jackie-Jiang May 9, 2022

suddendust May 10, 2022

suddendust commented May 10, 2022

suddendust commented May 13, 2022

Jackie-Jiang left a comment

Jackie-Jiang May 10, 2022

suddendust May 14, 2022

Jackie-Jiang May 10, 2022

suddendust May 14, 2022

Jackie-Jiang May 10, 2022

Jackie-Jiang May 10, 2022

suddendust May 14, 2022 •

edited

Loading


		public class CSVMessageDecoder implements StreamMessageDecoder<byte[]> {

		private static final String CONFIG_FILE_FORMAT = "csv.fileFmt";

CSV Realtime Decoder #8658

CSV Realtime Decoder #8658

Conversation

suddendust commented May 7, 2022 • edited Loading

codecov-commenter commented May 7, 2022 • edited Loading

Codecov Report

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

suddendust commented May 10, 2022

suddendust commented May 13, 2022

Jackie-Jiang 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

suddendust May 14, 2022 • edited Loading

Choose a reason for hiding this comment

suddendust commented May 7, 2022 •

edited

Loading

codecov-commenter commented May 7, 2022 •

edited

Loading

suddendust May 14, 2022 •

edited

Loading