Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ public static List<String> getFormulaTokens(String formula, Collection<String> n
}

public static String replaceFormulaTokens(String formula, String sourceToken, String destToken) {
if (sourceToken == null || sourceToken.isEmpty()) {
throw new IllegalArgumentException("sourceToken cannot be empty");
Comment thread
cpwright marked this conversation as resolved.
}
int lastIndex = 0;
while (lastIndex < formula.length() && (lastIndex = formula.indexOf(sourceToken, lastIndex)) != -1) {
if (lastIndex > 0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ private Collection<UpdateViewRequest> translateAndValidateFormatViews(
}

final Table source = inputHierarchicalTable.getSource();
final Selectable[] selectables = request.getUpdateViewsList().stream()
final Selectable[] selectables = request.getFormatViewsList().stream()
.map(uvr -> AggregationAdapter.adapt(uvr.getColumnSpec()))
.toArray(Selectable[]::new);
final String[] columnSpecs = Arrays.asList(selectables).stream()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
//
// Copyright (c) 2016-2026 Deephaven Data Labs and Patent Pending
//
package io.deephaven.server.table.ops;

import io.deephaven.api.ColumnName;
import io.deephaven.api.Selectable;
import io.deephaven.engine.table.ColumnDefinition;
import io.deephaven.engine.table.Table;
import io.deephaven.engine.table.impl.select.FormulaUtil;
import io.deephaven.engine.table.impl.select.SelectColumn;
import io.deephaven.engine.util.TableTools;
import io.deephaven.engine.validation.ColumnExpressionValidator;
import io.deephaven.proto.backplane.grpc.AggSpec.AggSpecFormula;

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

/**
* Shared helper that runs an {@link AggSpecFormula} through the {@link ColumnExpressionValidator} for every input
* column the engine will compile the formula against at runtime. The substitution performed here matches
* {@code FormulaChunkedOperator}'s use of {@link FormulaUtil#replaceFormulaTokens(String, String, String)}, so the
* validator inspects the exact string the engine would compile.
Comment thread
cpwright marked this conversation as resolved.
*/
final class AggSpecFormulaValidator {

private AggSpecFormulaValidator() {}

/**
* Validate {@code formula} for each column in {@code inputColumnNames}, substituting the formula's paramToken with
* the column name.
*
* @param formula the grpc {@link AggSpecFormula}
* @param parent the parent table whose definition is the source shape
* @param groupByColumns the group-by columns (used to build a grouped prototype so non-key columns appear as
* vectors, matching the post-{@code groupBy} shape the engine sees)
* @param inputColumnNames the names of the columns the formula will be applied to
* @param validator the column expression validator
*/
static void validate(
final AggSpecFormula formula,
final Table parent,
final List<ColumnName> groupByColumns,
final List<String> inputColumnNames,
final ColumnExpressionValidator validator) {
if (inputColumnNames.isEmpty()) {
return;
}
final Table groupedPrototype =
TableTools.newTable(parent.getDefinition()).groupBy(groupByColumns);
final String formulaString = formula.getFormula();
final String paramToken = formula.getParamToken();
final String[] expressions = inputColumnNames.stream()
.map(columnName -> columnName + "="
+ FormulaUtil.replaceFormulaTokens(formulaString, paramToken, columnName))
.toArray(String[]::new);
Comment thread
cpwright marked this conversation as resolved.
final SelectColumn[] selectColumns = SelectColumn.from(Selectable.from(expressions));
validator.validateColumnExpressions(selectColumns, expressions, groupedPrototype.getDefinition());
}

/**
* Compute the input column names for the {@code aggAllBy} path: every column in the parent table definition that
* isn't a group-by column. For {@link AggSpecFormula} this matches {@code AggregateAllExclusions} (which adds no
* further exclusions for formula specs).
*/
static List<String> nonKeyColumnNames(final Table parent, final List<ColumnName> groupByColumns) {
final Set<String> keyNames = groupByColumns.stream()
.map(ColumnName::name)
.collect(Collectors.toSet());
return parent.getDefinition().getColumnStream()
.map(ColumnDefinition::getName)
.filter(name -> !keyNames.contains(name))
.collect(Collectors.toList());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
//
package io.deephaven.server.table.ops;

import io.deephaven.api.ColumnName;
import io.deephaven.api.agg.spec.AggSpec;
import io.deephaven.auth.codegen.impl.TableServiceContextualAuthWiring;
import io.deephaven.base.verify.Assert;
import io.deephaven.engine.table.Table;
import io.deephaven.engine.validation.ColumnExpressionValidator;
import io.deephaven.proto.backplane.grpc.AggSpec.TypeCase;
import io.deephaven.proto.backplane.grpc.AggregateAllRequest;
import io.deephaven.proto.backplane.grpc.BatchTableRequest;
Expand All @@ -22,13 +24,17 @@
@Singleton
public final class AggregateAllGrpcImpl extends GrpcTableOperation<AggregateAllRequest> {

private final ColumnExpressionValidator expressionValidator;

@Inject
public AggregateAllGrpcImpl(final TableServiceContextualAuthWiring authWiring) {
public AggregateAllGrpcImpl(final TableServiceContextualAuthWiring authWiring,
final ColumnExpressionValidator expressionValidator) {
super(
authWiring::checkPermissionAggregateAll,
BatchTableRequest.Operation::getAggregateAll,
AggregateAllRequest::getResultId,
AggregateAllRequest::getSourceId);
this.expressionValidator = expressionValidator;
}

@Override
Expand All @@ -46,6 +52,17 @@ public Table create(AggregateAllRequest request, List<ExportObject<Table>> sourc
Assert.eqTrue(request.hasSpec(), "request.hasSpec()");
Assert.neq(request.getSpec().getTypeCase(), "request.getSpec().getTypeCase()", TypeCase.TYPE_NOT_SET);
final Table parent = sourceTables.get(0).get();

if (request.getSpec().getTypeCase() == TypeCase.FORMULA) {
final List<ColumnName> groupByColumns = ColumnName.from(request.getGroupByColumnsList());
AggSpecFormulaValidator.validate(
request.getSpec().getFormula(),
parent,
groupByColumns,
AggSpecFormulaValidator.nonKeyColumnNames(parent, groupByColumns),
expressionValidator);
}

final AggSpec spec = AggSpecAdapter.adapt(request.getSpec());
return parent.aggAllBy(spec, request.getGroupByColumnsList());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package io.deephaven.server.table.ops;

import io.deephaven.api.ColumnName;
import io.deephaven.api.Pair;
import io.deephaven.auth.codegen.impl.TableServiceContextualAuthWiring;
import io.deephaven.base.verify.Assert;
import io.deephaven.engine.table.Table;
Expand Down Expand Up @@ -98,5 +99,16 @@ private void validateFormulas(Aggregation agg, Table parent, List<ColumnName> gr
"Unsupported Selectable type (" + selectableGrpc.getTypeCase() + ") in Aggregation.");
}
}
if (agg.hasColumns()) {
final AggSpec innerSpec = agg.getColumns().getSpec();
if (innerSpec.getTypeCase() == AggSpec.TypeCase.FORMULA) {
final List<String> inputNames = agg.getColumns().getMatchPairsList().stream()
.map(Pair::parse)
.map(pair -> pair.input().name())
.collect(Collectors.toList());
AggSpecFormulaValidator.validate(innerSpec.getFormula(), parent, groupByColumns, inputNames,
expressionValidator);
Comment thread
cpwright marked this conversation as resolved.
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@
import javax.inject.Singleton;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.util.function.Predicate;
import java.util.stream.Collectors;

@Singleton
public class SortTableGrpcImpl extends GrpcTableOperation<SortTableRequest> {
Expand All @@ -40,6 +43,18 @@ public Table create(final SortTableRequest request,
final Table original = sourceTables.get(0).get();
Table result = original;

final Set<String> originalColumns = original.getDefinition().getColumnNameSet();
final List<String> missingColumns = request.getSortsList().stream()
.filter(sort -> sort.getDirection() != SortDescriptor.SortDirection.REVERSE)
.map(SortDescriptor::getColumnName)
.filter(Predicate.not(String::isEmpty))
.filter(Predicate.not(originalColumns::contains))
.collect(Collectors.toList());
Comment thread
cpwright marked this conversation as resolved.
if (!missingColumns.isEmpty()) {
throw Exceptions.statusRuntimeException(Code.FAILED_PRECONDITION,
"column(s) not found: " + String.join(", ", missingColumns));
Comment thread
cpwright marked this conversation as resolved.
}
Comment thread
cpwright marked this conversation as resolved.

final List<String> absColumns = new ArrayList<>();
final List<Selectable> absViews = new ArrayList<>();
for (int si = request.getSortsCount() - 1; si >= 0; si--) {
Expand All @@ -56,11 +71,11 @@ public Table create(final SortTableRequest request,
}

// This loop does two optimizations:
// 1. Consolidate all sorts into a SortPair array in order to only call one sort on the table
// 1. Consolidate all sorts into a List<SortColumn> to only call one sort on the table
// 2. Move all the reverses to the back:
// - For an odd number of reverses only call one reverse
// - For an even number of reverses do not call reverse (they cancel out)
// As a reverse moves past a sort, the direction of the sort is reversed (e.g. asc -> desc)
// As a reverse moves past a sort, the direction of the sort is reversed (e.g., asc -> desc)
final List<SortColumn> sortColumns = new ArrayList<>();
boolean shouldReverse = false;
for (int si = 0; si < request.getSortsCount(); si++) {
Expand All @@ -87,7 +102,7 @@ public Table create(final SortTableRequest request,
? AbsoluteSortColumnConventions.baseColumnNameToAbsoluteName(sort.getColumnName())
: sort.getColumnName();

// if should reverse is true, then a flip the direction of the sort
// if shouldReverse is true, then flip the direction of the sort
if (shouldReverse) {
direction *= -1;
}
Expand All @@ -100,7 +115,7 @@ public Table create(final SortTableRequest request,
}

// Next sort if there are any sorts
if (sortColumns.size() > 0) {
if (!sortColumns.isEmpty()) {
result = result.sort(sortColumns);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import com.google.protobuf.UnknownFieldSet.Field;
import io.deephaven.engine.util.TableTools;
import io.deephaven.proto.backplane.grpc.AggSpec;
import io.deephaven.proto.backplane.grpc.AggSpec.AggSpecFormula;
import io.deephaven.proto.backplane.grpc.AggSpec.AggSpecNonUniqueSentinel;
import io.deephaven.proto.backplane.grpc.AggSpec.AggSpecSum;
import io.deephaven.proto.backplane.grpc.AggSpec.AggSpecUnique;
Expand Down Expand Up @@ -110,6 +111,45 @@ public void emptySourceId() {
"io.deephaven.proto.backplane.grpc.TableReference must have oneof ref. Note: this may also indicate that the server is older than the client and doesn't know about this new oneof option.");
}

@Test
public void formulaRejectsDisallowedExpression() {
final TableReference ref = ref(TableTools.emptyTable(100).view("Key=ii % 2", "I=ii"));
final AggregateAllRequest request = AggregateAllRequest.newBuilder()
.setResultId(ExportTicketHelper.wrapExportIdInTicket(1))
.setSourceId(ref)
.setSpec(AggSpec.newBuilder()
.setFormula(AggSpecFormula.newBuilder()
.setFormula("Runtime.getRuntime().exec(\"touch /tmp/pwned\")")
.setParamToken("each")
.build())
.build())
.addGroupByColumns("Key")
.build();
// Export-time errors are sanitized to "Details Logged w/ID" with the INVALID_ARGUMENT status preserved;
// the full ColumnExpressionValidator message is written to the server log.
assertError(request, Code.INVALID_ARGUMENT, "Details Logged w/ID");
}

@Test
public void formulaAcceptsBenignExpression() {
final TableReference ref = ref(TableTools.emptyTable(100).view("Key=ii % 2", "I=ii"));
final AggregateAllRequest request = AggregateAllRequest.newBuilder()
.setResultId(ExportTicketHelper.wrapExportIdInTicket(1))
.setSourceId(ref)
.setSpec(AggSpec.newBuilder()
.setFormula(AggSpecFormula.newBuilder()
.setFormula("each.size()")
.setParamToken("each")
.build())
.build())
.addGroupByColumns("Key")
.build();
final ExportedTableCreationResponse response = channel().tableBlocking().aggregateAll(request);
assertThat(response.getSuccess()).isTrue();
assertThat(response.getIsStatic()).isTrue();
assertThat(response.getSize()).isEqualTo(2);
}

@Test
public void unknownField() {
final TableReference ref = ref(TableTools.emptyTable(100).view("Key=ii % 2", "I=ii"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import io.deephaven.engine.util.TableTools;
import io.deephaven.proto.backplane.grpc.AggSpec;
import io.deephaven.proto.backplane.grpc.AggSpec.AggSpecCountDistinct;
import io.deephaven.proto.backplane.grpc.AggSpec.AggSpecFormula;
import io.deephaven.proto.backplane.grpc.AggSpec.AggSpecSum;
import io.deephaven.proto.backplane.grpc.AggregateRequest;
import io.deephaven.proto.backplane.grpc.Aggregation;
Expand Down Expand Up @@ -247,6 +248,55 @@ public void aggregationColumnsBadType() {
"io.deephaven.proto.backplane.grpc.AggSpec must have oneof type. Note: this may also indicate that the server is older than the client and doesn't know about this new oneof option.");
}

@Test
public void columnsWithFormulaRejectsDisallowedExpression() {
final TableReference ref = ref(TableTools.emptyTable(100).view("Key=ii % 2", "I=ii"));
final AggregateRequest request = AggregateRequest.newBuilder()
.setResultId(ExportTicketHelper.wrapExportIdInTicket(1))
.setSourceId(ref)
.addAggregations(Aggregation.newBuilder()
.setColumns(AggregationColumns.newBuilder()
.setSpec(AggSpec.newBuilder()
.setFormula(AggSpecFormula.newBuilder()
.setFormula("Runtime.getRuntime().exec(\"touch /tmp/pwned\")")
.setParamToken("each")
.build())
.build())
.addMatchPairs("I")
.build())
.build())
.addGroupByColumns("Key")
.build();
// Export-time errors are sanitized to "Details Logged w/ID" with the INVALID_ARGUMENT status preserved;
// the full ColumnExpressionValidator message is written to the server log.
assertError(request, Code.INVALID_ARGUMENT, "Details Logged w/ID");
}

@Test
public void columnsWithFormulaAcceptsBenignExpression() {
final TableReference ref = ref(TableTools.emptyTable(100).view("Key=ii % 2", "I=ii"));
final AggregateRequest request = AggregateRequest.newBuilder()
.setResultId(ExportTicketHelper.wrapExportIdInTicket(1))
.setSourceId(ref)
.addAggregations(Aggregation.newBuilder()
.setColumns(AggregationColumns.newBuilder()
.setSpec(AggSpec.newBuilder()
.setFormula(AggSpecFormula.newBuilder()
.setFormula("each.size()")
.setParamToken("each")
.build())
.build())
.addMatchPairs("I")
.build())
.build())
.addGroupByColumns("Key")
.build();
final ExportedTableCreationResponse response = channel().tableBlocking().aggregate(request);
assertThat(response.getSuccess()).isTrue();
assertThat(response.getIsStatic()).isTrue();
assertThat(response.getSize()).isEqualTo(2);
}

@Test
public void unknownField() {
final TableReference ref = ref(TableTools.emptyTable(100).view("Key=ii % 2", "I=ii"));
Expand Down
Loading
Loading