Skip to content
This repository has been archived by the owner on Aug 21, 2023. It is now read-only.

support both filesize and rows arguments #177

Merged
merged 10 commits into from
Nov 9, 2020

Conversation

lichunzhu
Copy link
Contributor

@lichunzhu lichunzhu commented Oct 27, 2020

What problem does this PR solve?

Fix #164

What is changed and how it works?

Support configuring both -F and -r arguments.
When specifying -F or -r argument, the dumped SQL format is ${db}.${table}.%09d.sql.
When specifying both -F and -r arguments, the dumped SQL format is ${db}.${table}.%09d%04d.sql.

Check List

Tests

  • Unit test
  • Integration test

Related changes

  • Need to cherry-pick to the release branch

Release note

  • Support configuring both -F and -r arguments at the same time.

@kennytm
Copy link
Collaborator

kennytm commented Oct 29, 2020

tests/rows/run.sh: 57: printf: {1..10}: expected numeric value

Makefile:28: recipe for target 'integration_test' failed

DB: ir.DatabaseName(),
Table: ir.TableName(),
}
if bothEnabled {
o.id = 0
o.idFormat = fmt.Sprintf("%09d", ir.ChunkIndex()) + "%04d"

Choose a reason for hiding this comment

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

13 digits will overflow an int32. Do tools that read these files parse out the number, or just rely on it for filesystem ordering?

Copy link
Collaborator

Choose a reason for hiding this comment

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

so far all of myloader, Lightning and DM only rely on filesystem order, and won't parse the number. Lightning is the only one requiring that part to be [0-9]+ because the regex was written too tightly.

@@ -117,7 +117,7 @@ func (s *testDumpSuite) TestWriteTableData(c *C) {
err = writer.WriteTableData(ctx, tableIR)
c.Assert(err, IsNil)

p := path.Join(dir, "test.employee.0.sql")
p := path.Join(dir, "test.employee.000000000.sql")

Choose a reason for hiding this comment

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

Should there be a test that uses the 13 digit number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in ffcea13. The integration test in rows will also help check this.

v4/export/writer.go Outdated Show resolved Hide resolved
kennytm
kennytm previously approved these changes Nov 9, 2020
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.

LGTM.

Though those Index1 and Index2 could probably use more descriptive names 😉

@lichunzhu
Copy link
Contributor Author

lichunzhu commented Nov 9, 2020

LGTM.

Though those Index1 and Index2 could probably use more descriptive names 😉

Yes. I have revised Index1 to ChunkIndex and Index2 to FileIndex.

@kennytm
Copy link
Collaborator

kennytm commented Nov 9, 2020

(trigger CI)

@kennytm kennytm closed this Nov 9, 2020
@kennytm kennytm reopened this Nov 9, 2020
@kennytm kennytm merged commit 63c5a36 into pingcap:master Nov 9, 2020
@lichunzhu lichunzhu deleted the supportBothFilesizeAndRows branch November 9, 2020 07:31
tisonkun pushed a commit to tisonkun/dumpling that referenced this pull request Oct 20, 2021
* support both filesize and rows arguments

* fix bash

* add unit test for the situation that both filesize and rows are enabled

* address comment

* address comment
tisonkun pushed a commit to tisonkun/dumpling that referenced this pull request Oct 20, 2021
* support both filesize and rows arguments

* fix bash

* add unit test for the situation that both filesize and rows are enabled

* address comment

* address comment
tisonkun pushed a commit to tisonkun/dumpling that referenced this pull request Oct 20, 2021
* support both filesize and rows arguments

* fix bash

* add unit test for the situation that both filesize and rows are enabled

* address comment

* address comment
tisonkun pushed a commit to tisonkun/dumpling that referenced this pull request Oct 20, 2021
* support both filesize and rows arguments

* fix bash

* add unit test for the situation that both filesize and rows are enabled

* address comment

* address comment
tisonkun pushed a commit to tisonkun/tidb that referenced this pull request Oct 20, 2021
* support both filesize and rows arguments

* fix bash

* add unit test for the situation that both filesize and rows are enabled

* address comment

* address comment
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support -F even specified -r
3 participants