Skip to content

Replace constants with static factory methods #11660

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
arhimondr merged 6 commits into from
Oct 10, 2018
Merged

Replace constants with static factory methods #11660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fa95ae8
Add PlanNodeStatsEstimate#isOutputRowCountUnknown method
arhimondr Oct 9, 2018
9daf7d8
Add PlanNodeStatsEstimate#unknown() method
arhimondr Oct 9, 2018
2ccb855
Add SymbolStatsEstimate#unknown() method
arhimondr Oct 9, 2018
d77b5f6
Add SymbolStatsEstimate#zero() method
arhimondr Oct 9, 2018
e4360e6
Replace constants with static factory methods in PlanNodeCostEstimate
arhimondr Oct 9, 2018
3ab6145
Reorder methods in PlanNodeCostEstimate
arhimondr Oct 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion presto-main/src/main/java/com/facebook/presto/cost/AggregationStatsRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,6 @@ private static SymbolStatsEstimate estimateAggregationStats(Aggregation aggregat
requireNonNull(sourceStats, "sourceStats is null");

// TODO implement simple aggregations like: min, max, count, sum
return SymbolStatsEstimate.UNKNOWN_STATS;
return SymbolStatsEstimate.unknown();
}
}
3 changes: 1 addition & 2 deletions presto-main/src/main/java/com/facebook/presto/cost/CachingCostProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import java.util.Map;
import java.util.Optional;

import static com.facebook.presto.cost.PlanNodeCostEstimate.ZERO_COST;
import static com.google.common.base.Verify.verify;
import static java.util.Objects.requireNonNull;

Expand Down Expand Up @@ -93,7 +92,7 @@ private PlanNodeCostEstimate calculateCumulativeCost(PlanNode node)

PlanNodeCostEstimate sourcesCost = node.getSources().stream()
.map(this::getCumulativeCost)
.reduce(ZERO_COST, PlanNodeCostEstimate::add);
.reduce(PlanNodeCostEstimate.zero(), PlanNodeCostEstimate::add);

PlanNodeCostEstimate cumulativeCost = localCosts.add(sourcesCost);
return cumulativeCost;
Expand Down
3 changes: 1 addition & 2 deletions presto-main/src/main/java/com/facebook/presto/cost/CachingStatsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import java.util.Optional;

import static com.facebook.presto.SystemSessionProperties.isEnableStatsCalculator;
import static com.facebook.presto.cost.PlanNodeStatsEstimate.UNKNOWN_STATS;
import static com.facebook.presto.sql.planner.iterative.Lookup.noLookup;
import static com.google.common.base.Verify.verify;
import static java.util.Objects.requireNonNull;
Expand Down Expand Up @@ -59,7 +58,7 @@ public CachingStatsProvider(StatsCalculator statsCalculator, Optional<Memo> memo
public PlanNodeStatsEstimate getStats(PlanNode node)
{
if (!isEnableStatsCalculator(session)) {
return UNKNOWN_STATS;
return PlanNodeStatsEstimate.unknown();
}

requireNonNull(node, "node is null");
Expand Down
2 changes: 1 addition & 1 deletion presto-main/src/main/java/com/facebook/presto/cost/ComposableStatsCalculator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public PlanNodeStatsEstimate calculateStats(PlanNode node, StatsProvider sourceS
return calculatedStats.get();
}
}
return PlanNodeStatsEstimate.UNKNOWN_STATS;
return PlanNodeStatsEstimate.unknown();
}

private static <T extends PlanNode> Optional<PlanNodeStatsEstimate> calculateStats(Rule<T> rule, PlanNode node, StatsProvider sourceStats, Lookup lookup, Session session, TypeProvider types)
Expand Down
12 changes: 5 additions & 7 deletions presto-main/src/main/java/com/facebook/presto/cost/CostCalculatorUsingExchanges.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@
import java.util.Set;
import java.util.function.IntSupplier;

import static com.facebook.presto.cost.PlanNodeCostEstimate.UNKNOWN_COST;
import static com.facebook.presto.cost.PlanNodeCostEstimate.ZERO_COST;
import static com.facebook.presto.cost.PlanNodeCostEstimate.cpuCost;
import static com.facebook.presto.sql.planner.plan.AggregationNode.Step.FINAL;
import static com.facebook.presto.sql.planner.plan.AggregationNode.Step.SINGLE;
Expand Down Expand Up @@ -115,7 +113,7 @@ private static class CostEstimator
@Override
protected PlanNodeCostEstimate visitPlan(PlanNode node, Void context)
{
return UNKNOWN_COST;
return PlanNodeCostEstimate.unknown();
}

@Override
Expand All @@ -133,7 +131,7 @@ public PlanNodeCostEstimate visitAssignUniqueId(AssignUniqueId node, Void contex
@Override
public PlanNodeCostEstimate visitOutput(OutputNode node, Void context)
{
return ZERO_COST;
return PlanNodeCostEstimate.zero();
}

@Override
Expand All @@ -159,7 +157,7 @@ public PlanNodeCostEstimate visitProject(ProjectNode node, Void context)
public PlanNodeCostEstimate visitAggregation(AggregationNode node, Void context)
{
if (node.getStep() != FINAL && node.getStep() != SINGLE) {
return UNKNOWN_COST;
return PlanNodeCostEstimate.unknown();
}
PlanNodeStatsEstimate aggregationStats = getStats(node);
PlanNodeStatsEstimate sourceStats = getStats(node.getSource());
Expand Down Expand Up @@ -230,13 +228,13 @@ public PlanNodeCostEstimate visitSpatialJoin(SpatialJoinNode node, Void context)
@Override
public PlanNodeCostEstimate visitValues(ValuesNode node, Void context)
{
return ZERO_COST;
return PlanNodeCostEstimate.zero();
}

@Override
public PlanNodeCostEstimate visitEnforceSingleRow(EnforceSingleRowNode node, Void context)
{
return ZERO_COST;
return PlanNodeCostEstimate.zero();
}

@Override
Expand Down
5 changes: 2 additions & 3 deletions presto-main/src/main/java/com/facebook/presto/cost/CostCalculatorWithEstimatedExchanges.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import java.util.function.IntSupplier;

import static com.facebook.presto.cost.CostCalculatorUsingExchanges.currentNumberOfWorkerNodes;
import static com.facebook.presto.cost.PlanNodeCostEstimate.ZERO_COST;
import static com.facebook.presto.sql.planner.plan.ExchangeNode.Scope.LOCAL;
import static com.facebook.presto.sql.planner.plan.ExchangeNode.Scope.REMOTE;
import static com.facebook.presto.sql.planner.plan.ExchangeNode.Type.REPARTITION;
Expand Down Expand Up @@ -90,8 +89,8 @@ private static class ExchangeCostEstimator
@Override
protected PlanNodeCostEstimate visitPlan(PlanNode node, Void context)
{
// TODO implement logic for other node types and return UNKNOWN_COST here (or throw)
return ZERO_COST;
// TODO implement logic for other node types and return PlanNodeCostEstimate.unknown() here (or throw)
return PlanNodeCostEstimate.zero();
}

@Override
Expand Down
10 changes: 4 additions & 6 deletions presto-main/src/main/java/com/facebook/presto/cost/FilterStatsCalculator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,6 @@
import static com.facebook.presto.cost.PlanNodeStatsEstimateMath.differenceInNonRangeStats;
Copy link
Copy Markdown
Contributor

@kokosing kokosing Oct 9, 2018

Choose a reason for hiding this comment

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

It looks like inlining zero is error prone, see number of places where you missed NaN

import static com.facebook.presto.cost.PlanNodeStatsEstimateMath.differenceInStats;
import static com.facebook.presto.cost.StatsUtil.toStatsRepresentation;
import static com.facebook.presto.cost.SymbolStatsEstimate.UNKNOWN_STATS;
import static com.facebook.presto.cost.SymbolStatsEstimate.ZERO_STATS;
import static com.facebook.presto.spi.type.BooleanType.BOOLEAN;
import static com.facebook.presto.sql.ExpressionUtils.and;
import static com.facebook.presto.sql.tree.ComparisonExpression.Operator.EQUAL;
Expand Down Expand Up @@ -163,7 +161,7 @@ protected Optional<PlanNodeStatsEstimate> visitExpression(Expression node, Void
private Optional<PlanNodeStatsEstimate> filterForFalseExpression()
{
PlanNodeStatsEstimate.Builder falseStatsBuilder = PlanNodeStatsEstimate.builder();
input.getSymbolsWithKnownStatistics().forEach(symbol -> falseStatsBuilder.addSymbolStatistics(symbol, ZERO_STATS));
input.getSymbolsWithKnownStatistics().forEach(symbol -> falseStatsBuilder.addSymbolStatistics(symbol, SymbolStatsEstimate.zero()));
return Optional.of(falseStatsBuilder
.setOutputRowCount(0.0)
.build());
Expand Down Expand Up @@ -318,7 +316,7 @@ protected Optional<PlanNodeStatsEstimate> visitInPredicate(InPredicate node, Voi

Optional<Symbol> inValueSymbol = asSymbol(node.getValue());
SymbolStatsEstimate inValueStats = getExpressionStats(node.getValue());
if (Objects.equals(inValueStats, UNKNOWN_STATS)) {
if (Objects.equals(inValueStats, SymbolStatsEstimate.unknown())) {
return Optional.empty();
}

Expand Down Expand Up @@ -355,7 +353,7 @@ protected Optional<PlanNodeStatsEstimate> visitComparisonExpression(ComparisonEx

Optional<Symbol> leftSymbol = asSymbol(left);
SymbolStatsEstimate leftStats = getExpressionStats(left);
if (Objects.equals(leftStats, UNKNOWN_STATS)) {
if (Objects.equals(leftStats, SymbolStatsEstimate.unknown())) {
return visitExpression(node, context);
}

Expand All @@ -367,7 +365,7 @@ protected Optional<PlanNodeStatsEstimate> visitComparisonExpression(ComparisonEx
Optional<Symbol> rightSymbol = asSymbol(right);

SymbolStatsEstimate rightStats = getExpressionStats(right);
if (Objects.equals(rightStats, UNKNOWN_STATS)) {
if (Objects.equals(rightStats, SymbolStatsEstimate.unknown())) {
return visitExpression(node, context);
}

Expand Down
5 changes: 2 additions & 3 deletions presto-main/src/main/java/com/facebook/presto/cost/JoinStatsRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import java.util.stream.IntStream;

import static com.facebook.presto.cost.FilterStatsCalculator.UNKNOWN_FILTER_COEFFICIENT;
import static com.facebook.presto.cost.PlanNodeStatsEstimate.UNKNOWN_STATS;
import static com.facebook.presto.cost.SymbolStatsEstimate.buildFrom;
import static com.facebook.presto.sql.planner.plan.Patterns.join;
import static com.facebook.presto.sql.tree.ComparisonExpression.Operator.EQUAL;
Expand Down Expand Up @@ -247,7 +246,7 @@ PlanNodeStatsEstimate calculateJoinComplementStats(
if (criteria.isEmpty()) {
// TODO: account for non-equi conditions
if (filter.isPresent()) {
return UNKNOWN_STATS;
return PlanNodeStatsEstimate.unknown();
}

return leftStats.mapOutputRowCount(rowCount -> 0.0);
Expand Down Expand Up @@ -312,7 +311,7 @@ else if (leftNDV <= matchingRightNDV) {
}
else {
// either leftNDV or rightNDV is NaN
return UNKNOWN_STATS;
return PlanNodeStatsEstimate.unknown();
}

// limit the number of complement rows (to left row count) and account for remaining clauses
Expand Down
29 changes: 22 additions & 7 deletions presto-main/src/main/java/com/facebook/presto/cost/PlanNodeCostEstimate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,28 @@

public final class PlanNodeCostEstimate
{
public static final PlanNodeCostEstimate INFINITE_COST = new PlanNodeCostEstimate(POSITIVE_INFINITY, POSITIVE_INFINITY, POSITIVE_INFINITY);
public static final PlanNodeCostEstimate UNKNOWN_COST = new PlanNodeCostEstimate(NaN, NaN, NaN);
public static final PlanNodeCostEstimate ZERO_COST = new PlanNodeCostEstimate(0, 0, 0);
private static final PlanNodeCostEstimate INFINITE = new PlanNodeCostEstimate(POSITIVE_INFINITY, POSITIVE_INFINITY, POSITIVE_INFINITY);
private static final PlanNodeCostEstimate UNKNOWN = new PlanNodeCostEstimate(NaN, NaN, NaN);
private static final PlanNodeCostEstimate ZERO = new PlanNodeCostEstimate(0, 0, 0);

private final double cpuCost;
private final double memoryCost;
private final double networkCost;
Copy link
Copy Markdown
Contributor

@findepi findepi Oct 8, 2018

Choose a reason for hiding this comment

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

i prefer to keep static factory methods above the instance fields (and below class constants)

Copy link
Copy Markdown
Member Author

@arhimondr arhimondr Oct 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Everywhere else in the code the static factory methods are below the fields.

(Symbol, ExpressionInterpreter, TupleDomain, Domain, MetadataManager, ...)

Some clases define static factor methods after constructor, but i don't think it is right. As we try to define methods in the order of their appearance. So if method A uses method B, method A comes first and the method B comes second. Similar with static factory methods. Since factory methods use constructor, they should come before constructor.

Copy link
Copy Markdown
Contributor

@kokosing kokosing Oct 9, 2018

Choose a reason for hiding this comment

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

It would be nice to have a build check for that to not to go back and forth


public static PlanNodeCostEstimate infinite()
{
return INFINITE;
}

public static PlanNodeCostEstimate unknown()
{
return UNKNOWN;
}

public static PlanNodeCostEstimate zero()
{
return ZERO;
}

public static PlanNodeCostEstimate cpuCost(double cpuCost)
{
Expand All @@ -45,10 +64,6 @@ public static PlanNodeCostEstimate networkCost(double networkCost)
return new PlanNodeCostEstimate(0, 0, networkCost);
}

private final double cpuCost;
private final double memoryCost;
private final double networkCost;

@JsonCreator
public PlanNodeCostEstimate(
@JsonProperty("cpuCost") double cpuCost,
Expand Down
19 changes: 14 additions & 5 deletions presto-main/src/main/java/com/facebook/presto/cost/PlanNodeStatsEstimate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.facebook.presto.sql.planner.TypeProvider;
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.collect.ImmutableMap;
import org.pcollections.HashTreePMap;
import org.pcollections.PMap;

Expand All @@ -39,11 +40,16 @@
public class PlanNodeStatsEstimate
{
private static final double DEFAULT_DATA_SIZE_PER_COLUMN = 50;
public static final PlanNodeStatsEstimate UNKNOWN_STATS = builder().build();
private static final PlanNodeStatsEstimate UNKNOWN = new PlanNodeStatsEstimate(NaN, ImmutableMap.of());

private final double outputRowCount;
private final PMap<Symbol, SymbolStatsEstimate> symbolStatistics;

public static PlanNodeStatsEstimate unknown()
{
return UNKNOWN;
}

@JsonCreator
public PlanNodeStatsEstimate(
@JsonProperty("outputRowCount") double outputRowCount,
Expand All @@ -52,9 +58,7 @@ public PlanNodeStatsEstimate(
this(outputRowCount, HashTreePMap.from(requireNonNull(symbolStatistics, "symbolStatistics is null")));
}

private PlanNodeStatsEstimate(
double outputRowCount,
PMap<Symbol, SymbolStatsEstimate> symbolStatistics)
private PlanNodeStatsEstimate(double outputRowCount, PMap<Symbol, SymbolStatsEstimate> symbolStatistics)
{
checkArgument(isNaN(outputRowCount) || outputRowCount >= 0, "outputRowCount cannot be negative");
this.outputRowCount = outputRowCount;
Expand Down Expand Up @@ -128,7 +132,7 @@ public PlanNodeStatsEstimate mapSymbolColumnStatistics(Symbol symbol, Function<S

public SymbolStatsEstimate getSymbolStatistics(Symbol symbol)
{
return symbolStatistics.getOrDefault(symbol, SymbolStatsEstimate.UNKNOWN_STATS);
return symbolStatistics.getOrDefault(symbol, SymbolStatsEstimate.unknown());
}

@JsonProperty
Expand All @@ -142,6 +146,11 @@ public Set<Symbol> getSymbolsWithKnownStatistics()
return symbolStatistics.keySet();
}

public boolean isOutputRowCountUnknown()
{
return isNaN(outputRowCount);
}

@Override
public String toString()
{
Expand Down
2 changes: 1 addition & 1 deletion presto-main/src/main/java/com/facebook/presto/cost/ScalarStatsCalculator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ private class Visitor
@Override
protected SymbolStatsEstimate visitNode(Node node, Void context)
{
return SymbolStatsEstimate.UNKNOWN_STATS;
return SymbolStatsEstimate.unknown();
}

@Override
Expand Down
3 changes: 1 addition & 2 deletions presto-main/src/main/java/com/facebook/presto/cost/SpatialJoinStatsRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

import java.util.Optional;

import static com.facebook.presto.cost.PlanNodeStatsEstimate.UNKNOWN_STATS;
import static com.facebook.presto.sql.planner.plan.Patterns.spatialJoin;
import static java.util.Objects.requireNonNull;

Expand Down Expand Up @@ -49,7 +48,7 @@ protected Optional<PlanNodeStatsEstimate> doCalculate(SpatialJoinNode node, Stat
case INNER:
return Optional.of(statsCalculator.filterStats(crossJoinStats, node.getFilter(), session, types));
case LEFT:
return Optional.of(UNKNOWN_STATS);
return Optional.of(PlanNodeStatsEstimate.unknown());
default:
throw new IllegalArgumentException("Unknown spatial join type: " + node.getType());
}
Expand Down
10 changes: 4 additions & 6 deletions presto-main/src/main/java/com/facebook/presto/cost/StatsNormalizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@
import java.util.Optional;
import java.util.function.Predicate;

import static com.facebook.presto.cost.SymbolStatsEstimate.UNKNOWN_STATS;
import static com.facebook.presto.cost.SymbolStatsEstimate.ZERO_STATS;
import static java.lang.Double.NaN;
import static java.lang.Double.isNaN;
import static java.lang.Math.floor;
Expand Down Expand Up @@ -70,7 +68,7 @@ private PlanNodeStatsEstimate normalize(PlanNodeStatsEstimate stats, Optional<Co

SymbolStatsEstimate symbolStats = stats.getSymbolStatistics(symbol);
SymbolStatsEstimate normalizedSymbolStats = normalizeSymbolStats(symbol, symbolStats, stats, types);
if (UNKNOWN_STATS.equals(normalizedSymbolStats)) {
if (normalizedSymbolStats.isUnknown()) {
normalized.removeSymbolStatistics(symbol);
continue;
}
Expand All @@ -87,8 +85,8 @@ private PlanNodeStatsEstimate normalize(PlanNodeStatsEstimate stats, Optional<Co
*/
private SymbolStatsEstimate normalizeSymbolStats(Symbol symbol, SymbolStatsEstimate symbolStats, PlanNodeStatsEstimate stats, TypeProvider types)
{
if (UNKNOWN_STATS.equals(symbolStats)) {
return UNKNOWN_STATS;
if (symbolStats.isUnknown()) {
return SymbolStatsEstimate.unknown();
Copy link
Copy Markdown
Contributor

@findepi findepi Oct 9, 2018

Choose a reason for hiding this comment

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

After this change, there is no UNKNOWN_STATS constant, so you could as well return symbolStats

Copy link
Copy Markdown
Member Author

@arhimondr arhimondr Oct 9, 2018

Choose a reason for hiding this comment

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

Returning SymbolStatsEstimate.unknown() is more explicit. Think of it as of

if(variable == null){
  return null;
}

}

double outputRowCount = stats.getOutputRowCount();
Expand Down Expand Up @@ -116,7 +114,7 @@ private SymbolStatsEstimate normalizeSymbolStats(Symbol symbol, SymbolStatsEstim
}

if (distinctValuesCount == 0.0) {
return ZERO_STATS;
return SymbolStatsEstimate.zero();
}

return SymbolStatsEstimate.buildFrom(symbolStats)
Expand Down
Loading