Lazily load hive partition information

[Praveen2112]

No description provided.

[Praveen2112]

@findepi @sopel39 AC

[Praveen2112]

@sopel39 AC

[sopel39]

It's odd that we don't need to filter partition names while we had to do .filter(partition -> partitionMatches(partitionColumns, effectivePredicate, predicate, partition)) above.

Comment would be nice

[sopel39]

ping, I still don't understand why we don't have to filter here.
Why there can be partition names and not getPartitions?

[Praveen2112]

partitionMatches effectively converts the partition names from String to HivePartitionInformation and then tries to match with the filter predicate.

Why there can be partition names and not getPartitions?
The contract is that a table can either load a partition details (if it is less than a threshold) or could maintain it as raw String if it crosses a threshold. We don't have an intermediate state here

[sopel39]

partitionMatches effectively converts the partition names from String to HivePartitionInformation and then tries to match with the filter predicate.

So in this branch:

partitionNames = hiveTableHandle.getPartitionNames().get();

we don't do any filtering. Does it mean we return excessive partitionNames?

[Praveen2112]

we don't do any filtering.

We do kind of partitial filterting, like if we have a TupleDomain (with Domain on Partition columns) then the filtering would be applied at a metastore layer but we won't perform partitionMatches (and materializing it into HivePartition)

Does it mean we return excessive partitionNames?

Since we dont invoke partitionMatches there is a chance that we could return excessive partition names.

[sopel39]

Since we dont invoke partitionMatches there is a chance that we could return excessive partition names.

That doesn't seem correct, does it?

[Praveen2112]

I think in this case we don't have specify the enforcedTupleDomain as TupleDomain so that the filter expression is not lost. It will be applied during the next applyFilter optimizer. WDYT ?

[sopel39]

I think in this case we don't have specify the enforcedTupleDomain as TupleDomain so that the filter expression is not lost. It will be applied during the next applyFilter optimizer. WDYT ?

Depends on contract. If we say getPartitions returns partitions that match constrant, then it should be the case

[Praveen2112]

@sopel39 Added comments.

[sopel39]

Is it filtered by table predicate?
Can partitionNames be loaded independently of partitions?

[Praveen2112]

Is it filtered by table predicate?

Yes but partially.

Can partitionNames be loaded independently of partitions?

If partitions is loaded then partitionNames is reset to Optional.empty

[sopel39]

Could you add that as a comment?

[findepi]

Is it filtered by table predicate?

Yes but partially.

that must be documented

[sopel39]

ping, I still don't understand why we don't have to filter here.
Why there can be partition names and not getPartitions?

[findepi]

Is it filtered by table predicate?

Yes but partially.

that must be documented

[findepi]

use assertThat(query as in the next assertion

[findepi]

You can add this test in the prep commit, like you did with the other test.
i understand the query fails before changes?

[findepi]

not a javadoc

[findepi]

Document why we're making this choice.

In any case, putting this boolean in HivePartitionResult is wrong.
The user of HivePartitionResult should do this logic (getPartitionsAsList or caller of it)
Remove HivePartitionResult.canParsePartitions fielld

[findepi]

This looks wasteful. If we knot the partitions (the objects), we don't need the names anymore.

[findepi]

since we will calculate List<HivePartition> partitionList, the partitionNames list won't be needed.
no need to materialize the list.

[findepi]

This looks as potentially expensive and is also being thrown away.
Document why we believe this is not a problem.

[Praveen2112]

I think this is being seen in the current master too. Since getTableProperties doesn't allow us to update TableHandle this issue is seen.

[findepi]

can partitionColumns be empty here? The outer block of code looks like dealing with partitioned table

[Praveen2112]

In case of unpartitioned table too we might get a single entry for HivePartition, so we need this check.

[findepi]

Add a comment

[findepi]

Thanks for adding a comment.
as a followup please refactor this code. Doing some partition related work over 15 lines only to eventually check that... table isn't actually partitioned. We should reverse the checks

[alexjo2144]

Just a general question, when is getTableProperties called for the discretePredicate relative to when applyFilter is called?

[findepi]

@alexjo2144 seems the only use of ConnectorTableProperties#getDiscretePredicates is in MetadataQueryOptimizer (optimizer.optimize-metadata-queries), so unrelated to applyFilter.

[alexjo2144]

Thanks Piotr, I ask mainly because of the check in getTableProperties looks at partitionNames, but partitionNames is set in applyFilter. It wasn't clear to me that applyFilter happens before MetadataQueryOptimizer.

[findepi]

It wasn't clear to me that applyFilter happens before MetadataQueryOptimizer.

may or may not

[Praveen2112]

@sopel39 , @findepi AC

[Praveen2112]

In case of unpartitioned table too we might get a single entry for HivePartition, so we need this check.

[Praveen2112]

I think this is being seen in the current master too. Since getTableProperties doesn't allow us to update TableHandle this issue is seen.

[alexjo2144]

Why is this block conditional on partitions being loaded? Only the columns list is used here, not the values of the partitions.

[Praveen2112]

If the HiveTableHandle has only raw partition names, then there is a good chance that the Domain for partition columns partially enforced, so we compute the enforced tuple domain post materialization of HivePartition.

[findepi]

bump

[findepi]

The condition here (partitionNames list size, constraint.predicate() being present) doesn't not match the objects being used in the enclosed code (partitionColumns). I don't find the code-level documentation explanatory for this. Can you elaborate?

Also, the logic partitionNames.orElseGet(ImmutableList::of).size() <= maxPartitions is a tricky hack, as partitionNames being empty doesn't mean no partitions being scanned (empty list).
Split into .isEmpty / .isPresent check and separate size check.

[findepi]

"post"?
did you mean

// Note that the computation is not persisted in the table handle, so can be redone many times

?

[Praveen2112]

Yes.

[findepi]

i am not sure this is a great idea (would rather have a new API rather than make a seemingly read-only method not be a read-only method). However, if this is something to address, we should have an issue.

Also, can this cause planning time performance degradation? @sopel39 i think it can.
Note that this new code triggers not only for queries that would fail, but also for queries where we loaded many partition names, and didn't convert them and narrow down to partition objects, out of fear that there will be too many.

Should we have a kill switch?

[findepi]

Also, document why we're doing this. One can intuitively want to return "no known constraint here", and i currently wouldn't be able to explain why not.

[findepi]

Add a comment

[findepi]

Why does the query pass only if you disable join reordering?

[Praveen2112]

When we disable join reordering, the table statistics are not computed during the planning, so the query crosses the planning phase but it fails during execution.

[findepi]

1. this should be a code comment
2. that means the actual user problem has not been solved, right?

[Praveen2112]

that means the actual user problem has not been solved, right?

No. The actual problem that we are trying to solve is -

Query currently fails when we apply a criteria like partition_column like '%abc%' - if the initial number of partitions crosses the threshold - even if only a lesser number of partitions satisfies it.

[findepi]

Where is the test showing the successful execution of a query benefiting from the changes here?
i see one in io.trino.plugin.hive.BaseHiveConnectorTest#testPartitionPerScanLimitWithMultiplePartitionColumns but it's simple SELECT (good test case to test)
i also see io.trino.plugin.hive.optimizer.TestHivePlans#testQueryScanningForTooManyPartitions covering Joins, but it fails with defaults, and requires disabling join reordering to pass.

The solution should work with defaults setting, or any other setting that are probable to be used by a user.

[Praveen2112]

We have fixed for fetching table statistics - now it passes during planning while it fails during execution - due to scanning maximum partitions.

[Praveen2112]

@findepi Thanks for the feedback, have applied the comments.

[Praveen2112]

When we disable join reordering, the table statistics are not computed during the planning, so the query crosses the planning phase but it fails during execution.

[Praveen2112]

Yeah !! In case of HiveTableHandle it is a bit strict (like it fails if we have both partitionNames and partitions).. we check it when we are initializing.

[Praveen2112]

Constraint.alwaysTrue sets the predicate as Optional#empty but we need to pass an default predicate for the API..

Can we modify Constraint.alwaysTrue like we do for Constraint.alwaysFalse ?

[Praveen2112]

Yes.

[Praveen2112]

Yes so in that case we try to compute the remaining tuple domain.

[findepi]

Why didn't you add same check in HivePartitionResult::new?

[findepi]

Partitions are not loaded, since it's a throw-away information.
It's conceivable that hiveTableHandle is later consumed with a slightly different logic, making the enforcedTupleDomain untrue.

[findepi]

Can we modify Constraint.alwaysTrue like we do for Constraint.alwaysFalse ?

that's effective today, but basing on unwritten non-API assumptions sounds like a brittle hack.

[findepi]

That's a prescribed solution, and one which i am not sold on yet.
File a ticket about this problem, but avoiding prescribing solutions, to avoid channelling the discussion.
Link it here.

[Praveen2112]

#10980

[findepi]

1. this should be a code comment
2. that means the actual user problem has not been solved, right?

[alexjo2144]

There are some callers of getPartitions which don't then call this method before collecting the Partitions. getTableStatistics for example, should this be checked there?

[alexjo2144]

Is this change worth doing until we have that done? Seems like loading the partition information once eagerly is better than lazily doing it multiple times

[findepi]

@alexjo2144 not sure what's your suggestion here?

[alexjo2144]

I'm just asking if this change to lazily load the partition information is actually an improvement until that linked issue is completed.

[alexjo2144]

Or if that issue is a blocker for this to be merged.

[Praveen2112]

We lazily evaluate so that it will be loaded after all the filters are pushed to the Hive.

[Praveen2112]

@alexjo2144 AC

[findepi]

please switch to Constraint.alwaysTrue()

[findepi]

Still, can we have a check there?

[findepi]

Thanks for adding a comment.
as a followup please refactor this code. Doing some partition related work over 15 lines only to eventually check that... table isn't actually partitioned. We should reverse the checks

[findepi]

if (hiveTableHandle.getPartitions().isPresent()) then why do we pass partitionNames further, as if we're preserving some value?

move the assignment under if (hiveTableHandle.getPartitions().isPresent()) block, and set the value to Optional.empty() explicitly

[Praveen2112]

We can set it as Optional.empty() and no need to move the assignment.

[findepi]

enforcedConstraint or effectivePredicate ?

it's not enforced yet...
also includes Domains on non-partitioning columns, right? (otherwise you wouldn't do partitions.getEffectivePredicate().filter((column, domain) -> partitionColumns.contains(column)) below)

[Praveen2112]

Yeah. I think it has to be TupleDomain#all

[findepi]

if we skip the if (canPartitionsBeLoaded(partitions) || constraint.predicate().isPresent()) block above (i.e. condition was false), then this variable contains partitions.getEffectivePredicate() which looks like not enforced and potentially containing Domains on non-partitioning columns (so something that won't be enforced)
this is here passed to HiveTableHandle#enforcedConstraint

[Praveen2112]

then this variable contains partitions.getEffectivePredicate()

Actually it would contain TupleDomain#all - but now it will be initialized to HiveTableHandle#enforcedConstraint

[Praveen2112]

@findepi AC. Since there was a conflict had to rebase it.

[Praveen2112]

@findepi AC