[GitHub] [pinot] snleee commented on a diff in pull request #11540: Keep header parsing logic the same in default CSVRecordReader and the line iterator

[via GitHub]

snleee commented on code in PR #11540:
URL: https://github.com/apache/pinot/pull/11540#discussion_r1319099667


##########
pinot-plugins/pinot-input-format/pinot-csv/src/main/java/org/apache/pinot/plugin/inputformat/csv/CSVRecordReader.java:
##########
@@ -104,50 +103,42 @@ public void init(File dataFile, @Nullable Set<String> 
fieldsToRead, @Nullable Re
         }
       }
       char delimiter = config.getDelimiter();
-      format = format.withDelimiter(delimiter);
+      format = format.builder().setDelimiter(delimiter).build();
 
       if (config.isSkipUnParseableLines()) {
         _useLineIterator = true;
       }
 
-      String csvHeader = config.getHeader();
-      if (csvHeader == null) {
-        format = format.withHeader();
-      } else {
-        // do not validate header if using the line iterator
-        if (_useLineIterator) {
-          String[] header = StringUtils.split(csvHeader, delimiter);
-          setHeaderMap(header);
-          format = format.withHeader(header);
-          _isHeaderProvided = true;
-        } else {
-          // validate header for the delimiter before splitting
-          validateHeaderForDelimiter(delimiter, csvHeader, format);
-          format = format.withHeader(StringUtils.split(csvHeader, delimiter));
-        }
-      }
-      format = format.withSkipHeaderRecord(config.isSkipHeader());
+      _isHeaderProvided = config.getHeader() != null;
       _skipHeaderRecord = config.isSkipHeader();
-      format = format.withCommentMarker(config.getCommentMarker());
-      format = format.withEscape(config.getEscapeCharacter());
-      format = format.withIgnoreEmptyLines(config.isIgnoreEmptyLines());
-      format = 
format.withIgnoreSurroundingSpaces(config.isIgnoreSurroundingSpaces());
-      format = format.withQuote(config.getQuoteCharacter());
+      _format = format.builder()
+          .setHeader()
+          .setSkipHeaderRecord(config.isSkipHeader())
+          .setCommentMarker(config.getCommentMarker())
+          .setEscape(config.getEscapeCharacter())
+          .setIgnoreEmptyLines(config.isIgnoreEmptyLines())
+          .setIgnoreSurroundingSpaces(config.isIgnoreSurroundingSpaces())
+          .setQuote(config.getQuoteCharacter())
+          .build();
 
       if (config.getQuoteMode() != null) {
-        format = 
format.withQuoteMode(QuoteMode.valueOf(config.getQuoteMode()));
+        _format = 
_format.builder().setQuoteMode(QuoteMode.valueOf(config.getQuoteMode())).build();

Review Comment:
   `_format.builder()` will return the new builder object or existing builder?



##########
pinot-plugins/pinot-input-format/pinot-csv/src/main/java/org/apache/pinot/plugin/inputformat/csv/CSVRecordReader.java:
##########
@@ -104,50 +103,42 @@ public void init(File dataFile, @Nullable Set<String> 
fieldsToRead, @Nullable Re
         }
       }
       char delimiter = config.getDelimiter();
-      format = format.withDelimiter(delimiter);
+      format = format.builder().setDelimiter(delimiter).build();
 
       if (config.isSkipUnParseableLines()) {
         _useLineIterator = true;
       }
 
-      String csvHeader = config.getHeader();
-      if (csvHeader == null) {
-        format = format.withHeader();
-      } else {
-        // do not validate header if using the line iterator
-        if (_useLineIterator) {
-          String[] header = StringUtils.split(csvHeader, delimiter);
-          setHeaderMap(header);
-          format = format.withHeader(header);
-          _isHeaderProvided = true;
-        } else {
-          // validate header for the delimiter before splitting
-          validateHeaderForDelimiter(delimiter, csvHeader, format);
-          format = format.withHeader(StringUtils.split(csvHeader, delimiter));
-        }
-      }
-      format = format.withSkipHeaderRecord(config.isSkipHeader());
+      _isHeaderProvided = config.getHeader() != null;

Review Comment:
   Why we remove the header validation? I do see the good checks (e.g. having 
multiple values, check for delimiter etc)



##########
pinot-plugins/pinot-input-format/pinot-csv/src/main/java/org/apache/pinot/plugin/inputformat/csv/CSVRecordReader.java:
##########
@@ -77,7 +76,7 @@ public void init(File dataFile, @Nullable Set<String> 
fieldsToRead, @Nullable Re
     CSVRecordReaderConfig config = (CSVRecordReaderConfig) recordReaderConfig;
     Character multiValueDelimiter = null;
     if (config == null) {
-      _format = 
CSVFormat.DEFAULT.withDelimiter(CSVRecordReaderConfig.DEFAULT_DELIMITER).withHeader();
+      _format = 
CSVFormat.DEFAULT.builder().setDelimiter(CSVRecordReaderConfig.DEFAULT_DELIMITER).setHeader().build();

Review Comment:
   Does this mean that the library builder API is not backward compatibile?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org