Refactor HiveStatisticsProvider and statistics SPI

[arhimondr]

No description provided.

[kokosing]

Just skimmed so far

[kokosing]

Maybe you could extract to some Preconditions class in spi?

[arhimondr]

We got a lot of checkArgument and vefiry methods implementation across the presto-spi module. I think that a good idea to have a Precondition class in that module to avoid implementing this boilerplate code every time someone needs that. However to keep it consistent i would rather like to do this as a separate PR, and i would like to replace the usages everywhere.

[kokosing]

s/create/estimate

then you could static import this

[arhimondr]

Although i agree that createObjectName sounds more fluent that ObectName.create, i can see some disadvantages in the createObjectName approach. Here some of them:

• Additional (static) import introduces additional merge conflicts
• It is not always clear what is the exact name of the factory method. For example taking a class called DummyFunnyCircle, it is not always obvious how's the factory method is called. createDummyFunnyCircle? createFunnyCircle? createCircle?
• Static import may introduce a name clash. For example in test classes you may want to have a private factory method called createFunnyCircle(some parameters). In that method you have no other way, but use DummyFunnyCircle.createFunnyCircle() without static import. Using it without static import is tedious, and tautological.

I can agree that these arguments are not very strong. But still, i think it is better to follow some existing conventions:

• https://docs.oracle.com/javase/tutorial/datetime/overview/naming.html
• http://www.informit.com/articles/article.aspx?p=1216151 (check the last paragraph)

Considering that we are broadly using Optional and Guava's ImmutableList, ImmutableMap, Immutable*... i think that it is more consistent to use similar convention when naming the factory methods. Just because people are more used to it.

I'm going to rename Estimate.create to Estimate.of to follow the Optional convention.

Also i think we should adopt this convention all across the code.

For example:

• Some neutral, "empty" value must be expressed as Something.empty(), and not EMPTY_SOMETHING, SOMETHING_EMPTY, emptySomething(), so on.
• Static factory methods should follow this convention: https://docs.oracle.com/javase/tutorial/datetime/overview/naming.html
• Builder constructors should look like Something.builder(), and not bulderOfSomething or similar.

Ideally it would be nice to have a static rule that checks it. But i don't think it is easily implementable.

CC: @electrum @martint

[kokosing]

emptyStatistics? That way you could static import this.

[arhimondr]

Ditto. e.g. Optional.empty()

[kokosing]

what happened to timestamp?

[arhimondr]

It is not supported by the optimizer. I found it to be misleading, that's why i decided to remove it until we support it in the optimizer.

[kokosing]

just create?

[arhimondr]

This is a transitional method that is removed in the very next commit. I just moved it to the MetastoreHiveStatisticsProvider, since this is the only class that uses it.

[kokosing]

toStringLiteral?

[kokosing]

why it differs?

[arhimondr]

This value is out of range of the values, precisely represented by double. Before, when the value was transferred as Object it was truncated when converting to SymbolStatsEstimate. But now the truncation is visible to the end user.

[kokosing]

explain in commit message what you change and motivation behind this

[mbasmanova]

@arhimondr

Remove RangeColumnStatistics

[mbasmanova]

consider using MoreObjects#toStringHelper

[arhimondr]

Unfortunately Guava is not available in the presto-spi module

[mbasmanova]

Perhaps, remove range from the message. Here and in 2 more places further down.

[mbasmanova]

Perhaps, add a check for nonNullRowsCount being 0?

[mbasmanova]

@arhimondr

Refactor Estimate

[mbasmanova]

Add TableStatistics#empty() method

[mbasmanova]

Do we need isEmpty() method?

[arhimondr]

No, i don't think we ever check it for empty explicitly.

[mbasmanova]

Refactor TableStatistics

[mbasmanova]

Perhaps, use MoreObjects.toStringHelper

[arhimondr]

It is not available in presto-spi

[mbasmanova]

Add ignore_corrupted_statistics session property

LGTM

[mbasmanova]

Extract parsePartition in PartitionManager

[mbasmanova]

s/partitionId/partitionName for consistency with partitionName parameter in extractPartitionKeyValues method

[mbasmanova]

s/keys/values - extractPartitionKeyValues name is confusing; it suggests that the return value is a key-value maps of partition column names and values; in fact, the return value is a list of values of partition columns

[mbasmanova]

This code assumes that partition_name lists partition columns in proper order, however, extractPartitionKeyValues doesn't check for that; should we have an explicit check?

[arhimondr]

The partitionName is something very confusing. Although all the column names are listed, and technically it is possible to shuffle the column names without loosing any information. However it is not legit. The partition values in the partition name should always be in order of partition columns.

[mbasmanova]

Make getPartitionsSample deterministic

LGTM

[mbasmanova]

Print column statistics in deterministic manner

Could you update commit message to clarify that this change applies to SHOW STATS command?

[mbasmanova]

Shouldn't we use a List to preserve the order of columns as defined in a table?

[arhimondr]

We rely that the Map is ordered. Unfortunately this is something we rely on in the other parts of the code.

[mbasmanova]

@arhimondr Not sure I understand this completely. A more robust approach could be to fetch column list from ConnectorTableMetadata#columns.

[mbasmanova]

Could you add a test for a case when some columns (e.g. partition columns, maybe) don't have stats?

[arhimondr]

That's one of the reasons why am i changing this. Before we were always assume that the TableStatistics object contains the statistics for all the columns. And it was true before the refactor. After refactor it changes. So it is implicitly tested by the existing tests.

[mbasmanova]

Remove validation from HiveBasicStatistics

LGTM

[mbasmanova]

Represent min and max statistics in SPI as double

[mbasmanova]

MoreObjects.toStringHelper

[findepi]

@mbasmanova in SPI we don't have Guava dependency (and don't want to have it)

[mbasmanova]

@findepi Piotr, thanks for explaining. In this case, let's copy-paste most useful utilities like this one.

[findepi]

@mbasmanova We didn't do this yet, because we didn't want to add publicly accessible classes not intended for public consumption and package private didn't make it, as there as subpackages in SPI... Of course, this is something we can revisit anytime. I am all for it.

[electrum]

This is something we can explore after moving to Java 9 modules, as that will allow us to have module-private cross-package utility code.

[mbasmanova]

The name Range a bit too generic. Would you consider changing to DoubleRange. BTW, commons-lang also has DoubleRange class.

[electrum]

We also already have com.facebook.presto.spi.predicate.Range

[arhimondr]

com.facebook.presto.spi.predicate.Range is too generic. I gonna go with DoubleRange

[mbasmanova]

nit: perhaps, test the resulting object as well?

private static void assertRange(double min, double max)
{
    Range range = new Range(min, max);
    assertEquals(range.getMin(), min);
    assertEquals(range.getMax(), max);
}

[mbasmanova]

Would you update the commit message to explicitly mention this change?

[findepi]

This should be a separate commit.

[arhimondr]

Extracted to a separate commit

[findepi]

i just skimmed selected commits
@arhimondr apologies that i didn't do proper review you asked me for :/

[findepi]

cmt msg

If corruption is detected and the session property is set to true,
the statistics provider logs the corruption details and returns empty statistics.

this isn't true yet, since no-one reads the new flag. Maybe squash this commit with the usage? Or amend the cmt msg to reflect this?

[arhimondr]

I'm just going to squash it

[findepi]

cmt msg:

Make getPartitionsSample deterministic

... but the test added here doesn't look like testing the determinism.
(the previous tests look more like it... but probably were insufficient)

[arhimondr]

I don't know why did i add it. Let me remove it.

[findepi]

Sure. Anyway, would it be possible to cover Make getPartitionsSample deterministic with some kind of tests?

[arhimondr]

I see what is going on. Accidentally the test that verifies determinism went as part of the Extract parsePartition in PartitionManager commit. Let me move it here.

[findepi]

This should be a separate commit.

[findepi]

"ToDouble" doesn't convey the intended semantics; convertPrestoValueToStatsRepresentation

[findepi]

nit: express in ctor params order:

if (min > max) {
            throw new IllegalArgumentException(format("min must be lower than or equal to max. min: %s. max: %s.", min, max));

[electrum]

Or even simpler

format("min (%s) cannot be larger than max (%s)", min, max)

[arhimondr]

@electrum I don't like to mix the values with the static part of an error message, as it is harder than to search for it in the codebase. If you don't mind I would like to leave it as is.

[findepi]

@mbasmanova in SPI we don't have Guava dependency (and don't want to have it)

[findepi]

?

[arhimondr]

Let me remove it. Currently all the representation that are allowed for Presto types are equatable

[findepi]

cmt msg Normalize distinct values count:

s/or/of in than a total or non-null rows count

[findepi]

"produced by the HLL"

or anything else, depending who wrote those stats (Hive, Spark, Impala, ...). Does Hive use HLL?

[arhimondr]

Does Hive use HLL?

Yes, by default. There some other, legacy sketch it can use.

Let me remove the HLL

[mbasmanova]

Refactor MetastoreHiveStatisticsProvider

@arhimondr I'm still looking, but want to share comments I made before the code changed.

[mbasmanova]

perhaps, add checks to avoid creating empty instances using public constructor

[arhimondr]

We should allow creating empty() implicitly. For example if there is no information neither about nullsFraction nor distinctValuesCount nor dataSize nor range in the Metastore.

[mbasmanova]

Why not use Math.min?

[mbasmanova]

perhaps, combine these if statements

[mbasmanova]

I find it confusing to mix and match id and name terms wrt partition. I think name is more widely used term.

[mbasmanova]

Refactor MetastoreHiveStatisticsProvider

Some more questions.

[mbasmanova]

nit: perhaps, add Avg to the name to clarify that this method computes average number of rows

[arhimondr]

renamed to calculateAverageRowsPerPartition. Also renamed all the rowsPerPartition variables to averageRowsPerPartition

[mbasmanova]

Normalize distinct values count

LGTM

[mbasmanova]

@arhimondr Andrii, I finished reviewing this PR. Overall, I like these changes a lot. I have a few questions, though.

[arhimondr]

Comments addressed. Also i added one more tiny commit: Ignore corrupted statistics when altering partition

[mbasmanova]

@arhimondr Andrii, this change overall looks great. I made a few comments, but nothing major. I'm thinking that it would be helpful to document the new constraints that Presto enforces on Metastore stats.

[arhimondr]

@mbasmanova

Would you update the commit message to clarify that this change affects SHOW STATS command?

Done

[arhimondr]

@mbasmanova

Thanks for the review. Comments addressed.

[kokosing]

I am sorry that I was not able to do a proper review. From the discussions we had (online and offline ones) I see no blockers for that to be merged.

[mbasmanova]

@arhimondr Andrii, thanks for making all of these changes. I'm looking forward to seeing fewer queries fail when generating plans with hive.statistics_enabled = true.

[findepi]

I'm thinking that it would be helpful to document the new constraints that Presto enforces on Metastore stats.

@mbasmanova @arhimondr what are the new constraints?

[arhimondr]

@mbasmanova @findepi

I'm thinking that it would be helpful to document the new constraints that Presto enforces on Metastore stats.

The checks introduced are basically very straightforward sanity checks. Like if rowCount is not negative, or if nullsCount is less than or equal to rowCount. I'm not sure if it makes sense to explicitly document it, as it seems to be pretty obvious.

[findepi]

i am fine with not documenting these. However, if we run into real-life use-cases where the assertions do not hold (because of the way some other program populated the stats), we will need to update the code to support that.

[arhimondr]

Aggreed. I'm going add a note to the release note about this change though.

[findepi]

Apparently we have first real-life case of this already -- #11549