Fail loudly with corrupted Parquet statistics

[zhenxiao]

@nezihyigitbasi @dain @findepi @qqibrow

[electrum]

Overall this looks good to me. Something to consider, separately, is adding a data source ID like we have in OrcCorruptionException. When you have corruption, it's really useful to know exactly which file is bad.

[electrum]

Remove the assertEquals() since it should never be called, as we expect the test to throw.

[electrum]

This is fine, but you can make testing for exceptions nicer using AssertJ: http://joel-costigliola.github.io/assertj/assertj-core-features-highlight.html#exception-assertion

[electrum]

Same comments

[electrum]

This catch block seems to be in the wrong place, since this is catching an exception from the close() call. I think this needs to be below as a (e instanceof ParquetCorruptionException) check, similar to the check for PrestoException.

[zhenxiao]

thank you, @electrum
I get comments addressed. will do following task to add dataSourceID

[nezihyigitbasi]

In addition to knowing which file is bad, it would be easier to debug if we know which column's stats is corrupted. By looking at the stack trace of the exception we can see for which type we have corrupted stats, but if we have many columns of that type then that's not useful. In short, an exception message like: Corrupted statistics for column %s in Parquet file %s would be pretty useful.

[nezihyigitbasi]

There is also a risk of breaking user queries that scan Parquet files with corrupted stats. Do we want to have a flag to revert to the current behavior just in case (which we can eventually remove and make this new behavior the default behavior)?

[findepi]

Do we want to have a flag to revert to the current behavior just in case (which we can eventually remove and make this new behavior the default behavior)?

@nezihyigitbasi IMO we should fail on corrupted stats by default -- unless some widely used Parquet writing program is notorious for writing corrupted stats.
And yes, we should have safety flag to disable the check, at least for some transition period.

[zhenxiao]

@nezihyigitbasi @findepi @electrum thanks ur comments
get comments addressed, added dataSourceId for Parquet, added configuration to control whether fail query when scan Parquet file with corrupted statistics. During transition period, disable the check, we could enable it, and probably remove the config, to fail whenever hitting corrupted statistics

[nezihyigitbasi]

IMO we should fail on corrupted stats by default -- unless some widely used Parquet writing program is notorious for writing corrupted stats.

@findepi My comment was confusing probably, sorry. I totally agree with you. We should fail on corrupt stats by default. @zhenxiao looks like this PR sets the flag to false by default btw.

[zhenxiao]

oh, OK. Get it updated. by default, will fail on corrupted statistics.

[nezihyigitbasi]

Thanks @zhenxiao. I made a quick pass and left some comments.

• Please squash the commits (I don't think we need to have a separate commit for the config flag).
• I see tests for failing loudly with corrupted stats. But, I didn't see tests for the case where we want to ignore corrupted stats, please add some tests for that.

[nezihyigitbasi]

Fail when scanning Parquet files with corrupted statistics

[nezihyigitbasi]

"Fail when scanning Parquet files with corrupted statistics"

[nezihyigitbasi]

Don't need the toString call.

[nezihyigitbasi]

when scanning a Parquet file

[nezihyigitbasi]

You can create a method like this one (feel free to find a better name) to simplify & get rid of repetition:

private static void failWithCorruptionException(boolean failOnCorruptedParquetStatistics, String column, ParquetDataSourceId id, Object min, Object max)
{
    if (failOnCorruptedParquetStatistics) {
        throw new ParquetCorruptionException(format("Corrupted statistics for column %s in Parquet file %s: min %s, max %s", column, id, min, max));
    }
}

Then calls will look like:

failWithCorruptionException(failOnCorruptedParquetStatistics, column, id, intStatistics.genericGetMin(), intStatistics.genericGetMax());

[zhenxiao]

thank you @nezihyigitbasi
I get comments addressed, squash into one commit, added testcases for ignore and fail on corrupted statistics

[nezihyigitbasi]

@zhenxiao thanks for addressing the comments. I think in general this looks good. The only important issue to address is the unit tests. We only have unit tests for the float and date types for testing this new fail/ignore logic. I think we should have a unit test that tests the fail/ignore logic for all types.

[nezihyigitbasi]

I think it makes sense to keep this as IOException.

[zhenxiao]

OK, will keep it

[nezihyigitbasi]

I hope all children of Statistics type have toString implemented.

[nezihyigitbasi]

One minor change we can make to the error message is to add quotes/brackets to set the boundaries of the column/file name and stats information:

Corrupted statistics for column "column_name" in Parquet file "file_name": [stats_string]

Currently it looks like the following, which is a little bit hard to follow:

Corrupted statistics for column DateColumn in Parquet file testFile: min: 200, max: 100, num_nulls: 0

[zhenxiao]

yes, Statistics has a toString() abstract method
sure, will add quotes and brackets

[zhenxiao]

thank you, @nezihyigitbasi

[zhenxiao]

yes, Statistics has a toString() abstract method
sure, will add quotes and brackets

[zhenxiao]

OK, will keep it

[zhenxiao]

thank you, @nezihyigitbasi
comments addressed, added testcases for all types

[nezihyigitbasi]

LGTM % a minor comment.

@dain @electrum @findepi please let @zhenxiao know if you have any comments.

[nezihyigitbasi]

Instead of relying on the error message, can't we just do:

if (e instanceof ParquetCorruptionException)
    throw new PrestoException(HIVE_BAD_DATA, e);
}

[zhenxiao]

thank you, @nezihyigitbasi
get comments addressed
@dain @electrum @findepi your comments are welcome

[nezihyigitbasi]

Thanks @zhenxiao, merged.