Skip to content

Conversation

@vvysotskyi
Copy link
Member

DRILL-8190: Fix mongo project pushdown for queries with joins

Description

Improved costs calculation for vertex drel to consider resulting columns number in its cost to make plans with the pushed project more preferable.
Added SINGLETON to rel nodes for which execution cannot be distributed to several drillbits.
Fixed deserialization of EnumerableSubScan.

Documentation

NA

Testing

Added unit test, existing tests pass.

@vvysotskyi vvysotskyi added the bug label Sep 18, 2022
@vvysotskyi vvysotskyi self-assigned this Sep 18, 2022
@jnturton jnturton added the backport-to-stable This bug fix is applicable to the latest stable release and should be considered for inclusion there label Sep 19, 2022
@jnturton jnturton self-requested a review September 21, 2022 17:16
@jnturton
Copy link
Contributor

I don't have the expertise to spot any problems here so LGTM and I'll just ask some general questions here and there.

@Override
public RelOptCost computeSelfCost(RelOptPlanner planner, RelMetadataQuery mq) {
return super.computeSelfCost(planner, mq).multiplyBy(0.1);
return super.computeLogicalAggCost(planner, mq).multiplyBy(0.1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these Plugin*Rel RelNodes used when operations are pushed down to storage plugins?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they represent part of the pushed-down query, so it is possible to find out the most optimal query for both Drill and actual storage where the query is pushed down.

public @Nullable RelOptCost computeSelfCost(RelOptPlanner planner, RelMetadataQuery mq) {
double rowCount = estimateRowCount(mq);
double columnCount = Utilities.isStarQuery(getRowType()) ? STAR_COLUMN_COST : getRowType().getFieldCount();
double valueCount = rowCount * columnCount;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was it only queries with a pushed down join that were affected by there being no cost saving for adding a plugin projection?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think more queries were affected, but probably it was simpler to reproduce it on queries with joins.

RelNode intermediatePrel = new PluginIntermediatePrel(
in.getCluster(),
in.getTraitSet().replace(outTrait),
in.getTraitSet().replace(outTrait).plus(DrillDistributionTrait.SINGLETON),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the effect of adding this SINGLETON distribution trait on computed plans?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, Drill wouldn't add extra exchange operators, since this part of the plan is executed in a single drillbit.

import java.util.List;
import java.util.stream.Collectors;

public class DynamicTypeResolverBuilder extends StdTypeResolverBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this new serde builder become necessary because of the changes made in this PR for pushing down projections to Mongo?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, fixes for Mongo (in the java-exec module) helped to find out that serde for enumerable plugins is broken, so fixed it here.

@jnturton
Copy link
Contributor

Thanks for the explanations +1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-to-stable This bug fix is applicable to the latest stable release and should be considered for inclusion there bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants