Skip to content

Commit

Permalink
Feedback from Ryan's full review.
Browse files Browse the repository at this point in the history
  • Loading branch information
nbauernfeind committed Aug 15, 2024
1 parent 9744fc1 commit e699d48
Show file tree
Hide file tree
Showing 12 changed files with 149 additions and 170 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,11 @@
package io.deephaven.engine.table.impl;

import io.deephaven.UncheckedDeephavenException;
import io.deephaven.api.util.NameValidator;
import io.deephaven.engine.context.ExecutionContext;
import io.deephaven.engine.context.QueryCompiler;
import io.deephaven.engine.context.QueryCompilerRequest;
import io.deephaven.engine.context.QueryLibrary;
import io.deephaven.engine.context.QueryScope;
import io.deephaven.engine.rowset.TrackingWritableRowSet;
import io.deephaven.engine.table.WritableColumnSource;
import io.deephaven.engine.table.impl.perf.QueryPerformanceRecorder;
import io.deephaven.engine.table.impl.select.codegen.FormulaAnalyzer;
import io.deephaven.util.MultiException;
import io.deephaven.util.SafeCloseable;
import io.deephaven.util.CompletionStageFuture;
Expand All @@ -21,12 +17,7 @@
import org.jetbrains.annotations.VisibleForTesting;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
Expand All @@ -48,76 +39,20 @@ public static QueryCompilerRequestProcessor.BatchProcessor batch() {
}

/**
* @return a CachingSupplier that supplies a snapshot of the current query scope variables
* @return a CachingSupplier that supplies a snapshot of current query scope variables and query library imports
*/
@VisibleForTesting
public static CachingSupplier<Map<String, Object>> newQueryScopeVariableSupplier() {
final QueryScope queryScope = ExecutionContext.getContext().getQueryScope();
return new CachingSupplier<>(() -> Collections.unmodifiableMap(
queryScope.toMap((name, value) -> NameValidator.isValidQueryParameterName(name))));
public static CachingSupplier<FormulaAnalyzer.Imports> newFormulaImportsSupplier() {
return new CachingSupplier<>(FormulaAnalyzer.Imports::new);
}

/**
* @return a CachingSupplier that supplies a snapshot of the current {@link QueryLibrary} package imports
*/
private static CachingSupplier<Collection<Package>> newPackageImportsSupplier() {
final QueryLibrary queryLibrary = ExecutionContext.getContext().getQueryLibrary();
return new CachingSupplier<>(() -> Set.copyOf(queryLibrary.getPackageImports()));
}

/**
* @return a CachingSupplier that supplies a snapshot of the current {@link QueryLibrary} class imports
*/
private static CachingSupplier<Collection<Class<?>>> newClassImportsSupplier() {
final QueryLibrary queryLibrary = ExecutionContext.getContext().getQueryLibrary();
return new CachingSupplier<>(() -> {
final Collection<Class<?>> classImports = new HashSet<>(queryLibrary.getClassImports());
// because QueryLibrary is in the context package, without visibility, we need to add these manually
classImports.add(TrackingWritableRowSet.class);
classImports.add(WritableColumnSource.class);
return Collections.unmodifiableCollection(classImports);
});
}

/**
* @return a CachingSupplier that supplies a snapshot of the current {@link QueryLibrary} static imports
*/
private static CachingSupplier<Collection<Class<?>>> newStaticImportsSupplier() {
final QueryLibrary queryLibrary = ExecutionContext.getContext().getQueryLibrary();
return new CachingSupplier<>(() -> Set.copyOf(queryLibrary.getStaticImports()));
}

private final CachingSupplier<Map<String, Object>> queryScopeVariableSupplier = newQueryScopeVariableSupplier();
private final CachingSupplier<Collection<Package>> packageImportsSupplier = newPackageImportsSupplier();
private final CachingSupplier<Collection<Class<?>>> classImportsSupplier = newClassImportsSupplier();
private final CachingSupplier<Collection<Class<?>>> staticImportsSupplier = newStaticImportsSupplier();

/**
* @return a lazily cached snapshot of the current query scope variables
*/
public final Map<String, Object> getQueryScopeVariables() {
return queryScopeVariableSupplier.get();
}

/**
* @return a lazily cached snapshot of the current {@link QueryLibrary} package imports
*/
public final Collection<Package> getPackageImports() {
return packageImportsSupplier.get();
}

/**
* @return a lazily cached snapshot of the current {@link QueryLibrary} class imports
*/
public final Collection<Class<?>> getClassImports() {
return classImportsSupplier.get();
}
private final CachingSupplier<FormulaAnalyzer.Imports> formulaImportsSupplier = newFormulaImportsSupplier();

/**
* @return a lazily cached snapshot of the current {@link QueryLibrary} static imports
* @return a lazily cached snapshot of current query scope variables and query library imports
*/
public final Collection<Class<?>> getStaticImports() {
return staticImportsSupplier.get();
public final FormulaAnalyzer.Imports getFormulaImports() {
return formulaImportsSupplier.get();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1498,7 +1498,7 @@ public Table update(final Collection<? extends Selectable> newColumns) {
public SelectValidationResult validateSelect(final SelectColumn... selectColumns) {
final SelectColumn[] clones = SelectColumn.copyFrom(selectColumns);
SelectAndViewAnalyzer.AnalyzerContext analyzerContext = SelectAndViewAnalyzer.createContext(
this, SelectAndViewAnalyzer.Mode.SELECT_STATIC, getModifiedColumnSetForUpdates(), true,
this, SelectAndViewAnalyzer.Mode.SELECT_STATIC, true,
false, clones);
return new SelectValidationResult(analyzerContext.createAnalyzer(), clones);
}
Expand Down Expand Up @@ -1526,8 +1526,7 @@ private Table selectOrUpdate(Flavor flavor, final SelectColumn... selectColumns)
}
final boolean publishTheseSources = flavor == Flavor.Update;
final SelectAndViewAnalyzer.AnalyzerContext analyzerContext = SelectAndViewAnalyzer.createContext(
this, mode, getModifiedColumnSetForUpdates(), publishTheseSources, true,
selectColumns);
this, mode, publishTheseSources, true, selectColumns);

final SelectAndViewAnalyzer analyzer = analyzerContext.createAnalyzer();
final SelectColumn[] processedColumns = analyzerContext.getProcessedColumns()
Expand Down Expand Up @@ -1597,12 +1596,6 @@ this, mode, getModifiedColumnSetForUpdates(), publishTheseSources, true,
resultTable.setFlat();
}
propagateDataIndexes(processedColumns, resultTable);
for (final ColumnSource<?> columnSource : analyzerContext.getNewColumnSources()
.values()) {
if (columnSource instanceof PossiblyImmutableColumnSource) {
((PossiblyImmutableColumnSource) columnSource).setImmutable();
}
}
}
}
propagateFlatness(resultTable);
Expand Down Expand Up @@ -1766,8 +1759,7 @@ updateDescription, sizeForInstrumentation(), () -> {
final SelectAndViewAnalyzer.AnalyzerContext analyzerContext =
SelectAndViewAnalyzer.createContext(
this, SelectAndViewAnalyzer.Mode.VIEW_EAGER,
getModifiedColumnSetForUpdates(), publishTheseSources, true,
viewColumns);
publishTheseSources, true, viewColumns);
final SelectColumn[] processedViewColumns = analyzerContext.getProcessedColumns()
.toArray(SelectColumn[]::new);
QueryTable queryTable = new QueryTable(
Expand Down Expand Up @@ -1857,8 +1849,7 @@ public Table lazyUpdate(final Collection<? extends Selectable> newColumns) {

final SelectAndViewAnalyzer.AnalyzerContext analyzerContext =
SelectAndViewAnalyzer.createContext(
this, SelectAndViewAnalyzer.Mode.VIEW_LAZY,
getModifiedColumnSetForUpdates(), true, true, selectColumns);
this, SelectAndViewAnalyzer.Mode.VIEW_LAZY, true, true, selectColumns);
final SelectColumn[] processedColumns = analyzerContext.getProcessedColumns()
.toArray(SelectColumn[]::new);
final QueryTable result = new QueryTable(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import io.deephaven.engine.table.impl.util.UpdateGraphJobScheduler;

import java.util.Map;
import java.util.concurrent.atomic.AtomicBoolean;

/**
* A Shift-Aware listener for Select or Update. It uses the SelectAndViewAnalyzer to calculate how columns affect other
Expand Down Expand Up @@ -86,9 +87,19 @@ public void onUpdate(final TableUpdate upstream) {
jobScheduler = new ImmediateJobScheduler();
}

// do not allow a double-notify
final AtomicBoolean hasNotified = new AtomicBoolean();
analyzer.applyUpdate(acquiredUpdate, toClear, updateHelper, jobScheduler, this,
() -> completionRoutine(acquiredUpdate, jobScheduler, toClear, updateHelper),
this::handleException);
() -> {
if (!hasNotified.getAndSet(true)) {
completionRoutine(acquiredUpdate, jobScheduler, toClear, updateHelper);
}
},
error -> {
if (!hasNotified.getAndSet(true)) {
handleException(error);
}
});
}

private void handleException(Exception e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ public final class QueryLanguageParser extends GenericVisitorAdapter<Class<?>, Q
* imported.
* @param variables A map of the names of scope variables to their types
* @param variableTypeArguments A map of the names of scope variables to their type arguments
* @param queryScopeVariables A mutable map of the names of query scope variables to their values
* @param queryScopeVariables A map of the names of query scope variables to their values
* @param columnVariables A set of column variable names
* @param unboxArguments If true it will unbox the query scope arguments
* @param timeConversionResult The result of converting time literals in the expression
Expand Down Expand Up @@ -266,7 +266,7 @@ public QueryLanguageParser(
* imported.
* @param variables A map of the names of scope variables to their types
* @param variableTypeArguments A map of the names of scope variables to their type arguments
* @param queryScopeVariables A mutable map of the names of query scope variables to their values
* @param queryScopeVariables A map of the names of query scope variables to their values
* @param columnVariables A set of column variable names
* @param unboxArguments If true it will unbox the query scope arguments
* @param verifyIdempotence If true, the parser will verify that the result expression will not mutate when parsed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,7 @@ public synchronized void init(
try {
final QueryLanguageParser.Result result = FormulaAnalyzer.parseFormula(
formula, tableDefinition.getColumnNameMap(), outerToInnerNames,
compilationProcessor.getQueryScopeVariables(),
compilationProcessor.getPackageImports(),
compilationProcessor.getClassImports(),
compilationProcessor.getStaticImports(),
unboxArguments);
compilationProcessor.getFormulaImports(), unboxArguments);

formulaShiftColPair = result.getFormulaShiftColPair();
if (formulaShiftColPair != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
import io.deephaven.io.logger.Logger;
import io.deephaven.util.CompletionStageFuture;
import io.deephaven.util.type.TypeUtils;
import io.deephaven.vector.ObjectVector;
import io.deephaven.vector.VectorFactory;
import org.jetbrains.annotations.NotNull;
import org.jpy.PyObject;
Expand Down Expand Up @@ -181,10 +180,7 @@ public List<String> initDef(
try {
final QueryLanguageParser.Result result = FormulaAnalyzer.parseFormula(
formulaString, columnDefinitionMap, Collections.emptyMap(),
compilationRequestProcessor.getQueryScopeVariables(),
compilationRequestProcessor.getPackageImports(),
compilationRequestProcessor.getClassImports(),
compilationRequestProcessor.getStaticImports());
compilationRequestProcessor.getFormulaImports());
analyzedFormula = FormulaAnalyzer.analyze(formulaString, columnDefinitionMap, result);
hasConstantValue = result.isConstantValueExpression();
formulaShiftColPair = result.getFormulaShiftColPair();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,8 @@ public synchronized void init(
return;
}
final List<Object> valueList = new ArrayList<>();
final Map<String, Object> queryScopeVariables = compilationProcessor.getQueryScopeVariables();
final Map<String, Object> queryScopeVariables =
compilationProcessor.getFormulaImports().getQueryScopeVariables();
final ColumnTypeConvertor convertor = ColumnTypeConvertorFactory.getConvertor(column.getDataType());
for (String strValue : strValues) {
convertor.convertValue(column, tableDefinition, strValue, queryScopeVariables, valueList::add);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ public void init(

try {
boolean wasAnArrayType = convertor.convertValue(
def, tableDefinition, value, compilationProcessor.getQueryScopeVariables(),
def, tableDefinition, value, compilationProcessor.getFormulaImports().getQueryScopeVariables(),
realValue::setValue);
if (wasAnArrayType) {
conversionError =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public abstract class DependencyLayerBase extends SelectAndViewAnalyzer.Layer {
this.selectColumn = selectColumn;
selectColumnHoldsVector = Vector.class.isAssignableFrom(selectColumn.getReturnedType());
this.columnSource = columnSource;
context.populateModifiedColumnSet(mcsBuilder, dependencies);
context.populateParentDependenciesMCS(mcsBuilder, dependencies);
this.myModifiedColumnSet = mcsBuilder;
this.myLayerDependencySet = new BitSet();
context.populateLayerDependencySet(myLayerDependencySet, dependencies);
Expand Down
Loading

0 comments on commit e699d48

Please sign in to comment.