feat(pkg/csv2lp): add csv to line protocol conversion library #17764

sranka · 2020-04-16T13:21:00Z

#17004 requests CSV support in influx write command, #17599 requires a separate library for CSV to line protocol conversion that shall be then used in influx write. This PR introduces the library.

README.md describes this library and contains examples.

CHANGELOG.md updated with a link to the PR (not the Issue)
Well-formatted commit messages
Rebased/mergeable
Tests pass
Documentation updated or issue created (provide link to issue/pr)
Signed CLA (if not already signed)

sranka · 2020-04-16T13:24:08Z

@jsteenb2 can you please help with the stakeholders to review this PR

jsteenb2 · 2020-04-16T17:01:26Z

@sranka I'd like to see storage folks or someone who is more LP saavy give this a read. I pulled someone in from the auto assigner for the storage team.

sebito91 · 2020-04-22T20:28:03Z

Hey folks, I'm looking into this today!

sebito91

This is an awesome addition to the library, thanks @sranka! There are a few things to clear up here in terms of composition and clarification. Also, I would really like to see you include a docs update similar to what you've done here. Add that as a README into this package as it would make things a lot clearer!

Also, apart from the other naming please go through an move your filenames from camelCase to snake_case, so csvTable.go becomes csv_table.go, etc.

Lastly, all test names should be of the format Test_ThingToTestInCamelCase. You have a few references to the actual function you're testing under the covers, like Test_escapeMeasurement that should become Test_EscapeMeasurement.

Other than that, thank you again so very much for your contribution! If you have any questions I'm lurking both on the gophers.slack.com and influxcommunity.slack.com under sebito91.

pkg/csv2lp/csvAnnotations.go

pkg/csv2lp/csvTable.go

pkg/csv2lp/csvToProtocolLines.go

pkg/csv2lp/dataConversion.go

sranka

Thank you @sebito91 for a detailed code review, I fixed the naming conventions of files/tests, added documentation, and overall improved the code to address your feedback. I replied to all your comments and left them up to you to mark them as resolved. Some comments/conversations are still open, I would appreciate your feedback therein.

sranka · 2020-04-28T15:00:32Z

pkg/csv2lp/csvAnnotations.go

+	setupTable func(table *CsvTable, row []string) error
+}
+
+func (a *annotationComment) isTableAnnotation() bool {


sranka · 2020-04-28T15:05:46Z

pkg/csv2lp/csvAnnotations.go

+	return strings.HasPrefix(strings.ToLower(comment), a.prefix)
+}
+
+var supportedAnnotations = []annotationComment{


annotationComment is never updated, there is IMHO no need to have pointers herein; there are a few annotation comments (by design) that don't change

pkg/csv2lp/csvAnnotations.go

pkg/csv2lp/dataConversion.go

sranka · 2020-04-28T20:33:22Z

pkg/csv2lp/csvTable.go

+			errors.New("no measurement supplied"),
+		}
+	}
+	buffer = append(buffer, escapeMeasurement(measurement)...)


I also use appendConverted fn to append field values or various data types, the appendConverted internally uses strConv.append* functions that accept and return []byte. A change to bytes.Buffer would then require (on the other hand) to create temporary objects for every float, int, uint value that I don't need now. I am not sure that the situation gets better after this change. So it is trade-off situation herein. I would rather keep it the way it is.

I wish to have append optimized by the compiler or bytes.Buffer also accepting primitive types for writing.

sranka · 2020-04-28T20:36:22Z

pkg/csv2lp/csvTable.go

+			}
+		}
+	}
+	return buffer, nil


yes, or I could simplify the function signature to use *bytes.Buffer

sebito91 · 2020-04-29T20:31:00Z

Excellent work @sranka, will review this and get back to you today!

sebito91

Some small changes this time but we're getting closer, thanks so much for your patience @sranka !!

pkg/csv2lp/README.md

pkg/csv2lp/csv2lp.go

pkg/csv2lp/csv_annotations.go

sebito91 · 2020-05-12T15:44:01Z

pkg/csv2lp/multi_closer.go

@@ -0,0 +1,33 @@
+package csv2lp


Is this file actually used anywhere? If not we should remove...

It is not used internally by the library. It is referenced in README.MD (Support Existing CSV files) since it helps compose a single input reader out of multiple Readers and ReadClosers.

csv2lp.MultiCloser helps with closing multiple io.Closers (files) on input, because it is not available OOTB

sranka

thank you @sebito91 for review, it is now ready for the next review

pkg/csv2lp/README.md

pkg/csv2lp/csv2lp.go

pkg/csv2lp/csv_annotations.go

sranka · 2020-05-12T19:12:19Z

pkg/csv2lp/multi_closer.go

@@ -0,0 +1,33 @@
+package csv2lp


It is not used internally by the library. It is referenced in README.MD (Support Existing CSV files) since it helps compose a single input reader out of multiple Readers and ReadClosers.

csv2lp.MultiCloser helps with closing multiple io.Closers (files) on input, because it is not available OOTB

sebito91 · 2020-05-12T19:22:16Z

I think this build requires a go mod tidy before it'll pass integration tests. If you can make those changes I'll ack the PR and we're good to merge!

sranka · 2020-05-12T19:41:51Z

I would appreciate any insight or explanation regarding the failing tests that are (from time to time) causing unintended interferences and testing failures. @sebito91 I rebased this branch to the latest master, it usually helps.

sebito91 · 2020-05-12T19:51:39Z

That'll usually do it, but if you're importing new packages you'll need to run go mod vendor and go mod tidy to ensure the modules files are updated accordingly.

For example after bringing your branch down into my local repo, it looks like go.mod is missing a reference to testify:

(base)  [sborza@icebox]:~/src/github.com/influxdata/influxdb (master-1.x *%):$ git diff go.mod
diff --git a/go.mod b/go.mod
index cd2e3407f4..c702f309d2 100644
--- a/go.mod
+++ b/go.mod
@@ -40,6 +40,7 @@ require (
        github.com/segmentio/kafka-go v0.2.0 // indirect
        github.com/smartystreets/goconvey v1.6.4 // indirect
        github.com/spf13/cast v1.3.0
+       github.com/stretchr/testify v1.4.0
        github.com/tinylib/msgp v1.0.2
        github.com/willf/bitset v1.1.3 // indirect
        github.com/xlab/treeprint v0.0.0-20180616005107-d6fb6747feb6

sebito91

LGTM, well done!

chore(CHANGELOG): update to include recently merged PR #17764

sranka requested a review from jsteenb2 April 16, 2020 13:21

jsteenb2 requested review from a team and sebito91 and removed request for a team April 16, 2020 17:00

sebito91 suggested changes Apr 27, 2020

View reviewed changes

sranka commented Apr 28, 2020

View reviewed changes

sranka force-pushed the 17004/csv2lp branch from e21ad5b to 24e37b8 Compare April 28, 2020 20:52

sranka requested a review from sebito91 April 28, 2020 20:52

sebito91 suggested changes May 12, 2020

View reviewed changes

sranka commented May 12, 2020

View reviewed changes

sranka requested a review from sebito91 May 12, 2020 19:17

sranka added 13 commits May 12, 2020 21:32

feat(pkg/csv2lp): add csv to line protocol conversion library

234eac8

chore(pkg/csv2lp): add README.md

786f06b

chore(pkg/csv2lp): use snake_case

476acc4

chore(pkg/csv2lp): document tests

6e5aca1

chore(pkg/csv2lp): apply code-review comments

8cdde7a

chore: improve doc

94da005

feat(pkg/csv2lp): add MultiCloser

bfba348

feat(pkg/csv2lp): add SkipHeaderLinesReader

8dcbd15

feat(pkg/csv2lp): improve README.md

584ade5

feat(pkg/csv2lp): support quoted new lines in SkipHeaderLinesReader

010f23f

chore(pkg/csv2lp): rename CsvToProtocolLines to CsvToLineProtocol

d12d6de

chore(pkg/csv2lp): clarify README.md

9699352

chore(pkg/csv2lp): repair doc

7af307d

sranka force-pushed the 17004/csv2lp branch from 8e5539f to 7af307d Compare May 12, 2020 19:32

sebito91 approved these changes May 12, 2020

View reviewed changes

sebito91 merged commit 01b00da into master May 12, 2020

sebito91 added a commit that referenced this pull request May 12, 2020

chore(CHANGELOG): update to include recently merged PR #17764

8e5b76b

sebito91 mentioned this pull request May 12, 2020

chore(CHANGELOG): update to include recently merged PR #17764 #18067

Merged

sebito91 added a commit that referenced this pull request May 12, 2020

Merge pull request #18067 from influxdata/update_changelog

2fb92a1

chore(CHANGELOG): update to include recently merged PR #17764

sranka deleted the 17004/csv2lp branch May 13, 2020 05:36

sranka mentioned this pull request May 14, 2020

feat(cmd/influx/write): enhance the ability to import CSV files using csv2lp library #18093

Merged

6 tasks

sranka linked an issue May 14, 2020 that may be closed by this pull request

Add the ability to import a simple csv file via the influx cli #17004

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pkg/csv2lp): add csv to line protocol conversion library #17764

feat(pkg/csv2lp): add csv to line protocol conversion library #17764

sranka commented Apr 16, 2020 •

edited

Loading

sranka commented Apr 16, 2020

jsteenb2 commented Apr 16, 2020

sebito91 commented Apr 22, 2020

sebito91 left a comment

sranka left a comment

sranka Apr 28, 2020

sranka Apr 28, 2020

sranka Apr 28, 2020

sranka Apr 28, 2020

sebito91 commented Apr 29, 2020

sebito91 left a comment

sebito91 May 12, 2020

sranka May 12, 2020 •

edited

Loading

sranka left a comment

sranka May 12, 2020 •

edited

Loading

sebito91 commented May 12, 2020

sranka commented May 12, 2020

sebito91 commented May 12, 2020

sebito91 left a comment

feat(pkg/csv2lp): add csv to line protocol conversion library #17764

feat(pkg/csv2lp): add csv to line protocol conversion library #17764

Conversation

sranka commented Apr 16, 2020 • edited Loading

sranka commented Apr 16, 2020

jsteenb2 commented Apr 16, 2020

sebito91 commented Apr 22, 2020

sebito91 left a comment

Choose a reason for hiding this comment

sranka left a comment

Choose a reason for hiding this comment

sranka Apr 28, 2020

Choose a reason for hiding this comment

sranka Apr 28, 2020

Choose a reason for hiding this comment

sranka Apr 28, 2020

Choose a reason for hiding this comment

sranka Apr 28, 2020

Choose a reason for hiding this comment

sebito91 commented Apr 29, 2020

sebito91 left a comment

Choose a reason for hiding this comment

sebito91 May 12, 2020

Choose a reason for hiding this comment

sranka May 12, 2020 • edited Loading

Choose a reason for hiding this comment

sranka left a comment

Choose a reason for hiding this comment

sranka May 12, 2020 • edited Loading

Choose a reason for hiding this comment

sebito91 commented May 12, 2020

sranka commented May 12, 2020

sebito91 commented May 12, 2020

sebito91 left a comment

Choose a reason for hiding this comment

sranka commented Apr 16, 2020 •

edited

Loading

sranka May 12, 2020 •

edited

Loading

sranka May 12, 2020 •

edited

Loading