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

RegionedPageStore contexts must not unconditionally supportUnboundedF… #4261

Closed
wants to merge 1 commit into from
Closed
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 @@ -18,6 +18,15 @@ default long firstRowOffset() {
return 0;
}


/**
* Do the fill contexts produced by this region support unbounded fill?
* @return true if the contexts support unbounded fill; false otherwise
Comment on lines +23 to +24
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Do the fill contexts produced by this region support unbounded fill?
* @return true if the contexts support unbounded fill; false otherwise
* Do the {@link FillContext fill contexts} produced by this region support unbounded fill?
*
* @return {@code true} if the contexts support unbounded fill; {@code false} otherwise

*/
default boolean supportsUnboundedFill() {
return true;
}

abstract class Null<ATTR extends Any>
extends GenericColumnRegionBase<ATTR>
implements ColumnRegion<ATTR>, WithDefaultsForRepeatingValues<ATTR> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public RegionContextHolder(final int chunkCapacity, @Nullable final SharedContext sharedContext, boolean supportsUnboundedFill) {
public RegionContextHolder(
final int chunkCapacity,
@Nullable final SharedContext sharedContext,
final boolean supportsUnboundedFill) {

this.chunkCapacity = chunkCapacity;
this.sharedContext = sharedContext;
this.supportsUnboundedFill = supportsUnboundedFill;
}

@Override
public boolean supportsUnboundedFill() {
return true;
return supportsUnboundedFill;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private volatile boolean allRegionsSupportUnbounded = true;
private volatile boolean allRegionsSupportUnboundedFill = true;


@SuppressWarnings("rawtypes")
private static final ColumnRegion[] EMPTY = new ColumnRegion[0];

Expand Down Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
allRegionsSupportUnbounded &= region.supportsUnboundedFill();
allRegionsSupportUnboundedFill &= region.supportsUnboundedFill();

return regions[regionIndex] = region;
}

@Override
Expand All @@ -162,4 +168,9 @@ public void releaseCachedResources() {
public final REGION_TYPE getNullRegion() {
return nullRegion;
}

@Override
public boolean supportsUnboundedFill() {
return allRegionsSupportUnbounded;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return allRegionsSupportUnbounded;
return allRegionsSupportUnboundedFill;

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -90,4 +90,9 @@ public ColumnRegionReferencing.Null<ATTR, NATIVE_REGION_TYPE> getNullRegion() {
public RegionedColumnSourceBase<NATIVE_DATA_TYPE, ATTR, NATIVE_REGION_TYPE> getNativeSource() {
return nativeSource;
}

@Override
public boolean supportsUnboundedFill() {
return nativeSource.supportsUnboundedFill();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

}

@Override
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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> {

Expand Down Expand Up @@ -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.
*
Expand All @@ -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());
}

/**
Expand Down Expand Up @@ -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>>
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand All @@ -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
Expand All @@ -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;
}
}
}
Loading