Skip to content

PARQUET-1660: align Bloom filter implementation with format - #686

Merged
gszadovszky merged 4 commits into
apache:bloom-filterfrom
chenjunjiedada:bloom-filter
Jan 7, 2020
Merged

PARQUET-1660: align Bloom filter implementation with format#686
gszadovszky merged 4 commits into
apache:bloom-filterfrom
chenjunjiedada:bloom-filter

Conversation

@chenjunjiedada

@chenjunjiedadachenjunjiedada commented Sep 26, 2019

Copy link
Copy Markdown
Contributor

Make sure you have checked all steps below.

Jira

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

jbapple
jbapple reviewed Oct 5, 2019
View reviewed changes
Comment thread...column/src/main/java/org/apache/parquet/column/values/bloomfilter/BlockSplitBloomFilter.java Outdated
int bucketIndex = (int)(hash >> 32) & (bitset.length / BYTES_PER_BLOCK - 1);
long numBlocks = bitset.length / BYTES_PER_BLOCK;
long lowHash = hash >>> 32;
int blockIndex = (int)(lowHash * numBlocks >> 32);

@jbapplejbappleOct 5, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What happens if this product overflows? How does that behavior compare to this line operating on unsigned values in C++, which cannot overflow on multiplication?

@chenjunjiedadachenjunjiedadaOct 5, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The number of blocks right shift 5 bits at first, so its value should be less than 1<<27 and the overflow should not happen here.

@jbapple

jbapple commented Dec 16, 2019

Copy link
Copy Markdown

LGTM.

Fokko
Fokko approved these changes Dec 21, 2019
View reviewed changes

@FokkoFokko left a comment

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.

LGTM, do you already use this internally?

Comment threadparquet-column/src/main/java/org/apache/parquet/column/ParquetProperties.java Outdated
return bloomFilterExpectedDistinctNumbers;
}

public Set<String> getBloomFilterColumns() {return bloomFilterColumns;}

@FokkoFokkoDec 21, 2019

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.

Can we put the return on the next line, similar to getMaxBloomFilterBytes

Comment threadparquet-column/src/main/java/org/apache/parquet/column/ParquetProperties.java
Comment thread...column/src/main/java/org/apache/parquet/column/values/bloomfilter/BlockSplitBloomFilter.java
@chenjunjiedada

chenjunjiedada commented Dec 28, 2019

Copy link
Copy Markdown
ContributorAuthor

@Fokko, @gszadovszky, could you help to have another look? Is it close to merging?

@chenjunjiedada

chenjunjiedada commented Jan 2, 2020

Copy link
Copy Markdown
ContributorAuthor

@Fokko, forgot your last question. Yes, we already use it internally in some cases.

gszadovszky
gszadovszky requested changes Jan 6, 2020
View reviewed changes
Comment threadparquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetOutputFormat.java Outdated
Comment threadparquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetWriter.java Outdated
@chenjunjiedada

chenjunjiedada commented Jan 7, 2020

Copy link
Copy Markdown
ContributorAuthor

@gszadovszky, I updated the code, would you please take another look?

@chenjunjiedada

chenjunjiedada commented Jan 7, 2020

Copy link
Copy Markdown
ContributorAuthor

@gszadovszky , thanks for your review. I'd like to rebase this to master before merging. Maybe it needs your another look again. Thanks in advance!

@chenjunjiedada
chenjunjiedadaforce-pushed the bloom-filter branch 2 times, most recently from 81dd09b to 039ffdcCompareJanuary 7, 2020 09:15
@chenjunjiedada

chenjunjiedada commented Jan 7, 2020

Copy link
Copy Markdown
ContributorAuthor

Looks like I need to merge instead of rebasing. Let me fix this. Sorry for confusing.

@chenjunjiedada

chenjunjiedada commented Jan 7, 2020

Copy link
Copy Markdown
ContributorAuthor

@gszadovszky@Fokko, I just realized that you may need squashing for this PR, so it would be better to submit a separated PR for merging master. So please help to merge this in your convenience.

@Fokko

Fokko commented Jan 7, 2020

Copy link
Copy Markdown
Contributor

So you'll create a new PR from bloom-filter to master? I'm fine with that. @gszadovszky WDYT?

@chenjunjiedada

chenjunjiedada commented Jan 7, 2020

Copy link
Copy Markdown
ContributorAuthor

@Fokko, I may put in the wrong way, but it is a PR to merge master to bloom-filter branch. I have done that job local machine and can submit that if you prefer to squash one more merging PR.

@gszadovszky

gszadovszky commented Jan 7, 2020

Copy link
Copy Markdown
Contributor

Let's squash+merge this PR to the feature branch first. Then, check the merge PR and push it to the feature branch as well.
If everything is ready, create a PR from the feature branch to master. There shall be no conflicts. After the final review/tests succeed squash+merge the whole to master.

@chenjunjiedada

chenjunjiedada commented Jan 7, 2020

Copy link
Copy Markdown
ContributorAuthor

SGTM

@gszadovszky
gszadovszky merged commit ba28686 into apache:bloom-filterJan 7, 2020
gszadovszky pushed a commit that referenced this pull request Feb 26, 2020
@chenjunjiedada
PARQUET-41: Add bloom filter (#757)
806037c
* PARQUET-1328: Add Bloom filter reader and writer (#587)
* PARQUET-1516: Store Bloom filters near to footer (#608)
* PARQUET-1391: Integrate Bloom filter logic (#619)
* PARQUET-1660: align Bloom filter implementation with format (#686)
shangxinli pushed a commit to shangxinli/parquet-mr that referenced this pull request Mar 1, 2020
@chenjunjiedada@shangxinli
PARQUET-41: Add bloom filter (apache#757)
3c77510
* PARQUET-1328: Add Bloom filter reader and writer (apache#587)
* PARQUET-1516: Store Bloom filters near to footer (apache#608)
* PARQUET-1391: Integrate Bloom filter logic (apache#619)
* PARQUET-1660: align Bloom filter implementation with format (apache#686)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FokkoFokkoFokko approved these changes
@gszadovszkygszadovszkygszadovszky approved these changes
+1 more reviewer
@jbapplejbapplejbapple left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chenjunjiedada@jbapple@Fokko@gszadovszky