costas-db commented on code in PR #3827:
URL: https://github.com/apache/parquet-java/pull/3827#discussion_r4264700138
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileWriter.java:
##########
@@ -1078,6 +1078,16 @@ public void writeDataPage(
* @param bloomFilter the bloom filter of column values
*/
public void addBloomFilter(String column, BloomFilter bloomFilter) {
+ addBloomFilterForPath(ColumnPath.fromDotString(column), bloomFilter);
Review Comment:
Done in
[`18c26d68`](https://github.com/apache/parquet-java/pull/3827/commits/18c26d686d8ce716453b008a100b1aac351437fb).
I deprecated the String overload and updated its Javadoc to point callers to
`addBloomFilterForPath`.
To preserve existing unambiguous callers, the String overload now resolves
the name against the writer schema. If multiple component paths flatten to the
same string, it fails with a clear error instead of guessing. I also checked
the repository for callers—the rewriter uses the component-based API, and only
the tests intentionally exercise the deprecated method.
##########
parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestParquetWriter.java:
##########
@@ -335,6 +337,114 @@ public void testParquetFileWithBloomFilter() throws
IOException {
}
}
+ @Test
+ public void testBloomFiltersKeepCollidingDotStringPathsDistinct() throws
IOException {
+ Map<ColumnPath, BloomFilter> bloomFilters =
writeBloomFiltersForCollidingPaths();
+ BloomFilter topLevelBloom = bloomFilters.get(ColumnPath.get("a.b"));
+ BloomFilter nestedBloom = bloomFilters.get(ColumnPath.get("a", "b"));
+
+ for (int valueId = 0; valueId < 10; valueId++) {
+ long topLevelHash = LongHashFunction.xx(0)
+ .hashBytes(Binary.fromString("top-" + valueId).toByteBuffer());
+ long nestedHash = LongHashFunction.xx(0)
+ .hashBytes(Binary.fromString("nested-" + valueId).toByteBuffer());
+ assertThat(topLevelBloom.findHash(topLevelHash)).isTrue();
+ assertThat(nestedBloom.findHash(nestedHash)).isTrue();
+ }
+ }
+
+ @Test
+ public void testBloomFilterPathCollisionGolden() throws IOException {
+ Map<ColumnPath, BloomFilter> bloomFilters =
writeBloomFiltersForCollidingPaths();
+ String actual = String.format(
+ "{\n"
+ + " \"topLevelPath\" : [ \"a.b\" ],\n"
+ + " \"topLevelContainsOwnBloomValues\" : %s,\n"
+ + " \"nestedPath\" : [ \"a\", \"b\" ],\n"
+ + " \"nestedContainsOwnBloomValues\" : %s\n"
+ + "}\n",
+ containsAllBloomValues(bloomFilters.get(ColumnPath.get("a.b")),
"top-"),
+ containsAllBloomValues(bloomFilters.get(ColumnPath.get("a", "b")),
"nested-"));
+
+ try (InputStream input =
TestParquetWriter.class.getResourceAsStream("/bloom-filter-path-collision.json"))
{
Review Comment:
Removed the JSON resource and the duplicate golden test in
[`18c26d68`](https://github.com/apache/parquet-java/pull/3827/commits/18c26d686d8ce716453b008a100b1aac351437fb).
The remaining direct test now checks that both filters are present and that
the footer stores them at different offsets, so the expectations live in one
place.
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/rewrite/ParquetRewriter.java:
##########
Review Comment:
Added coverage in
[`18c26d68`](https://github.com/apache/parquet-java/pull/3827/commits/18c26d686d8ce716453b008a100b1aac351437fb).
The new test uses the colliding `["a.b"]` and `["a", "b"]` paths and runs the
rewriter both with and without a codec change. That covers the page-rewrite
path through `processChunk` and the direct-copy path through
`appendColumnChunk`, then verifies that both output Bloom filters still match
the input.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
