ARROW-17382: [C++] open_dataset doesn't ignore BOM in csv file when header's with quotes

[ZMZ91]

No description provided.

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - How to contribute patches

[github-actions]

https://issues.apache.org/jira/browse/ARROW-17382

[github-actions]

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

[ZMZ91]

Hi @pitrou, could you help review this pr?

[pitrou]

@ZMZ91 I could, but the first step would be to get the CI runs fixed.

In addition, since this claims to fix a bug, there should be a unit test added somewhere.

[ZMZ91]

Thanks @pitrou. I've pushed a new commit and still got 2 ci failures. I'm not sure it's related with my change. Could you help check?

[pitrou]

You are right, the failing CI checks are unrelated.

[pitrou]

@ZMZ91 There is already some logic in the CSV reader to skip the BOM:

arrow/cpp/src/arrow/csv/reader.cc

Lines 109 to 121 in fe9ca42

	Result<TransformFlow<std::shared_ptr<Buffer>>> operator()(std::shared_ptr<Buffer> buf) {
	if (buf == nullptr) {
	// EOF
	return TransformFinish();
	}
	int64_t offset = 0;
	if (first_buffer_) {
	ARROW_ASSIGN_OR_RAISE(auto data, util::SkipUTF8BOM(buf->data(), buf->size()));
	offset += data - buf->data();
	DCHECK_GE(offset, 0);
	first_buffer_ = false;
	}

Instead of adding the same logic in the CSV parser, you should instead try to find out what that logic (in the CSV reader) isn't sufficient here.

[ZMZ91]

I saw the code in reader.cc @pitrou. But it seems work on the entire csv file while the parser only takes a block of csv data and delimits rows and fields. And the data for each function are read respectively. Correct me if I'm not right.

[pitrou]

That is why your approach is wrong: the BOM is only expected at the beginning of the file, not at the beginning of each CSV cell.

[pitrou]

Thanks @ZMZ91 ! LGTM, will merge if CI is ok.

[pitrou]

CI failures are unrelated.

[ursabot]

Benchmark runs are scheduled for baseline = 8bf60b5 and contender = 01dce6a. 01dce6a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.18% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 01dce6a0 ec2-t3-xlarge-us-east-2
[Finished] 01dce6a0 test-mac-arm
[Failed] 01dce6a0 ursa-i9-9960x
[Finished] 01dce6a0 ursa-thinkcentre-m75q
[Finished] 8bf60b5d ec2-t3-xlarge-us-east-2
[Failed] 8bf60b5d test-mac-arm
[Failed] 8bf60b5d ursa-i9-9960x
[Finished] 8bf60b5d ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java