Fix parquet string statistics generation #643

alamb · 2021-07-31T12:13:58Z

Which issue does this PR close?

Closes #641

Rationale for this change

Statistics for strings (aka ByteArrays in the parquet format) are not calculated correctly

For example, given the strings "z", and "aa", the parquet writer will determine that "z" is the minimum value (because "z" is shorter).

What changes are included in this PR?

Fix the comparison code for ByteArray to lexographically compare the data
Add more statistics test coverage (kudos to @crepererum for the test harness in fix NaN handling in parquet statistics #256 which made this easy)

Correctness

I verified that the python implementation compares the strings lexicographically as proposed in this PR rather than the current behavior (see #641 (comment))

I also messed around with how partial cmp works with std::Option and I believe this implementation is consistent. You can see it for yourself in this playground

Notes

I could not figure out how to test for "Null" values in statistics (though I tested end to end using the ArrowWriter and confirmed null values didn't appear in the statistics, as expected)

Also, there are three things testing revealed:

BoolType columns produce Int32Stats which I found confusing
FixedLenByteArray columns produce ByteArrayStats
I don't know enough about how Int96 works to properly test it (my attempt failed badly)

Are there any user-facing changes?

correct statistics

alamb · 2021-07-31T12:16:34Z

parquet/src/data_type.rs

-                    }
-                    Some(Ordering::Equal)
-                }
+        // sort nulls first (consistent with PartialCmp on Option)


The previous implementation of PartialOrd seems to have been introduced in 48c3771 in apache/arrow#7622 by

It would be great if @zeevm or @sunchao had any additional context or comment to share.

Good catch! the original PR is apache/arrow#7586 but yea we missed this in the review and I think the existing logic is incorrect.

alamb · 2021-07-31T12:17:04Z

parquet/src/column/writer.rs

+        let stats = statistics_roundtrip::<BoolType>(&[true, false, false, true]);
+        assert!(stats.has_min_max_set());
+        // should this be BooleanStatistics??
+        if let Statistics::Int32(stats) = stats {


note 1: it is strange that a Boolean column produces Int32Stats (and not BooleanStats)

Filed #659 to track

alamb · 2021-07-31T12:17:50Z

parquet/src/column/writer.rs

+
+        let stats = statistics_roundtrip::<FixedLenByteArrayType>(&input);
+        assert!(stats.has_min_max_set());
+        // should it be FixedLenByteArray?


note 2: it is strange that a FixedLenByteArray column produces ByteArrayStats (and not FixedLenByteArrayStats)

alamb · 2021-07-31T12:18:28Z

parquet/src/column/writer.rs

+        }
+    }
+
+    // // TODO test int 96 stats -- this was failing


Note 3: I have no idea if this test is incorrect or if there is something wrong with the Int96 implementation. As Int96 seems to be deprecated, I don't plan to spend much more time on it

I think we should still keep INT96 for backward compatibility (it is still used today). In parquet-mr this is done by converting the binary into signed ints (see here).

codecov-commenter · 2021-07-31T12:30:22Z

Codecov Report

Merging #643 (7c573c8) into master (e84fe20) will increase coverage by 0.04%.
The diff coverage is 87.09%.

@@            Coverage Diff             @@
##           master     #643      +/-   ##
==========================================
+ Coverage   82.47%   82.52%   +0.04%     
==========================================
  Files         167      168       +1     
  Lines       46454    47280     +826     
==========================================
+ Hits        38314    39016     +702     
- Misses       8140     8264     +124

Impacted Files	Coverage Δ
parquet/src/data_type.rs	`77.80% <85.71%> (+0.16%)`	⬆️
parquet/src/column/writer.rs	`92.93% <87.27%> (-0.37%)`	⬇️
arrow/src/array/equal_json.rs	`88.69% <0.00%> (-2.52%)`	⬇️
parquet/src/arrow/schema.rs	`87.35% <0.00%> (-1.18%)`	⬇️
arrow/src/datatypes/datatype.rs	`66.08% <0.00%> (-0.74%)`	⬇️
arrow/src/array/equal/mod.rs	`93.45% <0.00%> (-0.48%)`	⬇️
arrow/src/datatypes/field.rs	`51.36% <0.00%> (-0.47%)`	⬇️
arrow/src/array/builder.rs	`85.97% <0.00%> (-0.29%)`	⬇️
arrow/src/ipc/convert.rs	`92.93% <0.00%> (-0.06%)`	⬇️
arrow/src/json/reader.rs	`83.99% <0.00%> (-0.04%)`	⬇️
... and 14 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 e84fe20...7c573c8. Read the comment docs.

Dandandan · 2021-08-01T17:54:47Z

parquet/src/data_type.rs

@@ -1356,7 +1351,7 @@ mod tests {
        let ba4 = ByteArray::from(vec![]);
        let ba5 = ByteArray::from(vec![2, 2, 3]);

-        assert!(ba1 > ba2);
+        assert!(ba1 < ba2);


alamb · 2021-08-05T00:59:04Z

Thanks for the review -- I plan to polish this one up tomorrow

alamb · 2021-08-05T12:41:35Z

parquet/src/column/writer.rs

+    }
+
+    #[test]
+    fn test_int96_statistics() {


Fixed Int96 stats test

alamb · 2021-08-05T12:42:49Z

@nevi-me / @sunchao I have resolved all outstanding questions in this PR and I think it is ready for review

nevi-me

I've gone through the change and the test cases, LGTM

nevi-me · 2021-08-08T07:46:10Z

I'm merging this one, @alamb has opened issues for follow-ups of what he noted while working on this. If there's anything else, it can be addressed separately.

alamb · 2021-08-08T10:30:49Z

Thank you @nevi-me ❤️

* Fix string statistics generation, add tests * fix Int96 stats test * Add notes for additional tickets

github-actions bot added the parquet Changes to the parquet crate label Jul 31, 2021

alamb commented Jul 31, 2021

View reviewed changes

alamb requested a review from sunchao August 1, 2021 10:30

Dandandan reviewed Aug 1, 2021

View reviewed changes

nevi-me self-requested a review August 2, 2021 17:31

xianwill mentioned this pull request Aug 2, 2021

Add failing test to track arrow-rs PR#643 delta-io/kafka-delta-ingest#45

Closed

alamb added 2 commits August 5, 2021 08:16

Fix string statistics generation, add tests

2f10b73

fix Int96 stats test

8f3d60e

This was referenced Aug 5, 2021

Parquet boolean columns write Int32 statistics #659

Closed

Parquet fixed length byte array columns write byte array statistics #660

Closed

Add notes for additional tickets

d0c3e93

alamb force-pushed the alamb/parquet_stats_bug branch from 7c573c8 to d0c3e93 Compare August 5, 2021 12:41

alamb commented Aug 5, 2021

View reviewed changes

parquet/src/column/writer.rs

}

#[test]

fn test_int96_statistics() {

Copy link

Contributor Author

alamb Aug 5, 2021

Choose a reason for hiding this comment

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

Fixed Int96 stats test

This was referenced Aug 5, 2021

Write boolean stats for boolean columns (not i32 stats) #661

Merged

Write FixedLenByteArray stats for FixedLenByteArray columns (not ByteArray stats) #662

Merged

nevi-me approved these changes Aug 8, 2021

View reviewed changes

nevi-me merged commit 4618ef5 into apache:master Aug 8, 2021

alamb deleted the alamb/parquet_stats_bug branch August 8, 2021 10:30

alamb added a commit that referenced this pull request Aug 8, 2021

Fix parquet string statistics generation (#643)

3274461

* Fix string statistics generation, add tests * fix Int96 stats test * Add notes for additional tickets

alamb added the cherry-picked label Aug 8, 2021

alamb mentioned this pull request Aug 8, 2021

Cherry pick Fix parquet string statistics generation to active_release #676

Merged

alamb added a commit that referenced this pull request Aug 9, 2021

Fix parquet string statistics generation (#643) (#676)

876f397

* Fix string statistics generation, add tests * fix Int96 stats test * Add notes for additional tickets

houqp mentioned this pull request Aug 12, 2021

tpch conversion generates parquet files that cannot be queried on simple predicates apache/datafusion#863

Closed

alamb mentioned this pull request Dec 16, 2021

Query failing to return any results when filter is an equality check on strings (bad statistics in parquet) apache/datafusion#1433

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix parquet string statistics generation #643

Fix parquet string statistics generation #643

alamb commented Jul 31, 2021

alamb Jul 31, 2021

sunchao Aug 1, 2021 •

edited

Loading

alamb Jul 31, 2021

alamb Aug 5, 2021

alamb Jul 31, 2021

alamb Aug 5, 2021

alamb Jul 31, 2021

sunchao Aug 1, 2021

codecov-commenter commented Jul 31, 2021

Dandandan Aug 1, 2021

alamb commented Aug 5, 2021

alamb Aug 5, 2021

alamb commented Aug 5, 2021

nevi-me left a comment

nevi-me commented Aug 8, 2021

alamb commented Aug 8, 2021

Fix parquet string statistics generation #643

Fix parquet string statistics generation #643

Conversation

alamb commented Jul 31, 2021

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Correctness

Notes

Are there any user-facing changes?

Choose a reason for hiding this comment

sunchao Aug 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 31, 2021

Codecov Report

Choose a reason for hiding this comment

alamb commented Aug 5, 2021

Choose a reason for hiding this comment

alamb commented Aug 5, 2021

nevi-me left a comment

Choose a reason for hiding this comment

nevi-me commented Aug 8, 2021

alamb commented Aug 8, 2021

sunchao Aug 1, 2021 •

edited

Loading