add TsvInputFormat by SEKIRO-J · Pull Request #8915 · apache/druid

SEKIRO-J · 2019-11-21T00:59:35Z

Description

add support for TSV format file.
add tsvInputFormat, tsvReader, and unit tests for them.

tsvInputFormat and csvInputFormat are pretty similar, I'm thinking have a parent class called seperateValueInputFormat implements InputFormat
and
tsvInputFormat extends seperateValueInputFormat, csvInputFormat extends seperateValueInputFormat

Configuration Interface Design

 "inputFormat": {
  "type": "tsv",
  "columns": [ "col1", "col2", "col3" ],
  "listDelimiter": "|",
  "findColumnsFromHeader" : true,
  "skipHeaderRows" : 3
}

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added documentation for new or modified features or behaviors.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths.
added integration tests.
been tested in a test Druid cluster.

Key changed/added classes in this PR

tsvInputFormat
tsvReader

a2l007

Sorry for being the grammar police here, but it would be better if we correct Seperate to Separate

SEKIRO-J · 2019-11-21T18:10:30Z

Sorry for being the grammar police here, but it would be better if we correct Seperate to Separate

thank you for pointing out

ccaominh · 2019-11-21T18:55:29Z

+    if (!this.columns.isEmpty()) {
+      for (String column : this.columns) {
+        Preconditions.checkArgument(
+            !column.contains("tab".equals(seperator) ? "\t" : ","),


What do you think of using something like the AbstractFlatTextFormatParser.FlatTextFormat enum for this instead of hard-coding strings?

it's a good idea, working on that.

suneet-s · 2019-11-22T05:44:20Z

+  public int hashCode()
+  {
+    return Objects.hash(listDelimiter, columns, findColumnsFromHeader, skipHeaderRows);
+  }


Looks like format is missing in equalsAndHashCode

jon-wei · 2019-11-22T06:13:57Z

+    this.multiValueFunction = ParserUtils.getMultiValueFunction(finalListDelimeter, Splitter.on(finalListDelimeter));
+    this.columns = findColumnsFromHeader ? null : columns; // columns will be overriden by header row
+    this.format = format;
+    this.parser = createOpenCsvParser(format.getDefaultDelimiter().charAt(0));


I think it'd be cleaner for the format to have a method that returns a char for this

suneet-s

Thanks for this PR! I read through it and have some overall thoughts. I'm just asking a bunch of questions to get a better understanding.

Overall comments:

Please add javadocs to the new files you've created. I think that will help clear up a lot of my confusion.
Could you explain why you chose to have the CSV* and TSV* classes extend the SeparateValue* classes. I like your idea of sharing the logic between them in a common class, but I think composition might be an easier approach to follow (ie CSV* and TSV* classes have a delegate SeparateValue* object that is instantiated differently based on which class is calling it)

suneet-s · 2019-11-22T18:08:19Z

+    CSV(","),
+    TSV("\t");
+
+    private final String defaultDelimiter;


Why defaultDelimiter Should this just be delimiter?

suneet-s · 2019-11-22T18:09:23Z

+
+    public String getLiteral()
+    {
+      return ",".equals(defaultDelimiter) ? "comma" : "tab";


nit: IMO it'd be easier to read if the enum had to define both the delimiter and the literal

CSV(",", "comma")

suneet-s · 2019-11-22T18:10:18Z

+  private final FlatTextFormat format;
+
+  @JsonCreator
+  public SeparateValueInputFormat(


Is this class ever instantiated via json? I think it's just a base class if I'm reading the PR correctly - so I think we can remove all the Json annotations. And I think the constructor should only be package private

suneet-s · 2019-11-22T18:13:07Z

+    );
+  }
+
+  public InputEntityReader createReader(


sorry, I find this a little confusing. The function to create a reader above creates a SeparateValueReader, but if you pass in a format it constructs either a CSVReader or a TSVReader. I think I'm finding it hard to wrap my head around the differences

Sorry I found that confusing too, will refactor that code

suneet-s · 2019-11-22T18:22:52Z

+  }
+
+  @VisibleForTesting
+  public TsvInputFormat(


Do we really need this public constructor to be visible for testing? If it was package private, that might have been ok, but it's hard to enforce public constructors are not used elsewhere in the main source code.

Maybe it's better if the tests are explicit about passing hasHeaderRow as null when instantiating this object

jon-wei · 2019-11-22T20:58:29Z

+ * It implements the common logic between {@link CsvInputFormat} and {@link TsvInputFormat}
+ * Should never be instantiated
+ */
+public class SeparateValueInputFormat implements InputFormat


This can be an abstract class and you can leave the "Should never be instantiated" comment out

jon-wei

LGTM after CI

SEKIRO-J added 2 commits November 20, 2019 19:52

add TsvInputFormat

1c0a219

refactor code

6590282

SEKIRO-J force-pushed the tsvInputFormat branch from fd1979e to 6590282 Compare November 21, 2019 03:52

vogievetsky added the Area - Batch Ingestion label Nov 21, 2019

a2l007 reviewed Nov 21, 2019

View reviewed changes

fix grammar

f025bff

ccaominh reviewed Nov 21, 2019

View reviewed changes

use enum replace string literal

86ad997

suneet-s reviewed Nov 22, 2019

View reviewed changes

jon-wei reviewed Nov 22, 2019

View reviewed changes

suneet-s reviewed Nov 22, 2019

View reviewed changes

SEKIRO-J added 2 commits November 22, 2019 11:22

code refactor

4c69223

code refactor

c3c5556

jon-wei reviewed Nov 22, 2019

View reviewed changes

SEKIRO-J added 2 commits November 22, 2019 13:25

mark abstract for base class meant not to be instantiated

83a2478

remove constructor for test

cd2666a

jon-wei approved these changes Nov 23, 2019

View reviewed changes

jon-wei merged commit 0514e56 into apache:master Nov 23, 2019

SEKIRO-J mentioned this pull request Dec 5, 2019

add customize separator for TSV inputFormat #8993

Merged

9 tasks

jon-wei added this to the 0.17.0 milestone Dec 17, 2019

jihoonson mentioned this pull request Jan 17, 2020

Fix TSV bugs #9199

Merged

9 tasks

Conversation

SEKIRO-J commented Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Configuration Interface Design

Key changed/added classes in this PR

Uh oh!

a2l007 left a comment

Choose a reason for hiding this comment

Uh oh!

SEKIRO-J commented Nov 21, 2019

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!

suneet-s left a comment

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!

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!

jon-wei Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jon-wei left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

SEKIRO-J commented Nov 21, 2019 •

edited

Loading

jon-wei Nov 22, 2019 •

edited

Loading