From 4afb025b3661f379f09eb8628df1490060bb9f0c Mon Sep 17 00:00:00 2001 From: Ryan Caudy Date: Mon, 4 Mar 2024 18:23:39 -0500 Subject: [PATCH] Address Blink table usage of `i`, `ii`, `k`, and column arrays; address improper memoization sharing between blink and non-blink table copies (#5216) * Conditions and formulas applied to blink tables should be allowed to use i, ii, k, and column arrays. * Ensure that aggregations and rollups on copied tables (e.g. those resulting from attribute changes for adding/removing "blink") never use the parent rather than the copy if "isBlink" is different. --- .../engine/table/impl/BlinkTableTools.java | 17 ++++++++++++++--- .../engine/table/impl/MemoizedOperationKey.java | 14 ++++++++++++-- .../impl/select/AbstractConditionFilter.java | 6 +++--- .../impl/select/AbstractFormulaColumn.java | 6 +++--- 4 files changed, 32 insertions(+), 11 deletions(-) diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/BlinkTableTools.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/BlinkTableTools.java index 12dc55fb52a..38e73b19b97 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/BlinkTableTools.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/BlinkTableTools.java @@ -104,19 +104,30 @@ public static Table blinkToAppendOnly( } /** - * Returns true if table is a blink table. + * Returns true if {@code table} is a blink table. * * @param table The table to check for blink behavior - * @return Whether this table is a blink table + * @return Whether {@code table} is a blink table * @see Table#BLINK_TABLE_ATTRIBUTE */ - public static boolean isBlink(Table table) { + public static boolean isBlink(@NotNull final Table table) { if (!table.isRefreshing()) { return false; } return Boolean.TRUE.equals(table.getAttribute(Table.BLINK_TABLE_ATTRIBUTE)); } + /** + * Returns true if {@code attributes} indicate a blink table. + * + * @param attributes The map to check for blink table attributes + * @return Whether {@code attributes} indicate a blink table + * @see Table#BLINK_TABLE_ATTRIBUTE + */ + public static boolean hasBlink(@NotNull final Map attributes) { + return Boolean.TRUE.equals(attributes.get(Table.BLINK_TABLE_ATTRIBUTE)); + } + private static class BlinkToAppendOnlyOperation implements QueryTable.MemoizableOperation { private final QueryTable parent; private final long sizeLimit; diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/MemoizedOperationKey.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/MemoizedOperationKey.java index 1aa88d6a160..6d7541b073f 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/MemoizedOperationKey.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/MemoizedOperationKey.java @@ -60,6 +60,16 @@ boolean attributesCompatible(Map oldAttributes, Map oldAttributes, Map newAttributes) { + return BlinkTableTools.hasBlink(oldAttributes) == BlinkTableTools.hasBlink(newAttributes); + } + + @Override + abstract BaseTable.CopyAttributeOperation copyType(); + } + public interface Provider { MemoizedOperationKey getMemoKey(); } @@ -360,7 +370,7 @@ BaseTable.CopyAttributeOperation getParentCopyType() { } } - private static class AggBy extends AttributeAgnosticMemoizedOperationKey { + private static class AggBy extends BlinkIncompatibleMemoizedOperationKey { private final List aggregations; private final boolean preserveEmpty; @@ -442,7 +452,7 @@ public int hashCode() { } } - private static class Rollup extends AttributeAgnosticMemoizedOperationKey { + private static class Rollup extends BlinkIncompatibleMemoizedOperationKey { private final AggBy aggBy; private final boolean includeConstituents; diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/AbstractConditionFilter.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/AbstractConditionFilter.java index 66a8513fa4c..04667b2c110 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/AbstractConditionFilter.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/AbstractConditionFilter.java @@ -241,9 +241,9 @@ public void validateSafeForRefresh(BaseTable sourceTable) { } if (sourceTable.isRefreshing() && !AbstractFormulaColumn.ALLOW_UNSAFE_REFRESHING_FORMULAS) { // note that constant offset array accesss does not use i/ii or end up in usedColumnArrays - boolean isUnsafe = !sourceTable.isAppendOnly() && (usesI || usesII); - isUnsafe |= !sourceTable.isAddOnly() && usesK; - isUnsafe |= !usedColumnArrays.isEmpty(); + boolean isUnsafe = (usesI || usesII) && !sourceTable.isAppendOnly() && !sourceTable.isBlink(); + isUnsafe |= usesK && !sourceTable.isAddOnly() && !sourceTable.isBlink(); + isUnsafe |= !usedColumnArrays.isEmpty() && !sourceTable.isBlink(); if (isUnsafe) { throw new IllegalArgumentException("Formula '" + formula + "' uses i, ii, k, or column array " + "variables, and is not safe to refresh. Note that some usages, such as on an append-only " + diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/AbstractFormulaColumn.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/AbstractFormulaColumn.java index 791be4beaf9..0661f8e5548 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/select/AbstractFormulaColumn.java +++ b/engine/table/src/main/java/io/deephaven/engine/table/impl/select/AbstractFormulaColumn.java @@ -90,9 +90,9 @@ public void validateSafeForRefresh(BaseTable sourceTable) { } if (sourceTable.isRefreshing() && !ALLOW_UNSAFE_REFRESHING_FORMULAS) { // note that constant offset array accesss does not use i/ii or end up in usedColumnArrays - boolean isUnsafe = !sourceTable.isAppendOnly() && (usesI || usesII); - isUnsafe |= !sourceTable.isAddOnly() && usesK; - isUnsafe |= !usedColumnArrays.isEmpty(); + boolean isUnsafe = (usesI || usesII) && !sourceTable.isAppendOnly() && !sourceTable.isBlink(); + isUnsafe |= usesK && !sourceTable.isAddOnly() && !sourceTable.isBlink(); + isUnsafe |= !usedColumnArrays.isEmpty() && !sourceTable.isBlink(); if (isUnsafe) { throw new IllegalArgumentException("Formula '" + formulaString + "' uses i, ii, k, or column array " + "variables, and is not safe to refresh. Note that some usages, such as on an append-only " +