Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address Blink table usage of i, ii, k, and column arrays; address improper memoization sharing between blink and non-blink table copies #5216

Merged
merged 2 commits into from
Mar 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, Object> attributes) {
return Boolean.TRUE.equals(attributes.get(Table.BLINK_TABLE_ATTRIBUTE));
}

private static class BlinkToAppendOnlyOperation implements QueryTable.MemoizableOperation<QueryTable> {
private final QueryTable parent;
private final long sizeLimit;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,16 @@ boolean attributesCompatible(Map<String, Object> oldAttributes, Map<String, Obje
abstract BaseTable.CopyAttributeOperation copyType();
}

abstract static class BlinkIncompatibleMemoizedOperationKey extends MemoizedOperationKey {
@Override
boolean attributesCompatible(Map<String, Object> oldAttributes, Map<String, Object> newAttributes) {
return BlinkTableTools.hasBlink(oldAttributes) == BlinkTableTools.hasBlink(newAttributes);
}

@Override
abstract BaseTable.CopyAttributeOperation copyType();
}

public interface Provider {
MemoizedOperationKey getMemoKey();
}
Expand Down Expand Up @@ -360,7 +370,7 @@ BaseTable.CopyAttributeOperation getParentCopyType() {
}
}

private static class AggBy extends AttributeAgnosticMemoizedOperationKey {
private static class AggBy extends BlinkIncompatibleMemoizedOperationKey {

private final List<? extends Aggregation> aggregations;
private final boolean preserveEmpty;
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 " +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 " +
Expand Down
Loading