PARQUET-922: Add column indexes to parquet.thrift by lekv · Pull Request #72 · apache/parquet-format

lekv · 2017-10-10T15:48:22Z

I moved the design doc to a .md file and addressed the first round of review comments.

closes #63

This is based on work done by @mkornacker and @lekv who wrote the initial proposal and @poojanilangekar who evolved the design, wrote a prototypical implementation, and evaluated its performance.

Added PageLocation, OffsetIndex and ColumnIndex structures to the parquet.thrift file in order to support secondary indexes in parquet files.

This commit moves the design doc from [1] to a .md file. It removes the actual thrift definitions, which are added to parquet.thrift. [1] https://docs.google.com/document/d/1sBACp8Lbutuj1Zxdowvsrlm8ku4BFxf8U_Do5K2wSO4/edit#

rdblue · 2017-10-10T16:34:11Z

+ * value that does not exist on that page. For ordered columns, these values
+ * must be lower and upper bounds for consecutive pages respectively.
+ */
+  2: required list<binary> min_values


I like where you're going with the description, but I was initially confused by it. How about giving a concrete example?

It may also be good to note that descending order still stores "min" values here, meaning the values that come first in the ordering.

The guarantee for ordered columns is only required if max_values is omitted. The reason is that it's hard to come up with a value between one page's max and the next's min when truncating. Imagine I have a page that ends with "aaaaaaaaaaaaaaaaa" and the next one starts with "aaaaaaaaaaaaaaaaaaaab". If we're truncating strings to save space, we don't want to add logic to come up with a value between the two for the min. It's better in that case to truncate both and also write max_values.

Should we also clarify that the guarantee is that x >= lower and x <= upper for all values x in a page? I think we should be explicit that bounds could be equal.

Also we should clarify where we imply strictly lower vs lower. The current definition of min and max always implies "or equal" which means that the boundary values in min_values here can be in both pages. It is not min included and max excluded as you would expect for boundary values (unless we add the constraint of not starting a new page as long as the same value is repeated). This lowers the value of making max_values optional. I think it general everything is simpler if we keep max_values. Is it really worth making optional?

julienledem

Thanks for posting!
Please see my comments.

julienledem · 2017-10-10T17:43:21Z

+struct OffsetIndex {
+/**
+ * PageLocations, ordered by increasing PageLocation.offset. It is required
+ * that page_locations[i].first_row_index > page_locations[i+1].first_row_index.


did you mean "<" ?
first_row_index should be increasing as well

julienledem · 2017-10-10T17:54:42Z

+ * value that does not exist on that page. For ordered columns, these values
+ * must be lower and upper bounds for consecutive pages respectively.
+ */
+  2: required list<binary> min_values


Also we should clarify where we imply strictly lower vs lower. The current definition of min and max always implies "or equal" which means that the boundary values in min_values here can be in both pages. It is not min included and max excluded as you would expect for boundary values (unless we add the constraint of not starting a new page as long as the same value is repeated). This lowers the value of making max_values optional. I think it general everything is simpler if we keep max_values. Is it really worth making optional?

julienledem · 2017-10-10T18:06:42Z

pleas add "closes #63" in the description since it supersedes the previous PR.

julienledem · 2017-10-10T18:07:50Z

also mention @poojanilangekar so that this is attributed appropriately when we merge.

rdblue · 2017-10-10T18:12:11Z

+ * Consider the following values:
+ * | 37 31 29 23 | 19 17 13 11 | 7 5 3 2 |
+ * ^             ^             ^         ^
+ * Examples for valid lists of minimum values are [23 11 2] or [ 20 8 0 ].


I think I disagree? Min should respect the sort order, so a descending order produces 23 < 11 => true. Valid mins for this example are [37, 19, 7] or [38, 20, 8] or [37, 23, 11]

On second thought, I don't think it is a good idea to do this. The mins should always be the min defined by the type's order. Descending order shouldn't change that, but it does make the logic more confusing when trying to use it as both min and max.

When mins are min and max, descending order changes the page for which the min of P_i min is a max. For asecending order:

$min(P_i) \approx max(P_{i-1})$

For descending:

$min(P_i) \approx max(P_{i+1})$

That's annoying. Maybe we should just always require max_values as Julien suggests. That makes it easy to produce smaller bounds anyway by simplifying the constraints for truncation.

I'm ok with making max_values required, too. However, if min_values is ordered and partitions the values, then one can use binary search to find matching pages. To enable that, we would need some boolean that writers can set to indicate this property.

@julienledem I couldn't reply to your comment above for some Github reason. :(

rdblue · 2017-10-10T18:12:40Z

+ * ^             ^             ^         ^
+ * Examples for valid lists of minimum values are [23 11 2] or [ 20 8 0 ].
+ *
+ * If max_values is set, then pairs of min and max values may be overlapping.


Change "may be overlapping" to "may overlap".

rdblue · 2017-10-10T20:31:22Z

The test failure is that PageIndex.md doesn't have a license header. You can use the one from LogicalTypes.md.

Added license to PageIndex.md

lekv · 2017-10-11T17:26:11Z

I addressed the comments from today's sync meeting.

@julienledem @mkornacker @rdblue @zivanfi - Please have a look.

rdblue

A couple of minor comments.

rdblue · 2017-10-11T17:38:06Z

  DATA_PAGE_V2 = 3;
 }

+enum SortOrder {


Could this enum have a more specific name? Maybe BoundaryOrder?

rdblue · 2017-10-11T17:41:53Z

+   * entries in min_values and max_values should be ignored. If false, the
+   * corresponding entries must be valid.
+   */
  1: required list<bool> null_pages


We should specify that the min and max values for a page of only null values cannot be omitted from the min_values and max_values lists. Instead it should be byte[0] (because thrift doesn't allow null in the lists). This preserves the property that for page i in a column, its min is min_values[i] max is max_values[i], etc.

rdblue · 2017-10-11T19:02:56Z

 }

-enum SortOrder {
+enum BoundayOrder {


Should be "BoundaryOrder". Missing the "r".

rdblue · 2017-10-11T20:54:19Z

+1

zivanfi

+1

zivanfi · 2017-10-13T15:37:46Z

+  only reason to write page-level statistics when writing ColumnIndex structs
+  is to support older readers (not recommended).
+
+This allows a reader to find matching pages by performing a binary search in


I would add "For ordered columns" to the beginning of this sentence.

zivanfi · 2017-10-13T15:40:27Z

+  - under the License.
+  -->
+
+# ColumnIndex Layout to Support Page Skipping


I would suggest linking to this page from the main documentation page (README.md) so that it becomes discoverable by readers.

zivanfi · 2017-10-13T15:43:05Z

+  1: required list<bool> null_pages
+
+  /**
+   * Two lists containing lower and upper bounds for the values of each page.


No strong opinion here, just asking: Wouldn't it be nicer to have a single list of elements containing both a min and a max field?

I think the property that for page i, relevant information is stored at min_values[i], max_values[i], null_pages[i], etc. is convenient enough. Plus, it simplifies binary search and makes the search more cache-friendly.

Also, if min_/max_values were fields in a struct, the resulting serialized byte sequence would be less compact (I think).

zivanfi · 2017-10-13T15:46:07Z

+  /**
+   * Stores whether both min_values and max_values are orderd and if so, in
+   * which order. This allows readers to perform binary searches in both lists.
+   * Readers cannot assume that max_values[i] <= min_values[i+1], even if the


I agree with this design decision and understand the reason behind it, but it may be hard for readers to understand why this is good (actual_max[i]="Blart Versenwald III", actual_min[i+1]="Blart Versenwald IV", stored_max[i]="C", stored_min[i+1]="B"). I would suggest elaborating a bit on this in the .md file and maybe also putting a note in the comment of this struct to refer to the documentation for details.

Do you have a suggestion what to add so it becomes more clear?

Is this so you can truncate very large values (e.g., strings)? I think it would be helpful to point that out in the .md (ie, something along the lines that the writer can/should enforce some reasonable bound on the size of the index structures).

zivanfi · 2017-10-13T15:48:29Z

+  /**
+   * Two lists containing lower and upper bounds for the values of each page.
+   * These may be the actual minimum and maximum values found on a page, but can
+   * also be (more compact) values that does not exist on a page. Readers must


I agree with this design decision and understand the reason behind it (instead of storing "Blart Versenwald III" you can store "C" as the max value), but it may be hard for readers to understand why this is good. I would suggest elaborating a bit on this in the .md file and maybe also putting a note in the comment of this struct to refer to the documentation for details.

I thought that "more compact" was the main reason here. I added your example to make it more clear.

lekv · 2017-10-13T18:56:01Z

After we now expanded several comments in parquet.thrift I'm inclined to remove the PageIndex.md from the commit altogether. It seems to me that the parquet.thrift documents the structures well enough for an unambiguous implementation. The intent behind adding the indexes is still visible in the original design proposal, which is linked in the JIRA itself.

What does everyone think on removing it?

lekv · 2017-10-13T22:03:51Z

I removed the .md doc, let me know what you think.

rdblue · 2017-10-13T22:16:52Z

I liked having the proposal and the picture. I think it's good to have the intent and more discussion documented.

This reverts commit 5df2bbc.

This reverts commit a39bf49.

lekv · 2017-10-13T22:34:09Z

@julienledem, @mkornacker - Can you have a final look at this? Thanks!

mkornacker

Mostly minor clarification.

mkornacker · 2017-10-13T23:54:27Z

+## Goals
+1. Make both range scans and point lookups I/O efficient by allowing direct
+   access to pages based on their min and max values. In particular:
+1. A single-row lookup in a rowgroup based on the sort column of that rowgroup


The markup here looks wrong.

Markdown translates this into an enumerated list, but using the actual numbers does indeed make more sense. Fixed.

mkornacker · 2017-10-14T00:07:02Z

  DATA_PAGE_V2 = 3;
 }

+enum BoundaryOrder {


missing comment

mkornacker · 2017-10-14T00:10:05Z

   **/
  3: optional ColumnMetaData meta_data
+
+  /** File offset of this column's OffsetIndex **/


"of this ColumnChunk's" (these are column chunk, not column, specific)

mkornacker · 2017-10-14T00:24:42Z

+  1: required list<bool> null_pages
+
+  /**
+   * Two lists containing lower and upper bounds for the values of each page.


Also, if min_/max_values were fields in a struct, the resulting serialized byte sequence would be less compact (I think).

mkornacker · 2017-10-14T00:25:00Z

+  /**
+   * Two lists containing lower and upper bounds for the values of each page.
+   * These may be the actual minimum and maximum values found on a page, but
+   * can also be (more compact) values that does not exist on a page. For


"values that do not"

mkornacker · 2017-10-14T00:28:20Z

+
+  /**
+   * Stores whether both min_values and max_values are orderd and if so, in
+   * which order. This allows readers to perform binary searches in both lists.


"in which direction"

I added a bullet point to the .md file.

mkornacker · 2017-10-14T00:39:23Z

+  /**
+   * Stores whether both min_values and max_values are orderd and if so, in
+   * which order. This allows readers to perform binary searches in both lists.
+   * Readers cannot assume that max_values[i] <= min_values[i+1], even if the


Is this so you can truncate very large values (e.g., strings)? I think it would be helpful to point that out in the .md (ie, something along the lines that the writer can/should enforce some reasonable bound on the size of the index structures).

mkornacker · 2017-10-14T00:40:20Z

+  4: required BoundaryOrder boundary_order
+
+  /** A list containing the number of null values for each page **/
+  5: optional list<i64> null_counts


What happened to distinct_counts?

In one of the recent parquet syncs we discussed that having a distinct_count per page seemed of little use, because they cannot be aggregated across pages. Additionally it looks like no writer currently populates the field (parquet-mr and Impala both don't). We could still add it back in a future commit if we think it's necessary.

Ah right, good point. Why don't we mark Statistics.distinct_count as deprecated then?

I don't think we should deprecate the distinct count until we know we won't use it in the future. Lets just leave it as is for now.

zivanfi · 2017-10-16T13:17:39Z

I also prefer having a separate documentation page for the feature. In general I think that it is easier to grasp the logic behind different Parquet mechanisms if they are documented per feature rather than scattered around in different data structures. It's like reading a book vs. trying to read an encyclopedia end-to-end.

zivanfi · 2017-10-16T14:23:49Z

I still see a few corner cases that should be explicitly dealt with in my opinion. They are related to the feature that allows the min and max values to not necessarily be present in the data. Implementations can also pick other values as long as all data falls between min and max, with the goal of allowing long data to be truncated. For example instead of "Blart Versenwald III", one can use "B" as min and "C" as max.

The corner cases are the following:

According to the (already existing) specification, implementations must use unsigned byte-wise comparison for UTF-8 data. As result, the single byte 0xFF is a valid max value for any valid UTF-8 string. It is, however, not a valid UTF-8 string. We should either disallow this, or explicitly call out in the docs that implementations must not rely on min and max being valid values of the logical type.

Values of the decimal type, however, must be compared using the represented value. In this case, min and max must be valid values of the logical type.

The last corner case involves non-truncatable BLOBs. Suppose that the largest value in a page is a several-megabytes-large BLOB, consisting of the 0xFF byte repeated over and over. Since there is no shorter value that is larger in byte-wise comparison order and the specification does not allow omitting a single max value without omitting all of them, it means that either all max values must be omitted or the whole several-megabytes-large BLOB must be repeated for the max value.

mkornacker · 2017-10-16T15:28:15Z

 }

+/**
+ * Enum to annotate whether lists of min/max elements inside column indexes


I'd refer to the actual struct.

Change the comment.

I thought that Statistics.distinct_count still may be helpful in row groups, for example a reader may decide whether to read the data into dictionaries on the fly. I think @rdblue wanted to double check that no writer currently uses distinct count in pages and then we would deprecate the page statistics in a subsequent change.

I'm also happy to do either of those in the current PR. @rdblue, @julienledem - what do you think.

Good point.

Probably best to leave stats-related changes to a separate PR and get this in.

mkornacker · 2017-10-16T15:31:21Z

+  4: required BoundaryOrder boundary_order
+
+  /** A list containing the number of null values for each page **/
+  5: optional list<i64> null_counts


Ah right, good point. Why don't we mark Statistics.distinct_count as deprecated then?

lekv · 2017-10-16T17:11:47Z

@zivanfi - I expanded the comment before min_values to explain that values must be valid within the column's logical type. This seems like the more natural constraint to me. Let me know if you wish to change it to allowing invalid boundaries.

Regarding the maximum strings, I thought the conclusion of the weekly sync meeting was that the writer of such data will either have to omit the index altogether, or will have to accept the overhead by storing the full value within the index. Would you like us to state this in the comment explicitly, or do you suggest to change the structures to allow a more concise representation in such cases?

rdblue · 2017-10-16T22:27:42Z

As result, the single byte 0xFF is a valid max value for any valid UTF-8 string.

If I remember correctly, 0xFE is a valid UTF-8 character, which is close enough.

Suppose that the largest value in a page is a several-megabytes-large BLOB, consisting of the 0xFF byte repeated over and over

In the sync-up, the conclusion was that this is an extremely rare case that we don't need to worry about.

julienledem · 2017-10-16T23:20:25Z

+1

ZJONSSON · 2018-02-07T19:14:03Z

Awesome @mkornacker @lekv @poojanilangekar !! I'm working on a branch of parquetjs replicating the structure.

The PageIndex diagram suggests storing ColumnIndex array and OffsetIndex array separately in sequence. This makes it hard to read both (for a single page) in a single read, let alone reading all of them for a single column (across all pages).

As the location of each struct is up to the user, would it be considered a bad implementation of the spec to:

Order [0][i], [1][i], ..... [n][i] instead of the other way around so that an entire column ColumnIndex and/or OffsetIndex can be read (for all pages or a consecutive range of pages) in a single i/o read?
Store ColumnIndex struct and OffsetIndex struct for each individual page together, such that a read might be able to get both in one read if OffsetIndex[i][j].offset for a given page is equal to ColumnIndex.offset[i][j] + ColumnIndex[i][j].length (otherwise fetch separately)

Also do you have any test parquet files available that follow this spec and include ColumnIndex and OffsetIndex or a link to a reference implementation?

Really appreciate the help!
Z

legend-hua · 2018-02-08T01:49:21Z

Hi @lekv @poojanilangekar @mkornacker
The feature is very helpful to improve the performance, but I have a question how to use it. I upgrade
parquet-format to 2.4.0 on my spark-hadoop environment, the performance of the parquet isn't improved.

poojanilangekar and others added 3 commits August 31, 2017 15:02

PARQUET-922: ColumnIndex Layout to Support Page Skipping

f983794

Added PageLocation, OffsetIndex and ColumnIndex structures to the parquet.thrift file in order to support secondary indexes in parquet files.

PARQUET-922: Add documentation on page indexes

700cc00

This commit moves the design doc from [1] to a .md file. It removes the actual thrift definitions, which are added to parquet.thrift. [1] https://docs.google.com/document/d/1sBACp8Lbutuj1Zxdowvsrlm8ku4BFxf8U_Do5K2wSO4/edit#

Address review comments on parquet.thrift.

f5bfe55

rdblue reviewed Oct 10, 2017

View reviewed changes

Address more review comments, clarify the description of ColumnIndex

109b20d

julienledem reviewed Oct 10, 2017

View reviewed changes

julienledem mentioned this pull request Oct 10, 2017

PARQUET-922: ColumnIndex Layout to Support Page Skipping #63

Closed

rdblue reviewed Oct 10, 2017

View reviewed changes

Lars Volker added 2 commits October 10, 2017 13:55

More review comments

8e3c533

Added license to PageIndex.md

Addressing review comments from the Parquet sync meeting

8541da7

rdblue requested changes Oct 11, 2017

View reviewed changes

Addressing more comments

da6f648

rdblue reviewed Oct 11, 2017

View reviewed changes

Fix Typo

5e8ea1c

lekv mentioned this pull request Oct 12, 2017

PARQUET-1136: Fix path to parquet.thrift in Makefile #73

Closed

Merge branch 'master' into index

9f79d72

zivanfi reviewed Oct 13, 2017

View reviewed changes

Lars Volker added 2 commits October 13, 2017 15:02

Address comments from Zoltan.

9ea100a

Extend comments in parquet.thrift, remove PageIndex.md

a39bf49

Remove picture

5df2bbc

Revert "Remove picture"

877e14c

This reverts commit 5df2bbc.

Lars Volker added 2 commits October 13, 2017 15:31

Revert "Extend comments in parquet.thrift, remove PageIndex.md"

ebcb33f

This reverts commit a39bf49.

Reinstate PageIndex.md

bbb3670

mkornacker suggested changes Oct 14, 2017

View reviewed changes

Address Marcel's comments.

6897c2b

mkornacker approved these changes Oct 16, 2017

View reviewed changes

Address comments from Marcel and Zoltan.

babb356

asfgit closed this in f1de77d Oct 16, 2017

This was referenced Jun 23, 2024

Add index pages to the format to support efficient page skipping #324

Closed

Check Makefile in Travis build #339

Open

Uh oh!

Conversation

lekv commented Oct 10, 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!

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

julienledem commented Oct 10, 2017

Uh oh!

julienledem commented Oct 10, 2017

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!

rdblue commented Oct 10, 2017

Uh oh!

lekv commented Oct 11, 2017

Uh oh!

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

rdblue commented Oct 11, 2017

Uh oh!

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

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!

lekv commented Oct 10, 2017 •

edited

Loading

lekv commented Oct 13, 2017 •

edited

Loading