Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

mydump: support multi bytes csv delimiter and separator #406

Merged
merged 8 commits into from
Sep 27, 2020
Merged

mydump: support multi bytes csv delimiter and separator #406

merged 8 commits into from
Sep 27, 2020

Conversation

glorv
Copy link
Contributor

@glorv glorv commented Sep 23, 2020

What problem does this PR solve?

partially reslove #321

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Possible performance regression
  • Increased code complexity

Related changes

  • Need to update the documentation
  • Need to be included in the release note

@glorv
Copy link
Contributor Author

glorv commented Sep 23, 2020

/run-all-tests

@djshow832
Copy link

/run-all-tests tidb=pr/20191

1 similar comment
@djshow832
Copy link

/run-all-tests tidb=pr/20191

kennytm
kennytm reviewed Sep 23, 2020
View reviewed changes
lightning/config/config.go Outdated Show resolved Hide resolved
lightning/config/config_test.go Show resolved Hide resolved
lightning/mydump/bytes.go Outdated Show resolved Hide resolved
lightning/mydump/csv_parser.go Outdated Show resolved Hide resolved
lightning/mydump/csv_parser.go Outdated Show resolved Hide resolved
lightning/mydump/csv_parser.go Outdated Show resolved Hide resolved
@kennytm kennytm added the Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated label Sep 23, 2020
kennytm
kennytm reviewed Sep 24, 2020
View reviewed changes
Copy link
Collaborator

@kennytm kennytm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update the description in tidb-lightning.toml.

rest LGTM

@kennytm kennytm added the status/LGT1 One reviewer already commented LGTM (LGTM1) label Sep 25, 2020
overvenus
overvenus approved these changes Sep 27, 2020
View reviewed changes
Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@overvenus overvenus added status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) and removed status/LGT1 One reviewer already commented LGTM (LGTM1) labels Sep 27, 2020
@kennytm kennytm merged commit bce9977 into master Sep 27, 2020
@kennytm kennytm deleted the flexible-csv branch September 27, 2020 05:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kennytm kennytm kennytm left review comments

@overvenus overvenus overvenus approved these changes

@YuJuncen YuJuncen Awaiting requested review from YuJuncen

@3pointer 3pointer Awaiting requested review from 3pointer

Assignees
No one assigned
Labels
Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@glorv @djshow832 @kennytm @overvenus