Skip to content

PARQUET-319: Define the parquet bloom filter statistics in parquet format - #28

Closed
winningsix wants to merge 6 commits into
apache:masterfrom
winningsix:Parquet-41
Closed

PARQUET-319: Define the parquet bloom filter statistics in parquet format#28
winningsix wants to merge 6 commits into
apache:masterfrom
winningsix:Parquet-41

Conversation

@winningsix

Copy link
Copy Markdown

This is the change for defining the bloom filter in parquet format.

Comment thread src/thrift/parquet.thrift Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why have an extra boolean? Why not just check whether blomm_filter_stats exists?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

agreed. I would suggest the following:
5: optional BloomFilter bloom_filter;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The extra boolean is removed in the updated PR. Thank you!

@julienledem

Copy link
Copy Markdown
Member

Thanks @winningsix
It is important to have a formal spec for this.
Could you add a description of the bloom filter format (or references to external doc) in a file Statistics.md like we have Encodings.md and LogicalTypes.md ?

@winningsix

Copy link
Copy Markdown
Author

@julienledem Thank you for your review. I have add the document for statistics in the updated PR.

@danielcweeks

Copy link
Copy Markdown

I think we need to add something that would allow multiple strategies. This assumes that there will only be one which does not allow for evolution or specialization.

@winningsix

Copy link
Copy Markdown
Author

@danielcweeks

Copy link
Copy Markdown

MURMUR128_MITZ_32 and MURMUR128_MITZ_64 are two examples, but there are many possible implementations. I think the format should have a field to identify strategy in case someone wants a different implementation. Without the field, you could have possible issues with different readers and writers trying to check the filter using the wrong strategy, which would result in incorrect results.

Comment thread src/thrift/parquet.thrift Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you remove all of the whitespace changes? Since this is the file format definition, it's more important to keep modifications to a minimum so that we can trace back changes through version control.

@winningsix
winningsix force-pushed the Parquet-41 branch 2 times, most recently from 100cab5 to 9500dd7 Compare June 26, 2015 07:01
Comment thread src/thrift/parquet.thrift Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rather than referring to a paper, this should explain what MURMUR128_32 means. We can provide more detail in the Statistics.md page, but I don't want the spec to depend on reading this paper to know how to build or use the bloom filter.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1 a formal definition of how to create the hash functions would be great.

@winningsix winningsix changed the title PARQUET-41: Add bloom filters to parquet statistics PARQUET-319: Define the parquet bloom filter statistics in parquet format Jun 29, 2015
@winningsix

Copy link
Copy Markdown
Author

Thanks @rdblue for your review. I'd like to update the PR after our design review process completes.

Comment thread src/thrift/parquet.thrift Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should this be called HashingStrategy or BloomFilterHashingStrategy?

@winningsix

Copy link
Copy Markdown
Author

@julienledem @rdblue I have updated the PR about bloom filter. I am thinking about whether we can add document as follow up task. So we can focus on the code part and move it forwards faster. Also the CI will be happy with my another PR(apache/parquet-java#215) since it depends on this. Please let me know what's your thought.

@winningsix

Copy link
Copy Markdown
Author

@rdblue could you help me review this PR? Thank you!

@rdblue

rdblue commented May 25, 2017

Copy link
Copy Markdown
Contributor

To be included in the format, this needs to be written up as a specification rather than docs. The current docs don't include a lot of the implementation details, are unclear, and include a strategy that is not portable across processors.

I'd like to see this written up in a google doc, like the Parquet index proposal. This should cover:

  • Goals, non-goals
  • Specific changes that are proposed to the format
    • Bloom filter additions to thrift
    • Where bloom filter bytes are stored and why
  • The algorithm for producing bloom filters
    • Required hash algorithm to produce 32-bit hashes, which must be portable
    • Details about the use of two 32-bit hashes

The proposal doesn't need to cover integration with the Parquet MR or CPP libraries. Configuration for FPP and bloom filter size, when to store bloom filters, etc. are concerns that are orthogonal to actually storing the bits in the file format.

@winningsix, could you start a proposal? I think that is going to be the best way forward, so we can discuss the details and get an unambiguous description of the bloom filter spec.

@winningsix

Copy link
Copy Markdown
Author

Hi @rdblue Thank you for the suggestion. I will prepare for a proposal. BTW, I am now in SF and we can have a F2F meeting if needed.

@chenjunjiedada

Copy link
Copy Markdown
Contributor

Hi @rdblue
Here is a initial version of specification for the Parquet Bloom Filter. Please help to review it.

@rdblue

rdblue commented Jun 5, 2017

Copy link
Copy Markdown
Contributor

Thanks, @cjjnjust and @winningsix! I'll take a look at it.

@julienledem, you may be interested in the draft spec as well.

@rdblue

rdblue commented Jun 5, 2017

Copy link
Copy Markdown
Contributor

Could you open the document for comments?

@winningsix

Copy link
Copy Markdown
Author

@rdblue Open for comments now.

@chenjunjiedada

Copy link
Copy Markdown
Contributor

@rdblue , Thanks for quick comments.

@chenjunjiedada

Copy link
Copy Markdown
Contributor

Hi @rdblue , I have update goals and bloom filter placement, do you have any further comments?

@julienledem, could you please help on review the doc?

@rdblue

rdblue commented Jun 12, 2017

Copy link
Copy Markdown
Contributor

@cjjnjust, I see comments that have been marked as resolved, but really haven't been. For example, the comment about where the bloom filter should be located in the file: the doc needs to propose a more appropriate location than in stats, which are in page-level structures and in the footer. I don't think either location is what you intend, plus we want to keep the footer metadata small. Could you restore the comments that haven't been resolved?

@chenjunjiedada

Copy link
Copy Markdown
Contributor

@rdblue , I 'm not very familiar with google doc and may did wrong operation. I just check comments you left, the discussion for bloom filter location still there and not marked as resolved, but not show on the right of doc, maybe because I delete the highlight keyword. Anyway I will reopen comments that still need to be address, and feel free to add new comment if it can't show on right side.

About the bloom filter location, actually I have a revision (at June 7. 11:39 AM) to put them as a new page while it may need additional IO so I reverted it back, it still need more advice.

@chenjunjiedada

Copy link
Copy Markdown
Contributor

Hi @rdblue , I restored spec to previous version which store bloom filter to new page. Could you please help to review this?

@chenjunjiedada

Copy link
Copy Markdown
Contributor

@rdblue Any update? @danielcweeks Could you please also have a look?

@rdblue

rdblue commented Jun 28, 2017

Copy link
Copy Markdown
Contributor

The blocker here is that the proposal is missing key parts, like what hash algorithm should be used as well as where and how bloom filters will be stored in the file. Getting those details right is the purpose of the proposal, so I'd like to see something proposed or discussed. If you don't know where they should go, then try sending an e-mail to the dev list to discuss. Be sure you include options for where you might put bloom filters and some thoughts about the pros and cons of each option.

lekv pushed a commit to lekv/parquet-format that referenced this pull request Jul 31, 2017
Author: Wes McKinney <wes@cloudera.com>

Closes apache#28 from wesm/PARQUET-439 and squashes the following commits:

7c0712b [Wes McKinney] PARQUET-439: Conform copyright headers to ASF requirements
@winningsix

Copy link
Copy Markdown
Author

Deprecated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants