From 2a7f6c0f96f721c9dbb50b33b2d98f9c7075c0da Mon Sep 17 00:00:00 2001 From: "Charles P. Wright" Date: Thu, 19 Sep 2024 17:02:22 -0400 Subject: [PATCH] Code review, buidler. --- .../engine/table/impl/QueryTable.java | 3 +- .../table/impl/select/TimeSeriesFilter.java | 161 ++++++++++++------ .../engine/table/impl/select/WhereFilter.java | 2 +- .../impl/select/TestTimeSeriesFilter.java | 25 ++- .../engine/table/impl/util/TestClock.java | 82 --------- .../io/deephaven/engine/util/TestClock.java | 41 +++++ 6 files changed, 170 insertions(+), 144 deletions(-) delete mode 100644 engine/table/src/test/java/io/deephaven/engine/table/impl/util/TestClock.java diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java index a9d15f9fb38..afc185720a9 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java @@ -1091,7 +1091,8 @@ void doRefilter( unmatchedRows.insert(refilterRequestedRowset); refilterRequestedRowset = null; } - final WhereListener.ListenerFilterExecution filterExecution = listener.makeRefilterExecution(unmatchedRows); + final WhereListener.ListenerFilterExecution filterExecution = + listener.makeRefilterExecution(unmatchedRows); filterExecution.scheduleCompletion((adds, unusedMods) -> { final WritableRowSet newMapping = adds.writableCast(); // add back what we previously matched, but for modifications and removals diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/TimeSeriesFilter.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/TimeSeriesFilter.java index d41b3135eac..aa34bb08e08 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/TimeSeriesFilter.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/TimeSeriesFilter.java @@ -11,7 +11,6 @@ import io.deephaven.engine.rowset.RowSet; import io.deephaven.engine.table.*; import io.deephaven.engine.table.impl.InstrumentedTableUpdateListenerAdapter; -import io.deephaven.engine.table.impl.ListenerRecorder; import io.deephaven.engine.table.impl.QueryTable; import io.deephaven.engine.updategraph.NotificationQueue; import io.deephaven.engine.updategraph.UpdateGraph; @@ -41,6 +40,107 @@ public class TimeSeriesFilter extends WhereFilterLivenessArtifactImpl implements NotificationQueue.Dependency { + /** + * A builder to create a TimeSeriesFilter. + */ + public static class Builder { + private String columnName; + private long periodNanos; + private boolean invert; + private Clock clock; + + private Builder() { + // use newBuilder + } + + /** + * Set the column name to filter on. + * + * @param columnName the column name to filter on, required + * @return this builder + */ + public Builder columnName(String columnName) { + this.columnName = columnName; + return this; + } + + /** + * Set an optional Clock to use for this filter. When not specified, the clock is determined by + * {@link DateTimeUtils#currentClock()}. + * + * @param clock the clock to use for the filter + * @return this builder + */ + public Builder clock(Clock clock) { + this.clock = clock; + return this; + } + + /** + * Set the period for this filter. + * + * @param period the period as a string for the filter + * @return this builder + */ + public Builder period(final String period) { + return period(DateTimeUtils.parseDurationNanos(period)); + } + + /** + * Set the period for this filter. + * + * @param period the period as a Duration for the filter + * @return this builder + */ + public Builder period(Duration period) { + return period(period.toNanos()); + } + + /** + * Set the period for this filter. + * + * @param period the period in nanoseconds for the filter + * @return this builder + */ + public Builder period(long period) { + this.periodNanos = period; + return this; + } + + /** + * Set whether this filter should be inverted. An inverted filter accepts only rows that have null timestamps or + * timestamps older than the period. + * + * @param invert true if the filter should be inverted. + * @return this builder + */ + public Builder invert(boolean invert) { + this.invert = invert; + return this; + } + + /** + * Create the TimeSeriesFilter described by this builder. + * + * @return a TimeSeriesFilter using the options from this builder. + */ + public TimeSeriesFilter build() { + if (columnName == null) { + throw new IllegalArgumentException("Column name is required"); + } + return new TimeSeriesFilter(columnName, periodNanos, invert, clock); + } + } + + /** + * Create a builder for a time series filter. + * + * @return a Builder object to configure a TimeSeriesFilter + */ + public static Builder newBuilder() { + return new Builder(); + } + private final String columnName; private final long periodNanos; private final boolean invert; @@ -54,8 +154,8 @@ public class TimeSeriesFilter private Runnable refreshFunctionForUnitTests; /** - * The listener is responsible for listening to the WindowCheck result, updating our RowSet that contains the - * rows in our window, and then notifying the WhereListener that we are requesting recomputation. + * The listener is responsible for listening to the WindowCheck result, updating our RowSet that contains the rows + * in our window, and then notifying the WhereListener that we are requesting recomputation. */ private TimeSeriesFilterWindowListener windowListener; @@ -79,62 +179,19 @@ public TimeSeriesFilter(final String columnName, */ public TimeSeriesFilter(final String columnName, final long periodNanos) { - this(columnName, periodNanos, false); - } - - // TODO: USE A BUILDER FOR THE CONSTRUCTOR - - /** - * Create a TimeSeriesFilter on the given column for the given period in nanoseconds. - * - *

- * The filter may be inverted, meaning that instead of including recent rows within the window it only - * includes rows outside the window or that are null. - *

- * - * @param columnName the name of the timestamp column - * @param periodNanos the duration of the window in nanoseconds - * @param invert true if only rows outside the window should be included in the result - */ - public TimeSeriesFilter(final String columnName, - final long periodNanos, - final boolean invert) { - this(columnName, periodNanos, invert, null); + this(columnName, periodNanos, false, null); } /** * Create a TimeSeriesFilter on the given column for the given period in nanoseconds. * - *

- * The filter may be inverted, meaning that instead of including recent rows within the window it only - * includes rows outside the window. - *

- * - * @param columnName the name of the timestamp column - * @param period the duration of the window as parsed by {@link DateTimeUtils#parseDurationNanos(String)}. - * @param invert true if only rows outside the window should be included in the result - */ - public TimeSeriesFilter(final String columnName, - final String period, - final boolean invert) { - this(columnName, DateTimeUtils.parseDurationNanos(period), invert, null); - } - - /** - * Create a TimeSeriesFilter on the given column for the given period in nanoseconds. - * - *

- * The filter may be inverted, meaning that instead of including recent rows within the window it only - * includes rows outside the window. - *

- * * @param columnName the name of the timestamp column * @param periodNanos the duration of the window in nanoseconds - * @param invert true if only rows outside the window should be included in the result + * @param invert true if only rows outside the window (or with a null timestamp) should be included in the result * @param clock the Clock to use as a time source for this filter, when null the clock supplied by * {@link DateTimeUtils#currentClock()} is used. */ - public TimeSeriesFilter(final String columnName, + private TimeSeriesFilter(final String columnName, final long periodNanos, final boolean invert, @Nullable final Clock clock) { @@ -236,7 +293,8 @@ private class TimeSeriesFilterWindowListener extends InstrumentedTableUpdateList // the column source containing the window; which we match on private final ColumnSource windowColumnSource; - protected TimeSeriesFilterWindowListener(String listenerDescription, QueryTable tableWithWindow, final String windowSourceName) { + protected TimeSeriesFilterWindowListener(String listenerDescription, QueryTable tableWithWindow, + final String windowSourceName) { super(listenerDescription, tableWithWindow, false); this.windowColumnSet = tableWithWindow.newModifiedColumnSet(windowSourceName); this.windowColumnSource = tableWithWindow.getColumnSource(windowSourceName); @@ -293,7 +351,8 @@ public SafeCloseable beginOperation(@NotNull Table sourceTable) { final QueryTable tableWithWindow = (QueryTable) pair.first; refreshFunctionForUnitTests = pair.second; - windowListener = new TimeSeriesFilterWindowListener("TimeSeriesFilter(" + columnName + ", " + Duration.ofNanos(periodNanos) + ", " + invert + ")", + windowListener = new TimeSeriesFilterWindowListener( + "TimeSeriesFilter(" + columnName + ", " + Duration.ofNanos(periodNanos) + ", " + invert + ")", tableWithWindow, windowSourceName); tableWithWindow.addUpdateListener(windowListener); manage(windowListener); diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/WhereFilter.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/WhereFilter.java index faac0d76b1a..9dae903ae4e 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/WhereFilter.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/WhereFilter.java @@ -71,7 +71,7 @@ interface RecomputeListener { /** * Notify that something about the filters has changed such that the following rows of the source table should - * be re-evaluated. The rowSet ownership is not taken by requestRecompute. + * be re-evaluated. The rowSet ownership is not taken by requestRecompute. */ void requestRecompute(RowSet rowSet); diff --git a/engine/table/src/test/java/io/deephaven/engine/table/impl/select/TestTimeSeriesFilter.java b/engine/table/src/test/java/io/deephaven/engine/table/impl/select/TestTimeSeriesFilter.java index 14559cc8bfc..d5cf3646f2c 100644 --- a/engine/table/src/test/java/io/deephaven/engine/table/impl/select/TestTimeSeriesFilter.java +++ b/engine/table/src/test/java/io/deephaven/engine/table/impl/select/TestTimeSeriesFilter.java @@ -9,13 +9,13 @@ import io.deephaven.engine.rowset.WritableRowSet; import io.deephaven.engine.table.Table; import io.deephaven.engine.table.TableDefinition; -import io.deephaven.engine.table.impl.util.TestClock; import io.deephaven.engine.testutil.*; import io.deephaven.engine.testutil.generator.DateGenerator; import io.deephaven.engine.testutil.generator.IntGenerator; import io.deephaven.engine.util.TableTools; import io.deephaven.engine.testutil.testcase.RefreshingTableTestCase; import io.deephaven.engine.table.impl.QueryTable; +import io.deephaven.engine.util.TestClock; import io.deephaven.time.DateTimeUtils; import org.apache.commons.lang3.mutable.MutableInt; import org.jetbrains.annotations.NotNull; @@ -49,7 +49,7 @@ public void testSimple() { final TestClock testClock = new TestClock().setMillis(startTime); final TimeSeriesFilter timeSeriesFilter = - new TimeSeriesFilter("Timestamp", DateTimeUtils.parseDurationNanos("PT00:00:05"), false, testClock); + TimeSeriesFilter.newBuilder().columnName("Timestamp").period("PT00:00:05").clock(testClock).build(); Table filtered = source.where(timeSeriesFilter); TableTools.show(filtered); @@ -97,7 +97,8 @@ public void testInverted() { final TestClock testClock = new TestClock().setMillis(startTime); final TimeSeriesFilter timeSeriesFilter = - new TimeSeriesFilter("Timestamp", DateTimeUtils.parseDurationNanos("PT00:00:05"), true, testClock); + TimeSeriesFilter.newBuilder().columnName("Timestamp").period("PT00:00:05").clock(testClock).invert(true) + .build(); Table filtered = source.where(timeSeriesFilter); TableTools.show(filtered); @@ -147,9 +148,11 @@ public void testIncremental() throws ParseException { final TestClock testClock = new TestClock().setMillis(startDate.getTime()); final TimeSeriesFilter inclusionFilter = - new TimeSeriesFilter("Date", DateTimeUtils.parseDurationNanos("PT01:00:00"), false, testClock); + TimeSeriesFilter.newBuilder().columnName("Date").period("PT01:00:00").clock(testClock).invert(false) + .build(); final TimeSeriesFilter exclusionFilter = - new TimeSeriesFilter("Date", DateTimeUtils.parseDurationNanos("PT01:00:00"), true, testClock); + TimeSeriesFilter.newBuilder().columnName("Date").period("PT01:00:00").clock(testClock).invert(true) + .build(); final ArrayList> filtersToRefresh = new ArrayList<>(); final ControlledUpdateGraph updateGraph = ExecutionContext.getContext().getUpdateGraph().cast(); @@ -184,9 +187,11 @@ public void testIncremental2() throws ParseException { final ControlledUpdateGraph updateGraph = ExecutionContext.getContext().getUpdateGraph().cast(); final TimeSeriesFilter inclusionFilter = - new TimeSeriesFilter("Date", DateTimeUtils.parseDurationNanos("PT01:00:00"), false, testClock); + TimeSeriesFilter.newBuilder().columnName("Date").period("PT01:00:00").clock(testClock).invert(false) + .build(); final TimeSeriesFilter exclusionFilter = - new TimeSeriesFilter("Date", DateTimeUtils.parseDurationNanos("PT01:00:00"), true, testClock); + TimeSeriesFilter.newBuilder().columnName("Date").period("PT01:00:00").clock(testClock).invert(true) + .build(); final ArrayList> filtersToRefresh = new ArrayList<>(); EvalNugget[] en = makeNuggets(table, inclusionFilter, filtersToRefresh, updateGraph, exclusionFilter); @@ -303,9 +308,11 @@ public void testFilterSequence() { final CountingFilter cfe2 = new CountingFilter(); final TimeSeriesFilter inclusionFilter = - new TimeSeriesFilter("Timestamp", DateTimeUtils.parseDurationNanos("PT01:00:00"), false, testClock); + TimeSeriesFilter.newBuilder().columnName("Timestamp").period("PT01:00:00").clock(testClock) + .invert(false).build(); final TimeSeriesFilter exclusionFilter = - new TimeSeriesFilter("Timestamp", DateTimeUtils.parseDurationNanos("PT01:00:00"), true, testClock); + TimeSeriesFilter.newBuilder().columnName("Timestamp").period("PT01:00:00").clock(testClock).invert(true) + .build(); final Table inclusion = source.where(Filter.and(cfi1, inclusionFilter, cfi2)); final Table exclusion = source.where(Filter.and(cfe1, exclusionFilter, cfe2)); diff --git a/engine/table/src/test/java/io/deephaven/engine/table/impl/util/TestClock.java b/engine/table/src/test/java/io/deephaven/engine/table/impl/util/TestClock.java deleted file mode 100644 index 4327a61bcd7..00000000000 --- a/engine/table/src/test/java/io/deephaven/engine/table/impl/util/TestClock.java +++ /dev/null @@ -1,82 +0,0 @@ -// -// Copyright (c) 2016-2024 Deephaven Data Labs and Patent Pending -// -package io.deephaven.engine.table.impl.util; - -import io.deephaven.base.clock.Clock; -import io.deephaven.time.DateTimeUtils; - -import java.time.Instant; -import java.time.ZoneId; - -// TODO: MOVE THIS TO THE EXISTING TESTCLOCK - -/** - * A clock that has a fixed time for use in unit tests. - */ -public class TestClock implements Clock { - private long nanos; - - /** - * Set the time. - * - * @param epochNanos the time to set in nanos since the epoch - * @return this clock - */ - public TestClock setNanos(long epochNanos) { - this.nanos = epochNanos; - return this; - } - - /** - * Set the time. - * - * @param epochMillis the time to set in millis since the epoch - * @return this clock - */ - public TestClock setMillis(long epochMillis) { - this.nanos = DateTimeUtils.millisToNanos(epochMillis); - return this; - } - - /** - * Add millis to the current time. - * - * @param millis the number of millis to add - * @return this clock - */ - public TestClock addMillis(long millis) { - this.nanos += DateTimeUtils.millisToNanos(millis); - return this; - } - - @Override - public long currentTimeMillis() { - return DateTimeUtils.nanosToMillis(nanos); - } - - @Override - public long currentTimeMicros() { - return DateTimeUtils.nanosToMicros(nanos); - } - - @Override - public long currentTimeNanos() { - return nanos; - } - - @Override - public Instant instantNanos() { - return DateTimeUtils.epochNanosToInstant(nanos); - } - - @Override - public Instant instantMillis() { - return instantNanos(); - } - - @Override - public String toString() { - return "TestClock{" + DateTimeUtils.epochNanosToInstant(nanos).atZone(ZoneId.systemDefault()) + "}"; - } -} diff --git a/engine/table/src/test/java/io/deephaven/engine/util/TestClock.java b/engine/table/src/test/java/io/deephaven/engine/util/TestClock.java index 2a7debb4362..0c4ea932f71 100644 --- a/engine/table/src/test/java/io/deephaven/engine/util/TestClock.java +++ b/engine/table/src/test/java/io/deephaven/engine/util/TestClock.java @@ -4,6 +4,9 @@ package io.deephaven.engine.util; import io.deephaven.base.clock.ClockNanoBase; +import io.deephaven.time.DateTimeUtils; + +import java.time.ZoneId; public final class TestClock extends ClockNanoBase { public long now; @@ -20,4 +23,42 @@ public TestClock(long nowNanos) { public long currentTimeNanos() { return now; } + + /** + * Set the time. + * + * @param epochNanos the time to set in nanos since the epoch + * @return this clock + */ + public TestClock setNanos(long epochNanos) { + this.now = epochNanos; + return this; + } + + /** + * Set the time. + * + * @param epochMillis the time to set in millis since the epoch + * @return this clock + */ + public TestClock setMillis(long epochMillis) { + this.now = DateTimeUtils.millisToNanos(epochMillis); + return this; + } + + /** + * Add millis to the current time. + * + * @param millis the number of millis to add + * @return this clock + */ + public TestClock addMillis(long millis) { + this.now += DateTimeUtils.millisToNanos(millis); + return this; + } + + @Override + public String toString() { + return "TestClock{" + DateTimeUtils.epochNanosToInstant(now).atZone(ZoneId.systemDefault()) + "}"; + } }