PARQUET-1002: Compute statistics based on Sort Order #383

majetideepak · 2017-08-09T20:12:49Z

@lomereiter, You might also want to take a look since you previously implemented the Statistics API.

lomereiter · 2017-08-10T18:25:07Z

src/parquet/statistics.cc

  IncrementNullCount(null_count);
  IncrementDistinctCount(distinct_count);

+  comparator_ = std::static_pointer_cast<CompareDefault<DType> >(Compare::Make(schema));


Should this line be just moved into SetDescr then?

Yeah, good catch.

lomereiter · 2017-08-10T18:35:15Z

src/parquet/util/comparison.cc

+namespace parquet {
+
+std::shared_ptr<Compare> Compare::Make(const ColumnDescriptor* descr) {
+  if (SortOrder::SIGNED == GetSortOrder(descr->logical_type(), descr->physical_type())) {


I counted 5 occurrences of this call, it probably makes sense to add a convenience method to ColumnDescriptor

Yes! makes sense.

majetideepak · 2017-08-21T22:28:49Z

@wesm, @xhochy can you take a look at this? thanks!

wesm · 2017-08-21T22:32:03Z

I'm sorry about the delay, I will try to review this today or worst case tomorrow

wesm · 2017-08-22T20:15:14Z

Going to review this today

wesm

thanks @majetideepak! this looks like it was a lot of work / very tedious. I'm sorry for the slow feedback.

I eyeballed the comparison details and didn't see anything awry. I admit to not having followed the comparison discussion vis-a-vis int96 on the Parquet syncs, but I trust you've implemented it correctly.

Most of my comments are nitpicks. I believe that @xhochy is back from vacation tomorrow so I would like for him to also look at this and then we try to do either a 1.2.1 release or 1.3.0 right away so that we don't continue to generate files with bad statistics.

wesm · 2017-08-23T02:51:54Z

src/parquet/file/metadata.cc

-  return SortOrder::UNKNOWN;
-}
+const ApplicationVersion ApplicationVersion::PARQUET_CPP_FIXED_STATS_VERSION =
+    ApplicationVersion("parquet-cpp version 1.2.1");


This will probably be 1.3.0, but we can also release 1.2.1 right away

Releasing 1.2.1 sounds like a good idea. But this value should work even if we make a 1.3.0 release.

I just noticed apache/parquet-format#55 adds some more changes to the min-max stats format. I think we should wait until that gets merged.

wesm · 2017-08-23T02:53:42Z

src/parquet/file/metadata.cc

+    // This for backward compatibility
+    if (is_signed) {
+      stats.max = val.max();
+      stats.min = val.min();


Do the new statistics fields cause any problems for older Parquet readers?

I checked and it didn't.

wesm · 2017-08-23T02:57:18Z

src/parquet/parquet_reader_writer-test.cc

+template <>
+void TestStatistics<DoubleType>::AddNodes(std::string name) {
+  // Double physical type has only Signed Statistics
+  fields_.push_back(schema::PrimitiveNode::Make(name, Repetition::REQUIRED, Type::DOUBLE,


Can this be simplified using type traits to avoid these specializations? We have type_traits, but they are seldom used, and could be templated on the FooType class rather than the type number, if that helps

I will take a look.

wesm · 2017-08-23T02:58:18Z

src/parquet/parquet_reader_writer-test.cc

@@ -0,0 +1,330 @@
+// Licensed to the Apache Software Foundation (ASF) under one


It seems like this belongs in statistics-test.cc, might it be clearer to put this code there?

My motivation is to add end to end writer-reader tests for Parquet which we currently don't have.

I would also like to see this in statistics-test.cc. We can move it back to src/parquet/parquet_reader_writer-test.cc once we have full end-to-end tests.

wesm · 2017-08-23T02:59:14Z

src/parquet/parquet_version.h

+#define CREATED_BY_VERSION "parquet-cpp version 1.2.1-SNAPSHOT"

-#endif  // PARQUET_VERSION_H
+#endif // PARQUET_VERSION_H


Do we mean to check this file in?

Removing this file causes one of the Travis CI tests to fail. I am not sure why. I will look into this as well.

Filed PARQUET-1088

wesm · 2017-08-23T03:01:31Z

src/parquet/util/comparison.h

-  explicit Compare(const ColumnDescriptor* descr) : type_length_(descr->type_length()) {}
-
-  inline bool operator()(const T& a, const T& b) { return a < b; }
+class PARQUET_EXPORT Compare {


nit: can we rename this Comparator?

wesm · 2017-08-23T03:02:21Z

src/parquet/util/comparison.h

+  typedef typename DType::c_type T;
+  CompareDefault() {}
+  virtual ~CompareDefault() {}
+  virtual bool operator()(const T& a, const T& b) { return a < b; }


override (instead of virtual)

will do here and below.

CompareDefault is the base class and will require virtual. I used override inside the derived classes.

wesm · 2017-08-23T03:02:24Z

src/parquet/util/comparison.h

+ public:
+  CompareDefault() {}
+  virtual ~CompareDefault() {}
+  virtual bool operator()(const Int96& a, const Int96& b) {


wesm · 2017-08-23T03:02:28Z

src/parquet/util/comparison.h

+ public:
+  CompareDefault() {}
+  virtual ~CompareDefault() {}
+  virtual bool operator()(const ByteArray& a, const ByteArray& b) {


wesm · 2017-08-23T03:02:36Z

src/parquet/util/comparison.h

+ public:
+  explicit CompareDefault(int length) : type_length_(length) {}
+  virtual ~CompareDefault() {}
+  virtual bool operator()(const FLBA& a, const FLBA& b) {


majetideepak · 2017-08-23T12:30:04Z

Thanks for the feedback @wesm! As far as I remember, the sync discussions were geared more towards UTF-8 (and other string encodings) and not much on Int96. Int96 is deprecated and is currently used only for Impala Timestamp values which require a custom sort order.

I have also implemented this patch with the idea of users be able to implement custom Comparator classes and plugin into the writer properties. Any custom order should be easy to implement now by simply extending the CompareDefault**Type** class

xhochy · 2017-08-23T14:08:33Z

src/parquet/column_writer.cc


    EncodedStatistics chunk_statistics = GetChunkStatistics();
-    if (chunk_statistics.is_set()) metadata_->SetStatistics(chunk_statistics);
+    if (chunk_statistics.is_set())


Nit: Please use braces.

xhochy · 2017-08-23T14:10:13Z

src/parquet/file/metadata.h


  // Checks if the Version has the correct statistics for a given column
-  bool HasCorrectStatistics(Type::type primitive) const;
+  bool HasCorrectStatistics(Type::type primitive, SortOrder::type sort_order) const;


As we change the interface here, we should make the new version 1.3.0.

@majetideepak alternatively can we provide a default argument here?

wesm · 2017-08-29T00:41:08Z

@majetideepak if you can incorporate feedback and make this not WIP, we should be able to cut the 1.3.0 RC after this is in

majetideepak · 2017-09-05T02:32:54Z

@wesm Sorry for the delay! I somehow missed your comment. I will incorporate this feedback tomorrow morning.

wesm · 2017-09-05T19:42:57Z

A follow up question on the comparator thing. Any way to do this as fully compile-time with no virtual functions (and so no exported symbols in the DLL)?

majetideepak · 2017-09-05T20:13:46Z

I couldn't come up with an elegant design to make it fully-compile time.
The comparator depends upon the SortOrder and the c_type of the parquet physical types.
The c_type can be determined at compile time for a given TypedRowGroupStatistics. I don't know if we can determine the SortOrder at compile time.
Another motivation for virtual functions is to allow users to specify custom comparators for unknown sort orders.

add int32 comparison INT96 fix statistics in metadata Use templates

read new max and min values

fix tests

Fix Warnings on Windows

xhochy

This looks ready to go except:

xhochy · 2017-09-10T06:12:43Z

src/parquet/parquet_reader_writer-test.cc

@@ -0,0 +1,330 @@
+// Licensed to the Apache Software Foundation (ASF) under one


I would also like to see this in statistics-test.cc. We can move it back to src/parquet/parquet_reader_writer-test.cc once we have full end-to-end tests.

majetideepak · 2017-09-10T12:58:44Z

Fixed issue https://github.com/apache/parquet-cpp/pull/383/files#r134647005
We can go ahead without the merge of apache/parquet-format#55. That PR only impacts how the SortOrder is inferred. This PR will not require any changes.

xhochy · 2017-09-11T07:25:41Z

Travis is failing with

/Users/travis/build/apache/parquet-cpp/src/parquet/statistics-test.cc:634:13: error: variable-sized object may not be initialized
  char vals[NUM_VALUES][FLBA_LENGTH] = {"b12345", "a12345", "c12345", "d12345", "e12345",

Once that is fixed, feel free to merge.

majetideepak · 2017-09-11T15:12:13Z

@xhochy Fixed the error. I don't think I have merge access yet!

wesm · 2017-09-11T15:30:05Z

Merging. It looks like you haven't been added to the LDAP group, @julienledem can you take a look?

majetideepak force-pushed the PARQUET-1002 branch 2 times, most recently from 6113f57 to f0560c6 Compare August 10, 2017 18:12

lomereiter reviewed Aug 10, 2017

View reviewed changes

majetideepak force-pushed the PARQUET-1002 branch 2 times, most recently from 13e82fc to 2150379 Compare August 10, 2017 19:48

wesm reviewed Aug 23, 2017

View reviewed changes

majetideepak changed the title ~~PARQUET-1002: Compute statistics based on Sort Order~~ [WIP] PARQUET-1002: Compute statistics based on Sort Order Aug 23, 2017

xhochy reviewed Aug 23, 2017

View reviewed changes

majetideepak force-pushed the PARQUET-1002 branch from 68283b0 to 8d401f0 Compare September 5, 2017 17:53

majetideepak changed the title ~~[WIP] PARQUET-1002: Compute statistics based on Sort Order~~ PARQUET-1002: Compute statistics based on Sort Order Sep 5, 2017

Deepak Majeti added 12 commits September 6, 2017 08:37

Move SortOrder to types.h

046f8d3

add int32 comparison INT96 fix statistics in metadata Use templates

Fix reader

4287565

read new max and min values

extend testing

c4b1827

fix tests

Add Reader Writer Statistics Test

3df2686

rename to Make

c1abb69

make format

d58658d

Add test for Unknown sort order

b4fba8b

format

55960fe

fix failure

808e764

Comments

6a985de

Add another test

bdd8d37

fix Clang failure and improve test

85a9052

Fix Warnings on Windows

Deepak Majeti added 6 commits September 6, 2017 08:37

fix failures

17a79f3

Review comments

3cb8a3e

Avoid UTF-8 file encoding mismatch between windows and linux

f87bda6

Review comments

ba18ae6

Rename compare to comparator

c5d9610

Fix formatting

75ea475

majetideepak force-pushed the PARQUET-1002 branch from 8d401f0 to 75ea475 Compare September 6, 2017 12:38

majetideepak mentioned this pull request Sep 9, 2017

PARQUET-1082: Change MutableBuffer to Buffer to account for deprecation in ARROW-1402 #384

Closed

xhochy suggested changes Sep 10, 2017

View reviewed changes

Deepak Majeti added 2 commits September 10, 2017 08:35

Move test to statistics-test

712ea90

change fix version from 1.2.1 to 1.3.0

48dd22d

fix error

5a93fe3

asfgit closed this in cf6992a Sep 11, 2017

majetideepak deleted the PARQUET-1002 branch September 20, 2017 14:02

		@@ -0,0 +1,330 @@
		// Licensed to the Apache Software Foundation (ASF) under one

PARQUET-1002: Compute statistics based on Sort Order #383

PARQUET-1002: Compute statistics based on Sort Order #383

Uh oh!

Conversation

majetideepak commented Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

majetideepak commented Aug 21, 2017

Uh oh!

wesm commented Aug 21, 2017

Uh oh!

wesm commented Aug 22, 2017

Uh oh!

wesm 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!

majetideepak Aug 23, 2017 • 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!

majetideepak Aug 23, 2017 • 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!

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!

xhochy Sep 10, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

majetideepak Sep 5, 2017 • 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!

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!

majetideepak commented Aug 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majetideepak commented Aug 9, 2017 •

edited

Loading

majetideepak Aug 23, 2017 •

edited

Loading

majetideepak Aug 23, 2017 •

edited

Loading

xhochy Sep 10, 2017 •

edited

Loading

majetideepak Sep 5, 2017 •

edited

Loading

xhochy Sep 10, 2017 •

edited

Loading