-
Notifications
You must be signed in to change notification settings - Fork 80
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
RegionedPageStore contexts must not unconditionally supportUnboundedF… #4261
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -11,17 +11,18 @@ | |||||||||||
public class RegionContextHolder implements ChunkSource.FillContext { | ||||||||||||
private final int chunkCapacity; | ||||||||||||
private final SharedContext sharedContext; | ||||||||||||
private final boolean supportsUnboundedFill; | ||||||||||||
private Context innerContext; | ||||||||||||
|
||||||||||||
public RegionContextHolder(final int chunkCapacity, @Nullable final SharedContext sharedContext) { | ||||||||||||
|
||||||||||||
public RegionContextHolder(final int chunkCapacity, @Nullable final SharedContext sharedContext, boolean supportsUnboundedFill) { | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
this.chunkCapacity = chunkCapacity; | ||||||||||||
this.sharedContext = sharedContext; | ||||||||||||
this.supportsUnboundedFill = supportsUnboundedFill; | ||||||||||||
} | ||||||||||||
|
||||||||||||
@Override | ||||||||||||
public boolean supportsUnboundedFill() { | ||||||||||||
return true; | ||||||||||||
return supportsUnboundedFill; | ||||||||||||
} | ||||||||||||
|
||||||||||||
/** | ||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -33,6 +33,8 @@ interface MakeDeferred<ATTR extends Values, REGION_TYPE extends ColumnRegion<ATT | |||||
private volatile int regionCount = 0; | ||||||
private volatile REGION_TYPE[] regions; | ||||||
|
||||||
private volatile boolean allRegionsSupportUnbounded = true; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
@SuppressWarnings("rawtypes") | ||||||
private static final ColumnRegion[] EMPTY = new ColumnRegion[0]; | ||||||
|
||||||
|
@@ -136,7 +138,11 @@ private void maybeExtendRegions() { | |||||
*/ | ||||||
@NotNull | ||||||
private synchronized REGION_TYPE updateRegion(final int regionIndex, final REGION_TYPE region) { | ||||||
return regions[regionIndex] = region == null ? nullRegion : region; | ||||||
if (region == null) { | ||||||
return regions[regionIndex] = nullRegion; | ||||||
} | ||||||
allRegionsSupportUnbounded &= region.supportsUnboundedFill(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return regions[regionIndex] = region; | ||||||
} | ||||||
|
||||||
@Override | ||||||
|
@@ -162,4 +168,9 @@ public void releaseCachedResources() { | |||||
public final REGION_TYPE getNullRegion() { | ||||||
return nullRegion; | ||||||
} | ||||||
|
||||||
@Override | ||||||
public boolean supportsUnboundedFill() { | ||||||
return allRegionsSupportUnbounded; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -99,6 +99,11 @@ public int getRegionCount() { | |||
return RegionedColumnSourceWithDictionary.this.getRegionCount(); | ||||
} | ||||
|
||||
@Override | ||||
public boolean supportsUnboundedFill() { | ||||
return RegionedColumnSourceWithDictionary.this.supportsUnboundedFill(); | ||||
} | ||||
|
||||
@Override | ||||
public ColumnRegionLong<DictionaryKeys> getRegion(final int regionIndex) { | ||||
final ColumnRegionObject<DATA_TYPE, Values> sourceRegion = | ||||
|
@@ -124,6 +129,8 @@ public ColumnRegionLong<DictionaryKeys> getRegion(final int regionIndex) { | |||
return localWrappers[regionIndex] = | ||||
ColumnRegionObject.DictionaryKeysWrapper.create(parameters(), regionIndex, sourceRegion); | ||||
} | ||||
|
||||
|
||||
Comment on lines
+132
to
+133
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
} | ||||
|
||||
@Override | ||||
|
@@ -195,6 +202,11 @@ public ColumnRegionObject<DATA_TYPE, Values> getRegion(final int regionIndex) { | |||
// so it's fine to call more than once and avoid extra backing storage in the column source. | ||||
return RegionedColumnSourceWithDictionary.this.getRegion(regionIndex).getDictionaryValuesRegion(); | ||||
} | ||||
|
||||
@Override | ||||
public boolean supportsUnboundedFill() { | ||||
return RegionedColumnSourceWithDictionary.this.supportsUnboundedFill(); | ||||
} | ||||
} | ||||
|
||||
@Override | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,8 @@ | |
import io.deephaven.util.annotations.FinalDefault; | ||
import org.jetbrains.annotations.NotNull; | ||
|
||
import java.util.Arrays; | ||
|
||
public interface RegionedPageStore<ATTR extends Any, INNER_ATTR extends ATTR, REGION_TYPE extends Page<INNER_ATTR>> | ||
extends PageStore<ATTR, INNER_ATTR, REGION_TYPE> { | ||
|
||
|
@@ -72,6 +74,14 @@ default int getRegionIndex(final long elementIndex) { | |
*/ | ||
REGION_TYPE getRegion(int regionIndex); | ||
|
||
/** | ||
* Whether this page store supports an unbounded fill operation. | ||
* | ||
* @return true if unbounded fill is supported. | ||
*/ | ||
boolean supportsUnboundedFill(); | ||
|
||
|
||
/** | ||
* Perform region lookup for an element row key. | ||
* | ||
|
@@ -92,7 +102,7 @@ default REGION_TYPE getPageContaining(final FillContext fillContext, final long | |
|
||
@Override | ||
default FillContext makeFillContext(final int chunkCapacity, final SharedContext sharedContext) { | ||
return new RegionContextHolder(chunkCapacity, sharedContext); | ||
return new RegionContextHolder(chunkCapacity, sharedContext, supportsUnboundedFill()); | ||
} | ||
|
||
/** | ||
|
@@ -133,11 +143,12 @@ private static long validateMask(final long mask, final String name) { | |
/** | ||
* A regioned page store for use when the full set of regions and their sizes are known. | ||
*/ | ||
abstract class Static<ATTR extends Any, INNER_ATTR extends ATTR, REGION_TYPE extends Page<INNER_ATTR>> | ||
abstract class Static<ATTR extends Any, INNER_ATTR extends ATTR, REGION_TYPE extends ColumnRegion<INNER_ATTR>> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the bound is Page instead of ColumnRegion, we can't interrogate the supportsUnboundedFill method. |
||
implements RegionedPageStore<ATTR, INNER_ATTR, REGION_TYPE> { | ||
|
||
private final Parameters parameters; | ||
private final REGION_TYPE[] regions; | ||
private final boolean supportsUnboundedFill; | ||
|
||
/** | ||
* @param parameters Mask and shift parameters | ||
|
@@ -152,6 +163,7 @@ public Static(@NotNull final Parameters parameters, | |
for (final REGION_TYPE region : regions) { | ||
Require.eq(region.mask(), "region.mask()", parameters.regionMask, "parameters.regionMask"); | ||
} | ||
supportsUnboundedFill = Arrays.stream(regions).allMatch(ColumnRegion::supportsUnboundedFill); | ||
} | ||
|
||
@Override | ||
|
@@ -168,5 +180,10 @@ public final int getRegionCount() { | |
public final REGION_TYPE getRegion(final int regionIndex) { | ||
return regions[regionIndex]; | ||
} | ||
|
||
@Override | ||
public boolean supportsUnboundedFill() { | ||
return supportsUnboundedFill; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.