fix(parsers.csv): Allow null-delimiters #12247

Meceron · 2022-11-16T09:16:38Z

https://github.com/influxdata/telegraf/issues/12243

Updated associated README.md.
Wrote appropriate unit tests.
Pull request title or commits are in conventional commit format

resolves #12243

Fixing null delimiters in csv, change the selected bytes before parsing to pipeline.

…parsing to pipeline

powersj · 2022-11-16T14:59:02Z

Hi,

Thanks for putting up this PR. It appears that this is essentially a "find and replace" before doing any parsing. Instead of having this hard-code the replacement, can we instead rename csv_force_replace_delimiter to csv_delimiter_replace and use the csv_delimiter as the replacement?

Essentially if I had:

csv_delimiter = ","
csv_delimiter_replace = "\u0000"

It would replace those null bytes with commas.

What do you think about that? Before you go off and make changes I would like to hear from @srebhan as well

srebhan · 2022-11-16T15:54:12Z

@Meceron thanks for your PR! Instead of forcing the user to give another option how about the following (in pseudo-code):

Check if delimiter is valid, i.e. copy the validDelim function of encoding/csv
If invalid:
2.1 replace all comma (, ) with \ufffd which is the unicode replacement character and should not appear in the text
2.2 replace all user-specified delimiter instances by comma
2.3 change CSV delimiter to comma
2.4 set a flag that we replaced data e.g. p.delimiterReplaced = true
do the csv parsing
If p.delimiterReplaced == true replace \ufffd in all resulting fields back to comma

What do you think?

Meceron · 2022-11-17T10:17:31Z

Thanks for the advice! I've changed code.

powersj

Thanks for the quick turn around! I have put up some comments that clarify the docs and rename the bool to invalidDelimiter.

Take a look and let us know!

plugins/parsers/csv/README.md

plugins/parsers/csv/parser.go

srebhan · 2022-12-08T14:52:26Z

@Meceron any news on this PR?

Hipska

Whoops, I didn't finished my review..

Hipska · 2022-11-22T11:07:08Z

plugins/parsers/csv/parser.go

+var replacementByte = "\ufffd"
+var commaByte = "\u002C"
+


Maybe const instead of var and directly a byte array so no conversion is needed on usage?

Hipska · 2022-12-08T15:07:00Z

plugins/parsers/csv/parser_test.go

+	err := p.Init()
+	require.NoError(t, err)
+
+	testCSV := strings.Join([]string{"3.4", "70", "test_name"}, "\u0000")


Suggested change

testCSV := strings.Join([]string{"3.4", "70", "test_name"}, "\u0000")

testCSV := "3.4\u000070\u0000test_name"

I prefer the existing as it makes it very clear what is going on versus having to count and determine the end of a unicode value

Meceron · 2022-12-09T16:03:44Z

Sorry , I was on sickleave :(. As I see You All done everything, thank you!

srebhan

I only do have two small comments @Meceron. Can you please address those and consider the other comment of @Hipska!?

plugins/parsers/csv/parser.go

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

powersj · 2022-12-12T15:28:43Z

I wanted to try to land this for today's release, so I accepted the changes.

telegraf-tiger · 2022-12-12T15:47:12Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

🥳 This pull request decreases the Telegraf binary size by -1.35 % for linux amd64 (new size: 157.2 MB, nightly size 159.3 MB)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_arm64.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz
		static_linux_amd64.tar.gz

Hipska · 2022-12-12T16:04:35Z

@powersj my comments are still open and un-addressed..

srebhan · 2022-12-12T16:37:50Z

@Meceron can you please at least make those two strings const?

Hipska · 2022-12-12T16:40:59Z

@srebhan This is what @powersj already did in a315d23.

powersj · 2022-12-12T16:41:02Z

@Meceron can you please at least make those two strings const?

This was done already. I accepted your change that did this.

edit: what Hipska said :)

srebhan · 2022-12-12T16:50:52Z

Oh didn't see a commit here... Sorry, I should better take a look at the code... :-/

srebhan

Thanks for the nice improvement @Meceron! LGTM.

powersj · 2022-12-12T16:55:19Z

Merging as integration test is a known issue fixed on master

fix: fixing null delimiters in csv, change the selected bytes before …

e0c7a64

…parsing to pipeline

telegraf-tiger bot added the fix pr to fix corresponding bug label Nov 16, 2022

run go fmt

82f1f11

advised changes, readme updated, logic changed

471b64f

powersj self-assigned this Nov 17, 2022

powersj reviewed Nov 17, 2022

View reviewed changes

Hipska suggested changes Dec 8, 2022

View reviewed changes

powersj added 7 commits December 9, 2022 07:41

Update plugins/parsers/csv/README.md

e704860

Update plugins/parsers/csv/parser.go

9d1d64d

Update plugins/parsers/csv/parser.go

1c6e69b

Update plugins/parsers/csv/parser.go

2dad52e

Update plugins/parsers/csv/parser.go

fb9c75d

Update plugins/parsers/csv/parser.go

08a415d

Update plugins/parsers/csv/parser.go

d806567

powersj assigned srebhan Dec 9, 2022

powersj added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Dec 9, 2022

powersj approved these changes Dec 9, 2022

View reviewed changes

srebhan reviewed Dec 9, 2022

View reviewed changes

plugins/parsers/csv/parser.go Outdated Show resolved Hide resolved

plugins/parsers/csv/parser.go Show resolved Hide resolved

powersj and others added 2 commits December 12, 2022 08:28

Update plugins/parsers/csv/parser.go

a315d23

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

Update plugins/parsers/csv/parser.go

613de49

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

srebhan approved these changes Dec 12, 2022

View reviewed changes

srebhan added plugin/parser 1. Request for new parser plugins 2. Issues/PRs that are related to parser plugins area/csv csv parser/serialiser related labels Dec 12, 2022

powersj approved these changes Dec 12, 2022

View reviewed changes

srebhan changed the title ~~fix: fixing null delimiters in csv, change the selected bytes before …~~ fix(parsers.csv): Allow null-delimiters Dec 12, 2022

powersj merged commit e264721 into influxdata:master Dec 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(parsers.csv): Allow null-delimiters #12247

fix(parsers.csv): Allow null-delimiters #12247

Meceron commented Nov 16, 2022 •

edited by srebhan

Loading

powersj commented Nov 16, 2022

srebhan commented Nov 16, 2022 •

edited

Loading

Meceron commented Nov 17, 2022

powersj left a comment

srebhan commented Dec 8, 2022

Hipska left a comment

Hipska Nov 22, 2022

Hipska Dec 8, 2022

powersj Dec 12, 2022

Meceron commented Dec 9, 2022

srebhan left a comment

powersj commented Dec 12, 2022

telegraf-tiger bot commented Dec 12, 2022

Artifact URLs

Hipska commented Dec 12, 2022

srebhan commented Dec 12, 2022

Hipska commented Dec 12, 2022

powersj commented Dec 12, 2022 •

edited

Loading

srebhan commented Dec 12, 2022

srebhan left a comment

powersj commented Dec 12, 2022

	testCSV := strings.Join([]string{"3.4", "70", "test_name"}, "\u0000")
	testCSV := "3.4\u000070\u0000test_name"

fix(parsers.csv): Allow null-delimiters #12247

fix(parsers.csv): Allow null-delimiters #12247

Conversation

Meceron commented Nov 16, 2022 • edited by srebhan Loading

powersj commented Nov 16, 2022

srebhan commented Nov 16, 2022 • edited Loading

Meceron commented Nov 17, 2022

powersj left a comment

Choose a reason for hiding this comment

srebhan commented Dec 8, 2022

Hipska left a comment

Choose a reason for hiding this comment

Hipska Nov 22, 2022

Choose a reason for hiding this comment

Hipska Dec 8, 2022

Choose a reason for hiding this comment

powersj Dec 12, 2022

Choose a reason for hiding this comment

Meceron commented Dec 9, 2022

srebhan left a comment

Choose a reason for hiding this comment

powersj commented Dec 12, 2022

telegraf-tiger bot commented Dec 12, 2022

Artifact URLs

Hipska commented Dec 12, 2022

srebhan commented Dec 12, 2022

Hipska commented Dec 12, 2022

powersj commented Dec 12, 2022 • edited Loading

srebhan commented Dec 12, 2022

srebhan left a comment

Choose a reason for hiding this comment

powersj commented Dec 12, 2022

Meceron commented Nov 16, 2022 •

edited by srebhan

Loading

srebhan commented Nov 16, 2022 •

edited

Loading

powersj commented Dec 12, 2022 •

edited

Loading