Skip to content

ARROW-10747: [Rust]: CSV reader optimization - #8781

Closed
Dandandan wants to merge 22 commits into
apache:masterfrom
Dandandan:csv_opt
Closed

ARROW-10747: [Rust]: CSV reader optimization#8781
Dandandan wants to merge 22 commits into
apache:masterfrom
Dandandan:csv_opt

Conversation

@Dandandan

@DandandanDandandan commented Nov 26, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

this PR makes CSV reading (quite a bit) faster by reusing allocations, and doing things a bit more manually.
It removes usage of BufReader, which is done in rust-csv already and causes overhead.

The nytaxi (entire job, with reading 1 year csv) benchmark speeds up from ~4500ms to ~1900ms.
Loading the line item csv in memory for the tpch benchmark for goes from ~9800ms -> ~6000 ms.

I think a further optimization would be to stop using the StringRecords altogether (e.g. by using the underlying https://docs.rs/csv-core/0.1.10/csv_core/ library instead) but that could be a next step.

FYI @alamb@nevi-me@jorgecarleitao

@github-actions

github-actionsBot commented Nov 26, 2020

Copy link
Copy Markdown

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

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

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

See also:

@DandandanDandandan changed the title ARROW-10747: [DataFusion]: WIP CSV reader optimization ARROW-10747: [Rust]: WIP CSV reader optimizationNov 26, 2020
@DandandanDandandan changed the title ARROW-10747: [Rust]: WIP CSV reader optimizationARROW-10747: [Rust]: WIP CSV reader optimizationNov 26, 2020
@github-actions

github-actionsBot commented Nov 26, 2020

Copy link
Copy Markdown

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

@DandandanDandandan changed the title ARROW-10747: [Rust]: WIP CSV reader optimizationARROW-10747: [Rust]: CSV reader optimizationNov 26, 2020
@Dandandan

Dandandan commented Nov 26, 2020

Copy link
Copy Markdown
ContributorAuthor

I found some further opportunities for optimizing by also reusing the stringrecord items, for another speed up.

@Dandandan

Dandandan commented Nov 26, 2020

Copy link
Copy Markdown
ContributorAuthor

Is ready for review now.

@DandandanDandandan mentioned this pull request Nov 27, 2020
Closed
jorgecarleitao
jorgecarleitao approved these changes Nov 27, 2020
View reviewed changes

@jorgecarleitaojorgecarleitao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice work, @Dandandan , really cool speedup for such an important op.

nevi-me
nevi-me approved these changes Nov 27, 2020
View reviewed changes

@nevi-menevi-me left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment threadrust/benchmarks/src/bin/nyctaxi.rs Outdated
@andygrove

andygrove commented Nov 27, 2020

Copy link
Copy Markdown
Member

@Dandandan I'm not sure why the Windows build failed and I assume it is unrelated but the logs are not available. Could you push an empty commit to trigger CI again?

@Dandandan

Dandandan commented Nov 27, 2020

Copy link
Copy Markdown
ContributorAuthor

@Dandandan I'm not sure why the Windows build failed and I assume it is unrelated but the logs are not available. Could you push an empty commit to trigger CI again?

Just did. Let's see what happens!

@nevi-me

nevi-me commented Nov 27, 2020

Copy link
Copy Markdown
Contributor

Could you push an empty commit to trigger CI again?

We can rerun failed CI jobs from the UI, which is often better as it doesn't trigger AppVeyor and Travis CI

@Dandandan

Dandandan commented Nov 27, 2020

Copy link
Copy Markdown
ContributorAuthor

Now some other jobs failed. Maybe we can rerun those?

@nevi-me

nevi-me commented Nov 27, 2020

Copy link
Copy Markdown
Contributor

Now some other jobs failed. Maybe we can rerun those?

CI seems to be misbehaving, it's not letting me cancel the workflow, even though the tests have failed. I'll leave this tab open, and retry the Rust jobs in about an hour. I'll merge this after CI passes

@Dandandan

Dandandan commented Nov 28, 2020

Copy link
Copy Markdown
ContributorAuthor

I also removed the now unused buffered iterator as it is unused by now, and I think will not lead to efficient code in general.

alamb
alamb reviewed Nov 30, 2020
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry I didn't get a chance to review this carefully before merge. It looks nice to me. Nice work @Dandandan

Comment threadrust/benchmarks/src/bin/nyctaxi.rs
) -> Result<()> {
let mut queries = HashMap::new();
queries.insert("fare_amt_by_passenger", "SELECT passenger_count, MIN(fare_amount), MIN(fare_amount), SUM(fare_amount) FROM tripdata GROUP BY passenger_count");
queries.insert("fare_amt_by_passenger", "SELECT passenger_count, MIN(fare_amount), MAX(fare_amount), SUM(fare_amount) FROM tripdata GROUP BY passenger_count");

@alambalambNov 30, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@jorgecarleitaojorgecarleitao mentioned this pull request Feb 17, 2021
Closed
@asfimportasfimport mentioned this pull request Dec 3, 2020
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alambalambalamb left review comments
@nevi-menevi-menevi-me approved these changes
@jorgecarleitaojorgecarleitaojorgecarleitao approved these changes

Assignees

No one assigned

Labels

Component: Rust

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Dandandan@andygrove@nevi-me@alamb@jorgecarleitao