ARROW-10654: [Rust] Specialize parsing of floats / bools in CSV Reader #8714

Dandandan · 2020-11-19T11:21:01Z

Internal rust float parser is known to be slow.

This change allows to have specialized implementations rather than relying on FromStr::parse.

Also avoids calling to_lowercase for booleans.

Would be nice to benchmark this.

Dandandan · 2020-11-19T11:22:33Z

Some benchmark/context of string -> f64 is here (note: log scale) https://github.com/Alexhuszagh/rust-lexical/

github-actions · 2020-11-19T11:33:02Z

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

Dandandan · 2020-11-19T16:34:54Z

Did some benchmarking on this. Seems like a small win.

Master:

Running benchmarks with the following options: Opt { debug: false, iterations: 10, concurrency: 1, batch_size: 4096, path: "./yellow_tripdata_2020-01.csv", file_format: "csv" }

Query 'fare_amt_by_passenger' iteration 0 took 4114 ms
Query 'fare_amt_by_passenger' iteration 1 took 4087 ms
Query 'fare_amt_by_passenger' iteration 2 took 4094 ms
Query 'fare_amt_by_passenger' iteration 3 took 4118 ms
Query 'fare_amt_by_passenger' iteration 4 took 4091 ms
Query 'fare_amt_by_passenger' iteration 5 took 4099 ms
Query 'fare_amt_by_passenger' iteration 6 took 4115 ms
Query 'fare_amt_by_passenger' iteration 7 took 4129 ms
Query 'fare_amt_by_passenger' iteration 8 took 4105 ms
Query 'fare_amt_by_passenger' iteration 9 took 4095 ms

This version:

Running benchmarks with the following options: Opt { debug: false, iterations: 10, concurrency: 1, batch_size: 4096, path: "./yellow_tripdata_2020-01.csv", file_format: "csv" }
Executing 'fare_amt_by_passenger'
Query 'fare_amt_by_passenger' iteration 0 took 3985 ms
Query 'fare_amt_by_passenger' iteration 1 took 3954 ms
Query 'fare_amt_by_passenger' iteration 2 took 3961 ms
Query 'fare_amt_by_passenger' iteration 3 took 3959 ms
Query 'fare_amt_by_passenger' iteration 4 took 3963 ms
Query 'fare_amt_by_passenger' iteration 5 took 3963 ms
Query 'fare_amt_by_passenger' iteration 6 took 3958 ms
Query 'fare_amt_by_passenger' iteration 7 took 3959 ms
Query 'fare_amt_by_passenger' iteration 8 took 3971 ms
Query 'fare_amt_by_passenger' iteration 9 took 3977 ms

nevi-me

LGTM

I've tried using lexical before, and I'm happy that it's faster than the stdlib

nevi-me · 2020-11-21T08:24:49Z

rust/arrow/src/csv/reader.rs

+    }
+}
+
+impl Parser for UInt64Type {}


Would there be plans to support these at all?

Currently they are supported by passing a schema to the csv reader? And here they keep using the standard string.parse::<T> method.

Perhaps @nevi-me was asking if there are plans to improve the parsing performance of these types as well

Ah. It could special case on those as well. Difference between lexical core and standard lib don't seem too big here though

yeah, I agree there is no reason to special case other types at this time

alamb · 2020-11-21T13:43:36Z

rust/arrow/src/csv/reader.rs

+    }
+}
+
+impl Parser for UInt64Type {}


Perhaps @nevi-me was asking if there are plans to improve the parsing performance of these types as well

alamb · 2020-11-21T13:51:14Z

rust/arrow/src/csv/reader.rs

+}
+
+impl Parser for BooleanType {
+    fn parse(string: &str) -> Option<bool> {


I wondered how this related to the rust standard boolean parsing: https://doc.rust-lang.org/src/core/str/traits.rs.html#590

Seems like it anything it would be slightly slower, but also support mixed case (true and True). Seems like a good improvement to me, though adding a test to encode the expected behavior would probably be a good idea.

Added https://issues.apache.org/jira/browse/ARROW-10677 to track extra parsing tests

Yes, the mixed case is to keep compatibility with the previous implementation (it used to_lower)

Could as well have specific cases for all caps / capitalized booleans instead maybe? That would avoid accepting tRue etc.

I think the code in this PR is good as is -- I just think it would be nice to document what the behavior is more explicitly -- so I did so in #8733

alamb · 2020-11-21T14:07:21Z

The CI failures don't seem related to this PR -- I am going to retrigger them

alamb · 2020-11-21T23:07:42Z

CI is green, so merging

alamb · 2020-11-22T12:17:54Z

rust/arrow/src/csv/reader.rs

+impl Parser for BooleanType {
+    fn parse(string: &str) -> Option<bool> {
+        if string == "false" || string == "FALSE" || string == "False" {
+            return Some(true);


I totally missed this, but this line has a bug parsing in this field appears to be backwards -- "false" seems to return true -- it looks like it got added in 52bf0e8 -- FYI @Dandandan and @nevi-me -- I have a fix for it shortly

O haha stupid bug, next time I'll make sure to add some unit tests directly in the PR.
Thanks for fixing.

it was my bad for not catching it in review as well

Dandandan added 2 commits November 19, 2020 12:17

Specialize parsers

7d802e1

Format

9fe98bc

github-actions bot added the Component: Rust label Nov 19, 2020

Dandandan changed the title ~~ARROW-10654: Specialize parsers~~ ARROW-10654: [Rust] Specialize parsing of floats / bools Nov 19, 2020

Format

9b3e5d8

Dandandan mentioned this pull request Nov 19, 2020

ARROW-10649: [Rust] Parse manually in infer_field_schema, remove lazy static dependency #8710

Closed

nevi-me approved these changes Nov 21, 2020

View reviewed changes

Dandandan mentioned this pull request Nov 21, 2020

ARROW-4804: [Rust] Parse Date32 and Date64 in CSV reader #8611

Closed

alamb changed the title ~~ARROW-10654: [Rust] Specialize parsing of floats / bools~~ ARROW-10654: [Rust] Specialize parsing of floats / bools iin CSV Reader Nov 21, 2020

alamb changed the title ~~ARROW-10654: [Rust] Specialize parsing of floats / bools iin CSV Reader~~ ARROW-10654: [Rust] Specialize parsing of floats / bools in CSV Reader Nov 21, 2020

alamb approved these changes Nov 21, 2020

View reviewed changes

alamb mentioned this pull request Nov 21, 2020

ARROW-10677: [Rust] Fix CSV Boolean parsing + add tests to demonstrate supported csv parsing #8733

Closed

Write out cases in boolean parser

52bf0e8

alamb closed this in d873657 Nov 21, 2020

alamb reviewed Nov 22, 2020

View reviewed changes

asfimport mentioned this pull request Dec 3, 2020

[Rust] Specialize parsing of floats / bools #26610

Closed

ARROW-10654: [Rust] Specialize parsing of floats / bools in CSV Reader #8714

ARROW-10654: [Rust] Specialize parsing of floats / bools in CSV Reader #8714

Uh oh!

Conversation

Dandandan commented Nov 19, 2020

Uh oh!

Dandandan commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 19, 2020

Uh oh!

Dandandan commented Nov 19, 2020

Uh oh!

nevi-me left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dandandan Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Nov 21, 2020

Uh oh!

alamb commented Nov 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Dandandan commented Nov 19, 2020 •

edited

Loading

alamb Nov 21, 2020 •

edited

Loading

Dandandan Nov 21, 2020 •

edited

Loading