Adding ability to parse float from number with leading decimal #831

brianrackle · 2021-10-15T22:47:12Z

Which issue does this PR close?

Closes #830

Rationale for this change

The issue this fixes results in real-world data sets being incorrectly inferred as strings when they are floats.

What changes are included in this PR?

Modifying the regular expression to make the digit proceeding the decimal optional

Are there any user-facing changes?

CSV inferred schemas which include floating-point data doesn't need a proceeding zero for floats between 0 and 1

codecov-commenter · 2021-10-16T04:33:46Z

Codecov Report

Merging #831 (f8a2dc1) into master (05eb63f) will decrease coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #831      +/-   ##
==========================================
- Coverage   82.45%   82.31%   -0.15%     
==========================================
  Files         168      168              
  Lines       48175    48092      -83     
==========================================
- Hits        39723    39587     -136     
- Misses       8452     8505      +53

Impacted Files	Coverage Δ
arrow/src/csv/reader.rs	`90.01% <100.00%> (-1.77%)`	⬇️
arrow/src/array/transform/fixed_binary.rs	`75.00% <0.00%> (-3.95%)`	⬇️
parquet/src/schema/visitor.rs	`67.10% <0.00%> (-2.64%)`	⬇️
arrow/src/compute/kernels/cast_utils.rs	`87.50% <0.00%> (-2.50%)`	⬇️
parquet/src/encodings/decoding.rs	`90.45% <0.00%> (-2.02%)`	⬇️
arrow/src/datatypes/ffi.rs	`70.85% <0.00%> (-2.02%)`	⬇️
parquet/src/arrow/arrow_reader.rs	`89.32% <0.00%> (-1.95%)`	⬇️
arrow/src/ffi.rs	`84.69% <0.00%> (-1.89%)`	⬇️
arrow/src/util/data_gen.rs	`75.44% <0.00%> (-1.66%)`	⬇️
... and 70 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05eb63f...f8a2dc1. Read the comment docs.

jimexist · 2021-10-16T06:19:29Z

arrow/src/csv/reader.rs

@@ -1341,6 +1341,7 @@ mod tests {
        assert_eq!(infer_field_schema("\"123\""), DataType::Utf8);
        assert_eq!(infer_field_schema("10"), DataType::Int64);
        assert_eq!(infer_field_schema("10.2"), DataType::Float64);
+        assert_eq!(infer_field_schema(".2"), DataType::Float64);


Can you also try to cover cases like '2.'?

sure, your suggesting that should also be inferred and parsed as a valid float as well right?

@brianrackle can you please rebase this PR (it should then pass CI) and let us know if you will be able to add the test that @jimexist suggests?

I am starting to prepare for the 6.1 release (I plan to build the release candidate late next week) and would like to get this in if possible

Taking a look at the JSON parser in more detail and running some tests to see how the JSON parser deals with a case like '2.'. Reason being is that I think '2.' is a little less clear how to handle because that value can be represented by an Int, so the question is does the presence of a decimal automatically infer Float or should there be some laziness to upgrading from Int to Float and only done when necessary.

As it isn't clear what to do in this case, filed #929 to track this as a follow on work

@brackle-lattice - here is a PR to your branch to pickup changes from master as well as add a test showing what happens with 2. (it is parsed as Utf8). brianrackle#1 (github shows the full diff which is somewhat silly, as it is a merge commit and then a 2 line change). I think what that change this PR will be ready to go

If you don't have time to merge it I can also create a new PR with the changes too to get it into arrow-rs

alamb · 2021-10-17T11:20:25Z

The tests were failing with

Run export CARGO_HOME="/github/home/.cargo"
    Updating crates.io index
error: failed to select a version for the requirement `comfy-table = "^4.0"`
candidate versions found which didn't match: 2.1.0, 2.0.0, 1.6.0, ...
location searched: crates.io index
required by package `arrow v7.0.0-SNAPSHOT (/__w/arrow-rs/arrow-rs/arrow)`
Error: Process completed with exit code 101.

I have triggered the tests to re-run so hopefully they will pass on the next run

…org/core/usize/constant.MAX.html and making consistent with other usages

alamb · 2021-10-18T13:41:53Z

Filed #838 for the CI failures

alamb · 2021-10-18T13:51:52Z

filed #838 for nightly test failures (there is a PR up so hopefully once that is merged, we can rebase this branch and have it pass too)

brianrackle · 2021-11-03T17:37:22Z

It looks like workflows are still awaiting approval

alamb · 2021-11-03T18:06:59Z

Workflows started

alamb · 2021-11-03T20:48:12Z

The integration test failure is unrelated. @brianrackle can you clarify your plans for #831 (comment) ?

alamb · 2021-11-09T11:19:12Z

arrow/src/csv/reader.rs

@@ -1341,6 +1341,7 @@ mod tests {
        assert_eq!(infer_field_schema("\"123\""), DataType::Utf8);
        assert_eq!(infer_field_schema("10"), DataType::Int64);
        assert_eq!(infer_field_schema("10.2"), DataType::Float64);
+        assert_eq!(infer_field_schema(".2"), DataType::Float64);


@brackle-lattice - here is a PR to your branch to pickup changes from master as well as add a test showing what happens with 2. (it is parsed as Utf8). brianrackle#1 (github shows the full diff which is somewhat silly, as it is a merge commit and then a 2 line change). I think what that change this PR will be ready to go

If you don't have time to merge it I can also create a new PR with the changes too to get it into arrow-rs

Update for JSON schema inference

brianrackle · 2021-11-09T16:43:56Z

@alamb thanks, I believe I have the PR ready. Thanks for helping me get this change in. I will follow up on the other ticket with more info when I get a chance to look into it.

* Adding ability to parse float from number with leading decimal * Fixing deprecated std::usize::MAX constant per https://doc.rust-lang.org/core/usize/constant.MAX.html and making consistent with other usages * Add test case for 2. and issue link Co-authored-by: Andrew Lamb <[email protected]>

…#962) * Adding ability to parse float from number with leading decimal * Fixing deprecated std::usize::MAX constant per https://doc.rust-lang.org/core/usize/constant.MAX.html and making consistent with other usages * Add test case for 2. and issue link Co-authored-by: Andrew Lamb <[email protected]> Co-authored-by: Brian Rackle <[email protected]>

Adding ability to parse float from number with leading decimal

62289e9

github-actions bot added the arrow Changes to the arrow crate label Oct 15, 2021

houqp approved these changes Oct 16, 2021

View reviewed changes

jimexist reviewed Oct 16, 2021

View reviewed changes

Fixing deprecated std::usize::MAX constant per https://doc.rust-lang.…

b0b9a15

…org/core/usize/constant.MAX.html and making consistent with other usages

Merge branch 'apache:master' into master

f8a2dc1

alamb mentioned this pull request Nov 9, 2021

Review / fix behavior of json schema inference for numbers with trailing decimal point (e.g. 2.) #929

Closed

alamb added 2 commits November 9, 2021 06:11

Merge remote-tracking branch 'apache/master' into brianrackle_master

5d817af

Add test case for 2. and issue link

815ea72

alamb approved these changes Nov 9, 2021

View reviewed changes

Merge pull request #1 from alamb/brianrackle_master

e388f11

Update for JSON schema inference

alamb merged commit ece8a7c into apache:master Nov 9, 2021

alamb added the cherry-picked label Nov 20, 2021

alamb mentioned this pull request Nov 20, 2021

Cherry pick Adding ability to parse float from number with leading decimal to active_release #962

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding ability to parse float from number with leading decimal #831

Adding ability to parse float from number with leading decimal #831

brianrackle commented Oct 15, 2021

codecov-commenter commented Oct 16, 2021 •

edited

Loading

jimexist Oct 16, 2021

brianrackle Oct 16, 2021

alamb Oct 23, 2021 •

edited

Loading

brackle-lattice Nov 8, 2021

alamb Nov 9, 2021

alamb Nov 9, 2021

alamb commented Oct 17, 2021

alamb commented Oct 18, 2021

alamb commented Oct 18, 2021

brianrackle commented Nov 3, 2021

alamb commented Nov 3, 2021

alamb commented Nov 3, 2021

alamb Nov 9, 2021

brianrackle commented Nov 9, 2021

Adding ability to parse float from number with leading decimal #831

Adding ability to parse float from number with leading decimal #831

Conversation

brianrackle commented Oct 15, 2021

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

codecov-commenter commented Oct 16, 2021 • edited Loading

Codecov Report

jimexist Oct 16, 2021

Choose a reason for hiding this comment

brianrackle Oct 16, 2021

Choose a reason for hiding this comment

alamb Oct 23, 2021 • edited Loading

Choose a reason for hiding this comment

brackle-lattice Nov 8, 2021

Choose a reason for hiding this comment

alamb Nov 9, 2021

Choose a reason for hiding this comment

alamb Nov 9, 2021

Choose a reason for hiding this comment

alamb commented Oct 17, 2021

alamb commented Oct 18, 2021

alamb commented Oct 18, 2021

brianrackle commented Nov 3, 2021

alamb commented Nov 3, 2021

alamb commented Nov 3, 2021

alamb Nov 9, 2021

Choose a reason for hiding this comment

brianrackle commented Nov 9, 2021

codecov-commenter commented Oct 16, 2021 •

edited

Loading

alamb Oct 23, 2021 •

edited

Loading