Skip to content
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.

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

[CALCITE-2944] Deprecate Aggregate indicator and remove fields where possible #1243

Merged
merged 1 commit into from
May 31, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public EnumerableAggregate(
List<ImmutableBitSet> groupSets,
List<AggregateCall> aggCalls)
throws InvalidRelException {
super(cluster, traitSet, child, indicator, groupSet, groupSets, aggCalls);
super(cluster, traitSet, child, groupSet, groupSets, aggCalls);
Preconditions.checkArgument(!indicator,
"EnumerableAggregate no longer supports indicator fields");
assert getConvention() instanceof EnumerableConvention;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public RelNode convert(RelNode rel) {
rel.getCluster(),
traitSet,
convert(agg.getInput(), EnumerableConvention.INSTANCE),
agg.indicator,
false,
agg.getGroupSet(),
agg.getGroupSets(),
agg.getAggCallList());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,7 @@ public RelNode convert(RelNode rel) {
agg.getTraitSet().replace(out);
try {
return new JdbcAggregate(rel.getCluster(), traitSet,
convert(agg.getInput(), out), agg.indicator, agg.getGroupSet(),
convert(agg.getInput(), out), false, agg.getGroupSet(),
agg.getGroupSets(), agg.getAggCallList());
} catch (InvalidRelException e) {
LOGGER.debug(e.toString());
Expand All @@ -688,10 +688,10 @@ public JdbcAggregate(
List<ImmutableBitSet> groupSets,
List<AggregateCall> aggCalls)
throws InvalidRelException {
super(cluster, traitSet, input, indicator, groupSet, groupSets, aggCalls);
super(cluster, traitSet, input, groupSet, groupSets, aggCalls);
assert getConvention() instanceof JdbcConvention;
assert this.groupSets.size() == 1 : "Grouping sets not supported";
assert !this.indicator;
assert !indicator;
final SqlDialect dialect = ((JdbcConvention) getConvention()).dialect;
for (AggregateCall aggCall : aggCalls) {
if (!canImplement(aggCall.getAggregation(), dialect)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ public AggregateNode(Compiler compiler, Aggregate rel) {

this.unionGroups = union;
this.outputRowLength = unionGroups.cardinality()
+ (rel.indicator ? unionGroups.cardinality() : 0)
+ rel.getAggCallList().size();

ImmutableList.Builder<AccumulatorFactory> builder = ImmutableList.builder();
Expand Down Expand Up @@ -374,9 +373,6 @@ public void end(Sink sink) throws InterruptedException {
for (Integer groupPos : unionGroups) {
if (grouping.get(groupPos)) {
rb.set(index, key.getObject(index));
if (rel.indicator) {
rb.set(unionGroups.cardinality() + index, true);
}
}
// need to set false when not part of grouping set.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -617,9 +617,11 @@ public BindableAggregate(
List<ImmutableBitSet> groupSets,
List<AggregateCall> aggCalls)
throws InvalidRelException {
super(cluster, traitSet, input, indicator, groupSet, groupSets, aggCalls);
super(cluster, traitSet, input, groupSet, groupSets, aggCalls);
assert getConvention() instanceof BindableConvention;

Preconditions.checkArgument(!indicator,
"indicator is not supported, use GROUPING function instead");
for (AggregateCall aggCall : aggCalls) {
if (aggCall.isDistinct()) {
throw new InvalidRelException(
Expand Down Expand Up @@ -680,7 +682,7 @@ public RelNode convert(RelNode rel) {
agg.getTraitSet().replace(BindableConvention.INSTANCE);
try {
return new BindableAggregate(rel.getCluster(), traitSet,
convert(agg.getInput(), traitSet), agg.indicator, agg.getGroupSet(),
convert(agg.getInput(), traitSet), false, agg.getGroupSet(),
agg.getGroupSets(), agg.getAggCallList());
} catch (InvalidRelException e) {
RelOptPlanner.LOGGER.debug(e.toString());
Expand Down
77 changes: 33 additions & 44 deletions core/src/main/java/org/apache/calcite/rel/core/Aggregate.java
Original file line number Diff line number Diff line change
Expand Up @@ -92,12 +92,9 @@ public static boolean isSimple(Aggregate aggregate) {

//~ Instance fields --------------------------------------------------------

/** Whether there are indicator fields.
*
* <p>We strongly discourage the use indicator fields, because they cause the
* output row type of GROUPING SETS queries to be different from regular GROUP
* BY queries, and recommend that you set this field to {@code false}. */
public final boolean indicator;
@Deprecated // unused field, to be removed before 2.0
public final boolean indicator = false;

protected final List<AggregateCall> aggCalls;
protected final ImmutableBitSet groupSet;
public final ImmutableList<ImmutableBitSet> groupSets;
Expand Down Expand Up @@ -125,8 +122,6 @@ public static boolean isSimple(Aggregate aggregate) {
* @param cluster Cluster
* @param traits Traits
* @param child Child
* @param indicator Whether row type should include indicator fields to
* indicate which grouping set is active; true is deprecated
* @param groupSet Bit set of grouping fields
* @param groupSets List of all grouping sets; null for just {@code groupSet}
* @param aggCalls Collection of calls to aggregate functions
Expand All @@ -135,12 +130,10 @@ protected Aggregate(
RelOptCluster cluster,
RelTraitSet traits,
RelNode child,
boolean indicator,
ImmutableBitSet groupSet,
List<ImmutableBitSet> groupSets,
List<AggregateCall> aggCalls) {
super(cluster, traits, child);
this.indicator = indicator; // true is allowed, but discouraged
this.aggCalls = ImmutableList.copyOf(aggCalls);
this.groupSet = Objects.requireNonNull(groupSet);
if (groupSets == null) {
Expand All @@ -161,12 +154,30 @@ protected Aggregate(
}
}

/**
* Creates an Aggregate.
*/
@Deprecated // to be removed before 2.0
protected Aggregate(
RelOptCluster cluster,
RelTraitSet traits,
RelNode child,
boolean indicator,
ImmutableBitSet groupSet,
List<ImmutableBitSet> groupSets,
List<AggregateCall> aggCalls) {
this(cluster, traits, child, groupSet, groupSets, aggCalls);
Preconditions.checkArgument(!indicator,
"indicator is not supported, use GROUPING function instead");
}

public static boolean isNotGrandTotal(Aggregate aggregate) {
return aggregate.getGroupCount() > 0;
}

@Deprecated // to be removed before 2.0
public static boolean noIndicator(Aggregate aggregate) {
return !aggregate.indicator;
return true;
}

private boolean isPredicate(RelNode input, int index) {
Expand All @@ -181,7 +192,6 @@ private boolean isPredicate(RelNode input, int index) {
*/
protected Aggregate(RelInput input) {
this(input.getCluster(), input.getTraitSet(), input.getInput(),
input.getBoolean("indicator", false),
input.getBitSet("group"), input.getBitSetList("groups"),
input.getAggregateCalls("aggs"));
}
Expand All @@ -190,17 +200,15 @@ protected Aggregate(RelInput input) {

@Override public final RelNode copy(RelTraitSet traitSet,
List<RelNode> inputs) {
return copy(traitSet, sole(inputs), indicator, groupSet, groupSets,
return copy(traitSet, sole(inputs), false, groupSet, groupSets,
aggCalls);
}

/** Creates a copy of this aggregate.
*
* @param traitSet Traits
* @param input Input
* @param indicator Whether row type should include indicator fields to
* indicate which grouping set is active; must be true if
* aggregate is not simple
* @param indicator Deprecated, always false
* @param groupSet Bit set of grouping fields
* @param groupSets List of all grouping sets; null for just {@code groupSet}
* @param aggCalls Collection of calls to aggregate functions
Expand Down Expand Up @@ -230,7 +238,7 @@ public List<AggregateCall> getAggCallList() {
* @return list of calls to aggregate functions and their output field names
*/
public List<Pair<AggregateCall, String>> getNamedAggCalls() {
final int offset = getGroupCount() + getIndicatorCount();
final int offset = getGroupCount();
return Pair.zip(aggCalls, Util.skip(getRowType().getFieldNames(), offset));
}

Expand All @@ -253,16 +261,13 @@ public int getGroupCount() {
/**
* Returns the number of indicator fields.
*
* <p>This is the same as {@link #getGroupCount()} if {@link #indicator} is
* true, zero if {@code indicator} is false.
*
* <p>The offset of the first aggregate call in the output record is always
* <i>groupCount + indicatorCount</i>.
* <p>Always zero.
*
* @return number of indicator fields
* @return number of indicator fields, always zero
*/
@Deprecated // to be removed before 2.0
public int getIndicatorCount() {
return indicator ? getGroupCount() : 0;
return 0;
}

/**
Expand All @@ -288,7 +293,6 @@ public RelWriter explainTerms(RelWriter pw) {
super.explainTerms(pw)
.item("group", groupSet)
.itemIf("groups", groupSets, getGroupType() != Group.SIMPLE)
.itemIf("indicator", indicator, indicator)
.itemIf("aggs", aggCalls, pw.nest());
if (!pw.nest()) {
for (Ord<AggregateCall> ord : Ord.zip(aggCalls)) {
Expand Down Expand Up @@ -332,17 +336,15 @@ public RelWriter explainTerms(RelWriter pw) {

protected RelDataType deriveRowType() {
return deriveRowType(getCluster().getTypeFactory(), getInput().getRowType(),
indicator, groupSet, groupSets, aggCalls);
false, groupSet, groupSets, aggCalls);
}

/**
* Computes the row type of an {@code Aggregate} before it exists.
*
* @param typeFactory Type factory
* @param inputRowType Input row type
* @param indicator Whether row type should include indicator fields to
* indicate which grouping set is active; must be true if
* aggregate is not simple
* @param indicator Deprecated, always false
* @param groupSet Bit set of grouping fields
* @param groupSets List of all grouping sets; null for just {@code groupSet}
* @param aggCalls Collection of calls to aggregate functions
Expand All @@ -365,21 +367,8 @@ public static RelDataType deriveRowType(RelDataTypeFactory typeFactory,
builder.nullable(true);
}
}
if (indicator) {
for (int groupKey : groupList) {
final RelDataType booleanType =
typeFactory.createTypeWithNullability(
typeFactory.createSqlType(SqlTypeName.BOOLEAN), false);
final String base = "i$" + fieldList.get(groupKey).getName();
String name = base;
int i = 0;
while (containedNames.contains(name)) {
name = base + "_" + i++;
}
containedNames.add(name);
builder.add(name, booleanType);
}
}
Preconditions.checkArgument(!indicator,
"indicator is not supported, use GROUPING function instead");
for (Ord<AggregateCall> aggCall : Ord.zip(aggCalls)) {
final String base;
if (aggCall.e.name != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import org.apache.calcite.rel.core.AggregateCall;
import org.apache.calcite.util.ImmutableBitSet;

import com.google.common.base.Preconditions;

import java.util.List;

/**
Expand All @@ -51,8 +53,7 @@ public final class LogicalAggregate extends Aggregate {
* @param cluster Cluster that this relational expression belongs to
* @param traitSet Traits
* @param child input relational expression
* @param indicator Whether row type should include indicator fields to
* indicate which grouping set is active
* @param indicator Unused field, alway false
* @param groupSet Bit set of grouping fields
* @param groupSets Grouping sets, or null to use just {@code groupSet}
* @param aggCalls Array of aggregates to compute, not null
Expand All @@ -65,7 +66,9 @@ public LogicalAggregate(
ImmutableBitSet groupSet,
List<ImmutableBitSet> groupSets,
List<AggregateCall> aggCalls) {
super(cluster, traitSet, child, indicator, groupSet, groupSets, aggCalls);
super(cluster, traitSet, child, groupSet, groupSets, aggCalls);
Preconditions.checkArgument(!indicator,
"indicator is not supported, use GROUPING function instead");
}

@Deprecated // to be removed before 2.0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@
import org.apache.calcite.rex.RexVisitorImpl;
import org.apache.calcite.util.BuiltInMethod;

import com.google.common.collect.ImmutableSet;

import java.util.HashSet;
import java.util.Set;

Expand Down Expand Up @@ -65,17 +63,10 @@ public Set<RelColumnOrigin> getColumnOrigins(Aggregate rel,
return mq.getColumnOrigins(rel.getInput(), iOutputColumn);
}

if (rel.indicator) {
if (iOutputColumn < rel.getGroupCount() + rel.getIndicatorCount()) {
// The indicator column is originated here.
return ImmutableSet.of();
}
}

// Aggregate columns are derived from input columns
AggregateCall call =
rel.getAggCallList().get(iOutputColumn
- rel.getGroupCount() - rel.getIndicatorCount());
- rel.getGroupCount());

final Set<RelColumnOrigin> set = new HashSet<>();
for (Integer iInput : call.getArgList()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -478,8 +478,7 @@ public static void setAggChildKeys(
} else {
// aggregate column -- set a bit for each argument being
// aggregated
AggregateCall agg = aggCalls.get(bit
- (aggRel.getGroupCount() + aggRel.getIndicatorCount()));
AggregateCall agg = aggCalls.get(bit - aggRel.getGroupCount());
for (Integer arg : agg.getArgList()) {
childKey.set(arg);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1035,7 +1035,7 @@ protected MaterializedViewAggregateRule(RelOptRuleOperand operand,
aggregateViewNode.getInput().getRowType().getFieldCount(),
aggregateViewNode.getInput().getRowType().getFieldCount() + offset));
final Aggregate newViewNode = aggregateViewNode.copy(
aggregateViewNode.getTraitSet(), relBuilder.build(), aggregateViewNode.indicator,
aggregateViewNode.getTraitSet(), relBuilder.build(), false,
groupSet.build(), null, aggregateViewNode.getAggCallList());

relBuilder.push(newViewNode);
Expand Down
Loading