Skip to content

Commit

Permalink
Fix failing unit tests for aggregations. (#1666)
Browse files Browse the repository at this point in the history
  • Loading branch information
@rcaudy
rcaudy authored Dec 9, 2021
1 parent 95ba60e commit c062c42
Show file tree
Hide file tree
Showing 12 changed files with 140 additions and 291 deletions.
12 changes: 0 additions & 12 deletions engine/table/src/main/java/io/deephaven/engine/table/impl/by/AggregationContextFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,6 @@
*/
public interface AggregationContextFactory {

/**
* Should we allow substitution with a {@link KeyOnlyAggregationFactory} (e.g. selectDistinct) when there are only
* key columns? Instances whose operators could have side effects or are already {@link KeyOnlyAggregationFactory}
* should return false.
*
* @return Whether to allow a {@link KeyOnlyAggregationFactory} to be substituted for this when there are only key
* columns
*/
default boolean allowKeyOnlySubstitution() {
return false;
}

/**
* Make an {@link AggregationContext} for this aggregation.
*
Expand Down
38 changes: 5 additions & 33 deletions engine/table/src/main/java/io/deephaven/engine/table/impl/by/AggregationFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1386,39 +1386,11 @@ public AggregationContextFactory makeAggregationContextFactory() {
operators.add(formulaChunkedOperator);
inputNames.add(CollectionUtil.ZERO_LENGTH_STRING_ARRAY);
} else if (isWeightedAverage || isWeightedSum) {
final String weightName;

if (isWeightedAverage) {
weightName = ((WeightedAverageSpecImpl) inputAggregationSpec)
.getWeightName();
} else {
weightName =
((WeightedSumSpecImpl) inputAggregationSpec).getWeightName();
}

final ColumnSource<?> weightSource = table.getColumnSource(weightName);
final DoubleWeightRecordingInternalOperator weightOperator =
new DoubleWeightRecordingInternalOperator(weightSource.getChunkType());
inputColumns.add(weightSource);
operators.add(weightOperator);

inputNames.add(Stream
.concat(Stream.of(weightName),
Arrays.stream(comboMatchPairs).map(MatchPair::rightColumn))
.toArray(String[]::new));

Arrays.stream(comboMatchPairs).forEach(mp -> {
final ColumnSource<?> columnSource = table.getColumnSource(mp.rightColumn());
inputColumns.add(columnSource);
inputNames.add(new String[] {weightName, mp.rightColumn()});
if (isWeightedAverage) {
operators.add(new ChunkedWeightedAverageOperator(columnSource.getChunkType(),
weightOperator, mp.leftColumn()));
} else {
operators.add(new DoubleChunkedWeightedSumOperator(columnSource.getChunkType(),
weightOperator, mp.leftColumn()));
}
});
final String weightName = isWeightedAverage
? ((WeightedAverageSpecImpl) inputAggregationSpec).getWeightName()
: ((WeightedSumSpecImpl) inputAggregationSpec).getWeightName();
WeightedAverageSumAggregationFactory.getOperatorsAndInputs(table,
weightName, isWeightedSum, comboMatchPairs, operators, inputNames, inputColumns);
} else {
throw new UnsupportedOperationException(
"Unknown AggregationElementImpl: " + inputAggregationSpec.getClass());
Expand Down
18 changes: 2 additions & 16 deletions ...ble/src/main/java/io/deephaven/engine/table/impl/by/ChunkedOperatorAggregationHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,6 @@
@SuppressWarnings("rawtypes")
public class ChunkedOperatorAggregationHelper {

@VisibleForTesting
public static boolean KEY_ONLY_SUBSTITUTION_ENABLED =
Configuration.getInstance()
.getBooleanWithDefault("ChunkedOperatorAggregationHelper.enableKeyOnlySubstitution", true);

static final int CHUNK_SIZE = 1 << 12;

public static QueryTable aggregation(AggregationContextFactory aggregationContextFactory, QueryTable queryTable,
Expand All @@ -68,15 +63,6 @@ public static QueryTable aggregation(AggregationControl control,
&& Arrays.stream(groupByColumns).anyMatch(selectColumn -> !selectColumn.isRetain());
final QueryTable withView = !viewRequired ? queryTable : (QueryTable) queryTable.updateView(groupByColumns);

final AggregationContextFactory aggregationContextFactoryToUse;
if (KEY_ONLY_SUBSTITUTION_ENABLED
&& withView.getDefinition().getColumns().length == groupByColumns.length
&& aggregationContextFactory.allowKeyOnlySubstitution()) {
aggregationContextFactoryToUse = new KeyOnlyAggregationFactory();
} else {
aggregationContextFactoryToUse = aggregationContextFactory;
}

if (queryTable.hasAttribute(Table.REVERSE_LOOKUP_ATTRIBUTE)) {
withView.setAttribute(Table.REVERSE_LOOKUP_ATTRIBUTE,
queryTable.getAttribute(Table.REVERSE_LOOKUP_ATTRIBUTE));
Expand All @@ -86,9 +72,9 @@ public static QueryTable aggregation(AggregationControl control,
final SwapListener swapListener =
withView.createSwapListenerIfRefreshing(SwapListener::new);
withView.initializeWithSnapshot(
"by(" + aggregationContextFactoryToUse + ", " + Arrays.toString(groupByColumns) + ")", swapListener,
"by(" + aggregationContextFactory + ", " + Arrays.toString(groupByColumns) + ")", swapListener,
(usePrev, beforeClockValue) -> {
resultHolder.setValue(aggregation(control, swapListener, aggregationContextFactoryToUse, withView,
resultHolder.setValue(aggregation(control, swapListener, aggregationContextFactory, withView,
groupByColumns, usePrev));
return true;
});
Expand Down
5 changes: 0 additions & 5 deletions ...able/src/main/java/io/deephaven/engine/table/impl/by/FirstOrLastByAggregationFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,6 @@ public FirstOrLastByAggregationFactory(boolean isFirst, String exposeRedirection
this.exposeRedirection = exposeRedirection == null ? null : NameValidator.validateColumnName(exposeRedirection);
}

@Override
public boolean allowKeyOnlySubstitution() {
return true;
}

@Override
public AggregationContext makeAggregationContext(@NotNull final Table table,
@NotNull final String... groupByColumns) {
Expand Down
5 changes: 0 additions & 5 deletions engine/table/src/main/java/io/deephaven/engine/table/impl/by/FormulaAggregationFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,6 @@ public FormulaAggregationFactory(@NotNull final String formula, @NotNull final S
this.columnParamName = columnParamName;
}

@Override
public boolean allowKeyOnlySubstitution() {
return true;
}

@Override
public AggregationContext makeAggregationContext(@NotNull final Table inputTable,
@NotNull final String... groupByColumnNames) {
Expand Down
4 changes: 0 additions & 4 deletions engine/table/src/main/java/io/deephaven/engine/table/impl/by/FreezeByAggregationFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,6 @@
import java.util.*;

public class FreezeByAggregationFactory implements AggregationContextFactory {
@Override
public boolean allowKeyOnlySubstitution() {
return true;
}

@Override
public AggregationContext makeAggregationContext(@NotNull final Table table,
Expand Down
5 changes: 0 additions & 5 deletions engine/table/src/main/java/io/deephaven/engine/table/impl/by/GroupByAggregationFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,6 @@ public static AggregationContextFactory getInstance() {

private GroupByAggregationFactory() {}

@Override
public boolean allowKeyOnlySubstitution() {
return true;
}

@Override
public AggregationContext makeAggregationContext(@NotNull final Table inputTable,
@NotNull final String... groupByColumnNames) {
Expand Down
5 changes: 0 additions & 5 deletions ...table/src/main/java/io/deephaven/engine/table/impl/by/NonKeyColumnAggregationFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,6 @@ public NonKeyColumnAggregationFactory(IterativeChunkedOperatorFactory iterativeC
this.iterativeChunkedOperatorFactory = iterativeChunkedOperatorFactory;
}

@Override
public boolean allowKeyOnlySubstitution() {
return true;
}

@Override
public AggregationContext makeAggregationContext(@NotNull final Table table,
@NotNull final String... groupByColumns) {
Expand Down
5 changes: 0 additions & 5 deletions ...rc/main/java/io/deephaven/engine/table/impl/by/SortedFirstOrLastByAggregationFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,6 @@ public SortedFirstOrLastByAggregationFactory(final boolean isFirst, final boolea
this.sortColumns = sortColumns;
}

@Override
public boolean allowKeyOnlySubstitution() {
return true;
}

@Override
public AggregationContext makeAggregationContext(@NotNull final Table table,
@NotNull final String... groupByColumns) {
Expand Down
Loading

0 comments on commit c062c42

Please sign in to comment.