PARQUET-593: Add API for writing Page statistics #129

lomereiter · 2016-06-26T18:44:21Z

No description provided.

lomereiter · 2016-06-26T18:50:44Z

src/parquet/column/page.h


-  virtual void Close() = 0;
+  // TODO think of a better way to pass column chunk statistics
+  virtual void Close(const std::shared_ptr<RowGroupStatistics>& chunk_statistics) = 0;


The API is admittedly rather ugly here, because PageWriter operates on raw bytes and not values. It could handle column chunk statistics by itself, but then it would have to be initialized somewhere with a concrete type.

wesm · 2016-06-29T20:21:02Z

Great to make progress on this -- a few nits, but main concern is working to create more thorough test cases verifying correctness of reads and writes (since databases use these statistics for to know when to skip pages / row groups, etc.).

wesm · 2016-07-06T00:58:54Z

Thank you, I will review the updated patch here in short order.

lomereiter · 2016-07-06T09:02:00Z

Thanks for your time.
I haven't got around to write comparison tests yet, but here's a quick summary of changes:

writing statistics is disabled by default (often it makes sense only for a few specific columns)
writing and reading is covered with tests, for single- and multi-page row groups
all column-level settings are grouped into a struct, as it's more convenient and doesn't require multiple unordered_map accesses

wesm · 2016-07-06T22:59:54Z

src/parquet/column/properties.h

+  Encoding::type encoding;
+  Compression::type codec;
+  bool collect_statistics;
+};


If this is strictly POD, this should be a struct (note google style guidelines on member variable names for structs vs classes)

wesm · 2016-09-01T02:21:08Z

@lomereiter @piyushnarang looks like we should do an unsigned binary comparison here. @lomereiter it looks like comparators are the only thing left -- you should wait to rebase until PARQUET-573 is merged. thanks for patience!

wesm · 2016-09-01T02:22:50Z

sorry looks like we still need to computed the signed max/min -- we can come back and add the unsigned statistics when the format version drops

apache/parquet-format#42 (comment)

piyushnarang · 2016-09-01T17:44:12Z

Yeah there's been a bit of iteration on the parquet-format PR :-)

wesm · 2016-09-06T20:43:03Z

@lomereiter let's proceed and compute only the signed max/min in this patch until the dust settles on parquet-format?

lomereiter · 2016-09-08T07:06:54Z

@wesm ok, I'll rebase and add comparison tests this weekend so that it can be merged finally

lomereiter · 2016-09-12T21:48:39Z

So far rebased only, will get back to tests and further cleanup next evening.

lomereiter · 2016-09-18T21:09:09Z

Switched to using signed min/max for byte array types, added a few comparison tests. Some inefficiencies in read/write paths remain, but are topics for separate JIRAs.

wesm · 2016-09-18T21:44:22Z

Awesome, will review when I can.

xhochy · 2016-09-20T07:04:28Z

src/parquet/column/page.h

-  friend class SerializedPageReader;
-  std::string max_;
-  std::string min_;
+  EncodedStatistics statistics_;


To avoid copying a lot of data, this would probably be better as a shared_ptr.

xhochy · 2016-09-20T07:05:44Z

src/parquet/column/properties.h

 static constexpr bool DEFAULT_IS_DICTIONARY_ENABLED = true;
 static constexpr int64_t DEFAULT_DICTIONARY_PAGE_SIZE_LIMIT = DEFAULT_PAGE_SIZE;
 static constexpr int64_t DEFAULT_WRITE_BATCH_SIZE = 1024;
+static constexpr bool DEFAULT_COLLECT_STATISTICS = false;


Are they enabled as default in parquet-mr? We should probably stick to the same defaults here. Collecting statistics costs a bit but will improve query times on the files a lot.

I checked and they are indeed enabled by default. However, if values of min/max are large (> 4KB) they are not written (https://issues.apache.org/jira/browse/PARQUET-372).

xhochy · 2016-09-20T07:09:49Z

src/parquet/column/properties.h

 static constexpr Compression::type DEFAULT_COMPRESSION_TYPE = Compression::UNCOMPRESSED;

-using ColumnCodecs = std::unordered_map<std::string, Compression::type>;
+class PARQUET_EXPORT ColumnSettings {


I'm ok with this class (although it should be named ColumnProperties) but not with the outcome at the end. Currently it is possible to select a custom encoding for a column but fallback to the file-global defaults for all other settings. With this new class, once we have set one property on a column-basis, for this column we have to either set all other file-global defaults explicitly on this column or we'll get the parquet-cpp defaults.

I don't understand this objection since WriterProperties::Builder interface is unchanged.

The interface is the same, just the behaviour changed.

Gotcha. AFAIU all setters should first set column_settings_[path] to default_column_settings_.

This will still not be enough. You will also need an isset for each entry. I would suggest to still use the numerous maps in the builder and build the column_settings_ instances at the end.

Hope I have covered my issue in an understandable unit test: #166

Ok, went with your suggestion, thanks for putting in the test.

xhochy · 2016-09-20T07:14:04Z

src/parquet/column/statistics.cc

+namespace parquet {
+
+template <typename TypedStats>
+std::shared_ptr<TypedStats> RowGroupStatistics::as() {


I would prefer if this was done explicitly by the user as with all other casts.

xhochy · 2016-09-20T07:14:56Z

src/parquet/column/statistics.cc

+void TypedRowGroupStatistics<FLBAType>::Copy(
+    const FLBA& src, FLBA* dst, OwnedMutableBuffer& buffer) {
+  if (dst->ptr == src.ptr) return;
+  auto len = descr_->type_length();


Please use the explicity type here.

xhochy · 2016-09-20T07:21:57Z

src/parquet/column/writer.h

+  std::shared_ptr<RowGroupStatistics> chunk_statistics_;
+
+  template <typename Type>
+  void InitStatistics() {


Please make this part of the TypedColumnWriter

xhochy · 2016-09-20T07:23:48Z

src/parquet/column/writer.cc

  }
+
+  if (properties->collect_statistics(descr_->path())) {
+    InitStatistics<Type>();


No need for <Type> if this function is part of TypedColumnWriter

xhochy · 2016-09-20T07:25:18Z

src/parquet/file/metadata.cc

+    case Type::FIXED_LEN_BYTE_ARRAY:
+      return MakeColumnStatsT<FLBAType>(meta_data, descr);
+  }
+  return nullptr;


Raise an exception if the type is not supported.

xhochy · 2016-09-20T07:26:56Z

src/parquet/file/metadata.cc

 namespace parquet {

+template <typename DType>
+static std::shared_ptr<RowGroupStatistics> MakeColumnStatsT(


MakeColumnStatsTyped

xhochy · 2016-09-20T07:28:35Z

src/parquet/file/reader.cc

    for (auto i : selected_columns) {
      auto column_chunk = group_metadata->ColumnChunk(i);
-      const ColumnStatistics stats = column_chunk->statistics();
+      const auto stats = column_chunk->statistics();


Please write the type here.

@wesm Do you have a link to the guidelines at hand?

https://google.github.io/styleguide/cppguide.html#auto

aside: we should survey our usages of auto to make sure we are using const auto& in the right places

xhochy

First pass, will review again once the changes are made.

wesm · 2016-09-29T01:49:44Z

I think this is about ready to go. Needs a rebase. @xhochy ?

xhochy

LGTM, rebase & then merge!

* EncodedStatistics now stores min/max as shared_ptr<string> * restored old WriterProperties::Builder behavior * renamed EncodedMin/EncodedMax to EncodeMin/EncodeMax * moved page_/chunk_statistics_ to TypedColumnWriter

lomereiter · 2016-10-03T11:28:15Z

Thanks all, I rebased the branch.

wesm · 2016-10-03T17:40:07Z

+1

lomereiter reviewed Jun 26, 2016
View reviewed changes

lomereiter force-pushed the parquet-593 branch from 838d351 to c4ba26d Compare July 3, 2016 10:04

wesm reviewed Jul 6, 2016
View reviewed changes

lomereiter force-pushed the parquet-593 branch from b80b07e to 45e418a Compare September 12, 2016 21:40

lomereiter force-pushed the parquet-593 branch 2 times, most recently from 8adc287 to 2594141 Compare September 18, 2016 20:16

xhochy reviewed Sep 20, 2016

View reviewed changes

xhochy suggested changes Sep 20, 2016

View reviewed changes

xhochy approved these changes Oct 1, 2016

View reviewed changes

Artem Tarasov added 8 commits October 3, 2016 12:12

writing min/max/null count statistics

bdf6a2d

use PrimitiveTypedTest for the test

c26508d

fix benchmark

2e39fc7

switched to signed binary comparison, added tests

f16b121

fixed linter warnings

48ff923

fix subobject initialization

f42950d

put back accidentally removed counts printing

a32ccc7

address reviewer comments

6a06dc1

* EncodedStatistics now stores min/max as shared_ptr<string> * restored old WriterProperties::Builder behavior * renamed EncodedMin/EncodedMax to EncodeMin/EncodeMax * moved page_/chunk_statistics_ to TypedColumnWriter

lomereiter force-pushed the parquet-593 branch from 06612c3 to 4c0ed2a Compare October 3, 2016 10:21

further cleanup in TypedColumnWriter, fixed tests

0be02ec

lomereiter force-pushed the parquet-593 branch from 4c0ed2a to 0be02ec Compare October 3, 2016 11:09

asfgit closed this in 176b08c Oct 3, 2016

lomereiter deleted the parquet-593 branch July 4, 2017 19:08

PARQUET-593: Add API for writing Page statistics #129

PARQUET-593: Add API for writing Page statistics #129

Uh oh!

Conversation

lomereiter commented Jun 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Jun 29, 2016

Uh oh!

wesm commented Jul 6, 2016

Uh oh!

lomereiter commented Jul 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Sep 1, 2016

Uh oh!

wesm commented Sep 1, 2016

Uh oh!

piyushnarang commented Sep 1, 2016

Uh oh!

wesm commented Sep 6, 2016

Uh oh!

lomereiter commented Sep 8, 2016

Uh oh!

lomereiter commented Sep 12, 2016

Uh oh!

lomereiter commented Sep 18, 2016

Uh oh!

wesm commented Sep 18, 2016

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!

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!

Choose a reason for hiding this comment

Uh oh!

xhochy left a comment

Choose a reason for hiding this comment

Uh oh!

wesm commented Sep 29, 2016

Uh oh!

xhochy left a comment

Choose a reason for hiding this comment

Uh oh!

lomereiter commented Oct 3, 2016

Uh oh!