wgtmac commented on code in PR #1020:
URL: https://github.com/apache/parquet-mr/pull/1020#discussion_r1070546933
##########
parquet-column/src/test/java/org/apache/parquet/column/values/bloomfilter/TestBlockSplitBloomFilter.java:
##########
@@ -181,6 +182,60 @@ public void testBloomFilterNDVs(){
assertTrue(bytes < 5 * 1024 * 1024);
}
+ @Test
+ public void testMergeBloomFilter() throws IOException {
+ Random random = new Random();
+ int numBytes = BlockSplitBloomFilter.optimalNumOfBits(1024 * 1024, 0.01) /
8;
+ BloomFilter otherBloomFilter = new BlockSplitBloomFilter(numBytes);
+ BloomFilter mergedBloomFilter = new BlockSplitBloomFilter(numBytes);
+
+ Set<String> originStrings = new HashSet<>();
+ Set<String> testStrings = new HashSet<>();
+ Set<Integer> testInts = new HashSet<>();
+ Set<Double> testDoubles = new HashSet<>();
+ Set<Float> testFloats = new HashSet<>();
+ for (int i = 0; i < 1024; i++) {
+
+ String originStrValue = RandomStringUtils.randomAlphabetic(1, 64);
+ originStrings.add(originStrValue);
+
mergedBloomFilter.insertHash(otherBloomFilter.hash(Binary.fromString(originStrValue)));
+
+ String testString = RandomStringUtils.randomAlphabetic(1, 64);
+ testStrings.add(testString);
+
otherBloomFilter.insertHash(otherBloomFilter.hash(Binary.fromString(testString)));
+
+ int testInt = random.nextInt();
+ testInts.add(testInt);
+ otherBloomFilter.insertHash(otherBloomFilter.hash(testInt));
+
+ double testDouble = random.nextDouble();
+ testDoubles.add(testDouble);
+ otherBloomFilter.insertHash(otherBloomFilter.hash(testDouble));
+
+ float testFloat = random.nextFloat();
+ testFloats.add(testFloat);
+ otherBloomFilter.insertHash(otherBloomFilter.hash(testFloat));
+ }
+ mergedBloomFilter.merge(otherBloomFilter);
+ for (String testString : originStrings) {
+
assertTrue(mergedBloomFilter.findHash(mergedBloomFilter.hash(Binary.fromString(testString))));
Review Comment:
Can you add some cases like below to have more coverage:
- value that is tested false on the original BF but true on the other side,
and vice versa?
- value that is tested false on both sides.
- value that is tested true on both sides.
##########
parquet-column/src/main/java/org/apache/parquet/column/values/bloomfilter/BlockSplitBloomFilter.java:
##########
@@ -394,4 +395,21 @@ public long hash(float value) {
public long hash(Binary value) {
return hashFunction.hashBytes(value.getBytes());
}
+
+ @Override
+ public void merge(BloomFilter otherBloomFilter) throws IOException {
+ Preconditions.checkArgument((otherBloomFilter.getAlgorithm() ==
getAlgorithm()),
Review Comment:
It would be more user-friendly to print the mismatched values on both sides.
##########
parquet-column/src/main/java/org/apache/parquet/column/values/bloomfilter/BloomFilter.java:
##########
@@ -176,4 +176,13 @@ public String toString() {
* @return compress algorithm that the bloom filter apply
*/
Compression getCompression();
+
+ /**
+ * Combines this Bloom filter with another Bloom filter by performing a
bitwise OR of the underlying bits
+ *
+ * @param otherBloomFilter The Bloom filter to combine this Bloom filter
with.
+ */
+ default void merge(BloomFilter otherBloomFilter) throws IOException {
+ throw new UnsupportedOperationException("Not supported merge operation.");
Review Comment:
```suggestion
throw new UnsupportedOperationException("Merge is not implemented.");
```
##########
parquet-column/src/test/java/org/apache/parquet/column/values/bloomfilter/TestBlockSplitBloomFilter.java:
##########
@@ -181,6 +182,60 @@ public void testBloomFilterNDVs(){
assertTrue(bytes < 5 * 1024 * 1024);
}
+ @Test
+ public void testMergeBloomFilter() throws IOException {
+ Random random = new Random();
+ int numBytes = BlockSplitBloomFilter.optimalNumOfBits(1024 * 1024, 0.01) /
8;
+ BloomFilter otherBloomFilter = new BlockSplitBloomFilter(numBytes);
+ BloomFilter mergedBloomFilter = new BlockSplitBloomFilter(numBytes);
+
+ Set<String> originStrings = new HashSet<>();
Review Comment:
Better to split different types into separate test cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@parquet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
