-
Notifications
You must be signed in to change notification settings - Fork 102
lightning/mydump: support mydumper.csv.terminator #1297
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -535,13 +536,20 @@ func (cfg *Config) Adjust(ctx context.Context) error { | |||
return errors.New("invalid config: `mydumper.csv.separator` and `mydumper.csv.delimiter` must not be prefix of each other") | |||
} | |||
|
|||
if len(csv.Terminator) > 0 && cfg.Mydumper.StrictFormat { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also return an error if both Terminator
and TrimLastSep
are set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@glorv Terminator and StrictFormat are currently mutually exclusive because the latter is hard-coded to use [\r\n]
for now. There's no problem using TrimLastSep and Terminator together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this will trim the last two seps, is there any scenario that actually need this? BTW, do we want to finally replace TrimLastSep
with Terminator
or they just two different feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the original purpose of TrimLastSep was to support importing TPC-H generated data in the form
a|b|c|
d|e|f|
, which we can use terminator = "|\n"
instead.
i don't know if there are any other use cases 😅
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 1d8dba0
|
What problem does this PR solve?
Allow user to specify a custom CSV line terminator (corresponding to LOAD DATA's LINES TERMINATED BY option.)
What is changed and how it works?
mydumper.csv.terminator
. When it is not empty, this will be used as the line separator instead of[\r\n]
.peekBytes
stuff randomly appearing somewhere.Check List
Tests
Code changes
Side effects
Related changes
Release note
\r
/\n
in CSV files.