Repository navigation
Conversation
costas-db
changed the title
|
@wgtmac Can you take a look at this? Seems important if it's a real issue. @costas-db found some similar issues in other parts of the code as well (#3830, #3831, #3833) |
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
This overall looks good to me. Thanks @costas-db and @divjotarora. I've left some minor comments.
BTW, do you plan to land #3830 / #3831 together with this one, so schemas with dots in names are safe end to end?
| * @param bloomFilter the bloom filter of column values | ||
| */ | ||
| public void addBloomFilter(String column, BloomFilter bloomFilter) { | ||
| addBloomFilterForPath(ColumnPath.fromDotString(column), bloomFilter); |
There was a problem hiding this comment.
fromDotString splits on dots. A caller who passes a name that contains a dot (for example the top-level a.b column this PR fixes) now builds a path that does not match the column, so their Bloom filter is dropped without any error or log. The javadoc above still says "the column name".
Perhaps we need to deprecate this method and add a comment to point callers at addBloomFilterForPath?
| 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")) { |
There was a problem hiding this comment.
The expected file repeats the values built two lines above, so there are two copies of the same expectations. Let's remove this resource file to make it easier to maintain.
3 participants
Rationale for this change
Parquet column paths are component-based, but Bloom filters were stored under dot-string keys. A top-level field named
a.b(["a.b"]) therefore collided with nested fielda.b(["a", "b"]), allowing one column’s Bloom filter to overwrite the other.What changes are included in this PR?
ColumnPathrather than flattened strings.addBloomFilter(String, …)API and addsaddBloomFilterForPath(ColumnPath, …)for structured callers.ParquetRewriter.Are these changes tested?
Before the fix, the focused reproduction reports:
After the fix, both the direct and expected-output tests pass.
Also verified:
TestParquetWriterTestParquetFileWriterParquetRewriterTestspotless:checkapache-rat:checkAre there any user-facing changes?
No on-disk format change. The existing string-based API remains compatible; this adds a component-based API for callers that already have a
ColumnPath.Closes #3826.