[FLINK-5714] [table] Use a builder pattern for creating CsvTableSource #3273
Conversation
wuchong
force-pushed
the
CsvTableSource-builder-FLINK-5714
branch
from
10ffca1 to
751e7a7
Compare
docs/dev/table_api.md
Outdated
| - `fieldDelimiter(String delim)` Sets the field delimiter, `","` by default. | ||
| - `lineDelimiter(String delim)` Sets the line delimiter, `"\n"` by default. | ||
| - `quoteCharacter(Character quote)` Sets the quote character for String values, `null` by default. | ||
| - `commentPrefix(String prefix)` Sets a prefix to indicate comments, null by default. |
There was a problem hiding this comment.
Shall we change to null to null to keep consistency
docs/dev/table_api.md
Outdated
| - `lineDelimiter(String delim)` Sets the line delimiter, `"\n"` by default. | ||
| - `quoteCharacter(Character quote)` Sets the quote character for String values, `null` by default. | ||
| - `commentPrefix(String prefix)` Sets a prefix to indicate comments, null by default. | ||
| - `ignoreFirstLine()` Ignore the first line. Not skip the first line by default. |
| * For example: | ||
| * | ||
| * {{{ | ||
| * val source: CsvTableSource = new CsvTableSourceBuilder() |
There was a problem hiding this comment.
CsvTableSourceBuilder should be CsvTableSource.builder()
and does this comment a little duplicated with method builder ?
There was a problem hiding this comment.
Yes, new CsvTableSourceBuilder() should be new CsvTableSource.Builder(). The comment is a little duplicate. But I think it is reasonable to let user create a builder by the constructor, not only the static builder method.
| */ | ||
| class Builder { | ||
|
|
||
| private val fieldNames: ListBuffer[String] = ListBuffer[String]() |
There was a problem hiding this comment.
Can we bind the name and type to a MutableMap and do some check when we adding fields? Like can not have duplicated filed names?
| ignoreFirstLine: Boolean = false, | ||
| ignoreComments: String = null, | ||
| lenient: Boolean = false) | ||
| private val path: String, |
There was a problem hiding this comment.
I think these two forms are identical
There was a problem hiding this comment.
Actually, I want to access these fields in the equals(Object) by that.path. The private val modifier will create getter/setter implicitly.
| * @return a newly-created [[CsvTableSource]]. | ||
| */ | ||
| def build: CsvTableSource = { | ||
| Preconditions.checkNotNull(path, "Path must not be null.") |
There was a problem hiding this comment.
Fields can not be empty too?
KurtYoung
left a comment
KurtYoung
left a comment
There was a problem hiding this comment.
Thanks for the work, looks good to me, i only have some minor comments
|
Thank you for your advice. I will update the PR soon. |
|
updated! |
| def build: CsvTableSource = { | ||
| Preconditions.checkNotNull(path, "Path must not be null.") | ||
| if (path == null) { | ||
| throw new IllegalArgumentException("Path must be defined.") |
There was a problem hiding this comment.
You can still use Preconditions.checkArgument though.
|
Looks good to merge. Merging... |
|
I found another issue: We should use a |
5 participants
Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the How To Contribute guide.
In addition to going through the list, please provide a meaningful description of your changes.
General
Documentation
Tests & Build
mvn clean verifyhas been executed successfully locally or a Travis build has passedAdd a builder to create CsvTableSource. And I have also updated the documentation.