lighting/parser: align NULL and ESCAPED BY with LOAD DATA #40909

lance6716 · 2023-01-31T11:33:55Z

Signed-off-by: lance6716 lance6716@gmail.com

What problem does this PR solve?

Issue Number: ref #40499

Problem Summary:

What is changed and how it works?

ESCAPED BY: add a new configuration to alter the escape character from \
NULL: the full behaviour should work together with caller. If the ESCAPED BY is set (for example \), \N should be treated as NULL. If ENCLOSED BY is set (for example "), unenclosed NULL in data file should be treated as NULL. Modify the code so caller should set \N, \NULL in the configuration

Also fix a bug in ReadUntilTerminator.

Check List

Tests

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

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot · 2023-01-31T11:33:57Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

buchuitoudegou
gozssky

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

lance6716 · 2023-01-31T11:34:13Z

/cc @gozssky @dsdashun

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2023-01-31T12:31:32Z

/retest

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2023-01-31T12:57:56Z

br/pkg/lightning/mydump/csv_parser_test.go

 		},
 	}

 	testCases := []testCase{
 		{
 			input:    `\\`,
-			expected: [][]types.Datum{{nullDatum}},
+			expected: [][]types.Datum{{types.NewStringDatum("")}},


note that the behaviour is changed. empty field inside delimiter is no longer NULL

Oh I just noticed we have explicitly stated the behaviour in doc

Quoting does not affect whether a field is null.

https://docs.pingcap.com/tidb/stable/tidb-lightning-data-source#not-null-and-null

I should add a hide configuration for LOAD DATA 😂

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2023-02-01T01:40:24Z

/run-integration-br-tests

lance6716 · 2023-02-01T02:09:47Z

BR CI passed, ptal @gozssky @dsdashun @D3Hunter

https://ci.pingcap.net/blue/organizations/jenkins/tidb_ghpr_integration_br_test/detail/tidb_ghpr_integration_br_test/11813/pipeline

lance6716 · 2023-02-01T08:20:26Z

/cc @buchuitoudegou @lichunzhu

sleepymole · 2023-02-07T02:18:31Z

br/pkg/lightning/mydump/parser.go

@@ -303,20 +303,22 @@ func (parser *blockParser) readBlock() error {
 	}
 }

-var unescapeRegexp = regexp.MustCompile(`(?s)\\.`)
+var chunkParserUnescapeRegexp = regexp.MustCompile(`(?s)\\.`)


Almost LGTM. I don't quite understand why this regexp has been preserved and will review it later.

ti-chi-bot · 2023-02-12T06:39:00Z

@lance6716: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lance6716 · 2023-02-13T02:07:33Z

ping

buchuitoudegou · 2023-02-13T06:36:22Z

br/pkg/lightning/config/config.go

+		csv.EscapedBy = `\`
+	}
+	if !csv.BackslashEscape && csv.EscapedBy == `\` {
+		csv.EscapedBy = ""


Why is csv.EscapedBy set to empty if it is set in the config but backslashEscape is false?

Currently the default value of EscapedBy is \, and BackslashEscape is hidden and has default value true. So if BackslashEscape is changed to false, it means this is an old format configuration file with false BackslashEscape, we should disable escaping

br/pkg/lightning/config/config.go

br/pkg/lightning/mydump/csv_parser.go

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 · 2023-02-13T09:01:29Z

/merge

ti-chi-bot · 2023-02-13T09:01:33Z

This pull request has been accepted and is ready to merge.

Commit hash: 79f2d6b

lance6716 · 2023-02-13T09:52:59Z

/merge

lance6716 · 2023-02-13T11:03:07Z

/retest

lance6716 · 2023-02-13T12:07:39Z

/retest

lance6716 · 2023-02-13T13:31:33Z

/test all

lance6716 · 2023-02-13T14:52:24Z

/retest

) ref pingcap#40499

lighting/parser: align NULL and ESCAPED BY with LOAD DATA

4d87ac6

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot added release-note-none size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 31, 2023

ti-chi-bot requested review from dsdashun and sleepymole January 31, 2023 11:34

lance6716 added 2 commits January 31, 2023 19:40

fix lint

dc74f56

Signed-off-by: lance6716 <lance6716@gmail.com>

fix make bazel

b49f717

Signed-off-by: lance6716 <lance6716@gmail.com>

fix some UT

1b19a33

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716 commented Jan 31, 2023

View reviewed changes

lance6716 added 4 commits January 31, 2023 21:14

fix UT

c7dde83

Signed-off-by: lance6716 <lance6716@gmail.com>

add more UT

3e6624f

Signed-off-by: lance6716 <lance6716@gmail.com>

keep compatibility

d242a67

Signed-off-by: lance6716 <lance6716@gmail.com>

fix more UT

a6cf286

Signed-off-by: lance6716 <lance6716@gmail.com>

ti-chi-bot requested review from buchuitoudegou and lichunzhu February 1, 2023 08:20

sleepymole reviewed Feb 7, 2023

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2023

buchuitoudegou reviewed Feb 13, 2023

View reviewed changes

sleepymole reviewed Feb 13, 2023

View reviewed changes

br/pkg/lightning/config/config.go Outdated Show resolved Hide resolved

br/pkg/lightning/mydump/csv_parser.go Show resolved Hide resolved

br/pkg/lightning/mydump/csv_parser.go Show resolved Hide resolved

lance6716 added 3 commits February 13, 2023 16:12

address comment

51e8c1d

Signed-off-by: lance6716 <lance6716@gmail.com>

Merge branch 'master' of github.com:pingcap/tidb into csv-parser-2

2e9da99

Signed-off-by: lance6716 <lance6716@gmail.com>

fix wrongly paste

79f2d6b

Signed-off-by: lance6716 <lance6716@gmail.com>

buchuitoudegou approved these changes Feb 13, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 13, 2023

sleepymole approved these changes Feb 13, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 13, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 13, 2023

lance6716 removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2023

ti-chi-bot merged commit 55c8358 into pingcap:master Feb 13, 2023

blacktear23 pushed a commit to blacktear23/tidb that referenced this pull request Feb 15, 2023

lighting/parser: align NULL and ESCAPED BY with LOAD DATA (pingcap#40909

302c3a2

) ref pingcap#40499

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lighting/parser: align NULL and ESCAPED BY with LOAD DATA #40909

lighting/parser: align NULL and ESCAPED BY with LOAD DATA #40909

lance6716 commented Jan 31, 2023 •

edited

ti-chi-bot commented Jan 31, 2023 •

edited

lance6716 commented Jan 31, 2023

lance6716 commented Jan 31, 2023

lance6716 Jan 31, 2023

lance6716 Jan 31, 2023

lance6716 commented Feb 1, 2023

lance6716 commented Feb 1, 2023

lance6716 commented Feb 1, 2023

sleepymole Feb 7, 2023

ti-chi-bot commented Feb 12, 2023

lance6716 commented Feb 13, 2023

buchuitoudegou Feb 13, 2023

lance6716 Feb 13, 2023

buchuitoudegou Feb 13, 2023

lance6716 commented Feb 13, 2023

ti-chi-bot commented Feb 13, 2023

lance6716 commented Feb 13, 2023

lance6716 commented Feb 13, 2023

lance6716 commented Feb 13, 2023

lance6716 commented Feb 13, 2023

lance6716 commented Feb 13, 2023

lighting/parser: align NULL and ESCAPED BY with LOAD DATA #40909

lighting/parser: align NULL and ESCAPED BY with LOAD DATA #40909

Conversation

lance6716 commented Jan 31, 2023 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jan 31, 2023 • edited

lance6716 commented Jan 31, 2023

lance6716 commented Jan 31, 2023

lance6716 Jan 31, 2023

Choose a reason for hiding this comment

lance6716 Jan 31, 2023

Choose a reason for hiding this comment

lance6716 commented Feb 1, 2023

lance6716 commented Feb 1, 2023

lance6716 commented Feb 1, 2023

sleepymole Feb 7, 2023

Choose a reason for hiding this comment

ti-chi-bot commented Feb 12, 2023

lance6716 commented Feb 13, 2023

buchuitoudegou Feb 13, 2023

Choose a reason for hiding this comment

lance6716 Feb 13, 2023

Choose a reason for hiding this comment

buchuitoudegou Feb 13, 2023

Choose a reason for hiding this comment

lance6716 commented Feb 13, 2023

ti-chi-bot commented Feb 13, 2023

lance6716 commented Feb 13, 2023

lance6716 commented Feb 13, 2023

lance6716 commented Feb 13, 2023

lance6716 commented Feb 13, 2023

lance6716 commented Feb 13, 2023

lance6716 commented Jan 31, 2023 •

edited

ti-chi-bot commented Jan 31, 2023 •

edited