Core, Parquet: Fix null counting for parquet that doesn't have null_count stats - #17557
Conversation
|
Issue #17558 |
anoopj
left a comment
There was a problem hiding this comment.
The core code change looks good to me. This is an important correctness fix - thanks for catching this!
I have a couple of comments on tests.
singhpk234
left a comment
There was a problem hiding this comment.
Thanks for fix @xndai, I added this to 1.12 milestone too keep track of it
| // without accounting for the -1 sentinel, this sums to an incorrect null count of 0 | ||
| Metrics metrics = metrics(block(statsWithoutNullCount(1, 10), 10), block(stats(20, 30, 1), 10)); | ||
|
|
||
| assertThat(metrics.nullValueCounts()).doesNotContainKey(1); |
There was a problem hiding this comment.
should we instead assert what the val returned is ? -1 right ?
There was a problem hiding this comment.
The -1 is returned from Statistics.getNumNulls() as an internal sentinel to represent unknown state. But when the null count is stored into Metrics, -1 is dropped so here we assert the null value count is unavailable in the map.
|
@singhpk234 can you please review again? |
RussellSpitzer
left a comment
There was a problem hiding this comment.
This looks good to me but I have a few blockers on the test layout. Let's fix that up and we can merge this in
|
@xndai do you have some time to address the pending comments. It would be great to get this in the release. |
…ount stats Parquet's Statistics#getNumNulls returns -1 when null_count is missing from the stats. But our current metrics logic doesn't handle this -1 value specifically, instead it just adds to the existing count. For a single row group the -1 total was dropped because Metrics only keeps non-negative counts. But say we have row group 0 that has one null value and row group 1 doesn't have null_count stats and return -1, we end up counting total null count as 0, which is wrong. A query engine that relys on total null count being 0, can skip the file completely when evaluating predicate like `WHERE c IS NULL` causing wrong results. Fix by checking the -1 when sum the nulls. Add corresponding tests.
Address review feedback: the manifest round-trip test does not need the filesystem. Use InMemoryOutputFile and InMemoryFileIO instead of a temp directory, following TestVariantMetrics.
Address review feedback: the variant metrics paths handle a missing null_count too, but had no test for it. Add cases that build a two row group footer where one row group omits the variant column's null_count, covering both the footer null count path and the all-null-variant path that uses the value count instead.
Address review feedback: follow the convention of omitting the redundant "test" prefix on test method names.
Address review feedback: relocate the missing null_count tests into TestParquet, alongside testMetricsMissingColumnStatisticsInRowGroups which has the same shape. Drop knownNullCountsAreSummed and missingNullCountIsNotWrittenToManifest, which duplicate coverage in TestMetrics and TestManifestWriterVersions, and remove the now-empty TestParquetMissingNullCount class.
111d68b to
4c7433d
Compare
fixed. |
thanks for review. I see this PR is approved. I will send out a small follow up PR to address your comments. |
|
Please address @nssalian's comments if they are valid. We try to get consensus before merging especially for folks who have been reviewing the PR since the beginning. If you think they aren't worth addressing we can close them, but if they are then let's just patch them right now. |
nssalian
left a comment
There was a problem hiding this comment.
Should have mentioned those are non-blocking mostly cleanup related. Everything else does look good. @xndai if you want to clean it up here that works just want to make sure we are not leaving the tests with a lot of duplication. In the past I've cleaned up a bunch of the variant related tests as we go.
Address review feedback: extract a shared writeVariants helper so the variant write helpers no longer repeat the Parquet.write build and record loop, and tighten a comment on the shredded null-count test.
|
Merged, Thanks @xndai for the PR and @anoopj , @nssalian , @uros-b , @nandorKollar for reviewing! |
Parquet's Statistics#getNumNulls returns -1 when null_count is missing from the stats. But our current metrics logic doesn't handle this -1 value specifically, instead it just adds to the existing count.
For a single row group the -1 total was dropped because Metrics only keeps non-negative counts. But say we have row group 0 that has one null value and row group 1 doesn't have null_count stats and return -1, we end up counting total null count as 0, which is wrong. A query engine that relays on total null count being 0, can skip the file completely when evaluating predicate like
WHERE c IS NULLcausing wrong results.Fix by checking the -1 when sum the nulls. Add corresponding tests.