Skip to content

Commit

Permalink
Parallelize TableLocation creation and address some interface cleanups (
Browse files Browse the repository at this point in the history
#5432)

* Allow parallel TableLocation factory invocation in AbstractTableLocationProvider

* Delete ColumnLocation.getMetadata()

* Make AbstractTableLocation.loadDataIndex public
  • Loading branch information
rcaudy authored and stanbrub committed May 3, 2024
1 parent 67d1b4f commit af8847a
Show file tree
Hide file tree
Showing 10 changed files with 21 additions and 49 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ private void maybeAddLocations(@NotNull final Collection<ImmutableTableLocationK
return;
}
filterLocationKeys(locationKeys)
.parallelStream()
.forEach(lk -> columnSourceManager.addLocation(locationProvider.getTableLocation(lk)));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import io.deephaven.engine.table.impl.sources.regioned.*;
import io.deephaven.util.type.NamedImplementation;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

/**
* Per-TableLocation, per-column key, state, and factory object.
Expand Down Expand Up @@ -41,20 +40,6 @@ public interface ColumnLocation extends StringUtils.StringKeyedObject, NamedImpl
*/
boolean exists();

/**
* <p>
* Get the metadata object stored with this column, or null if no such data exists.
* <p>
* Historically, this was typically a value to range map (grouping metadata). The value to range map, if non-null,
* is a map from unique (boxed) column values for this location to the associated ranges in which they occur. Ranges
* are either 2-element int[]s, or 2-element long[]s.
*
* @return The metadata stored with this column, or null if no such data exists
*/
@SuppressWarnings("unused")
@Nullable
<METADATA_TYPE> METADATA_TYPE getMetadata(@NotNull ColumnDefinition<?> columnDefinition);

/**
* Get this column location cast to the specified type
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import io.deephaven.engine.rowset.RowSet;
import io.deephaven.hash.KeyedObjectHashMap;
import io.deephaven.hash.KeyedObjectKey;
import io.deephaven.util.annotations.InternalUseOnly;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

Expand Down Expand Up @@ -223,7 +224,10 @@ public final BasicDataIndex getDataIndex(@NotNull final String... columns) {
*
* @param columns The columns to load an index for
* @return The data index, or {@code null} if none exists
* @apiNote This method is {@code public} for use in delegating implementations, and should not be called directly
* otherwise.
*/
@InternalUseOnly
@Nullable
protected abstract BasicDataIndex loadDataIndex(@NotNull String... columns);
public abstract BasicDataIndex loadDataIndex(@NotNull String... columns);
}
Original file line number Diff line number Diff line change
Expand Up @@ -198,18 +198,18 @@ public TableLocation getTableLocationIfPresent(@NotNull final TableLocationKey t
// See JavaDoc on tableLocations for background.
// The intent is to create a TableLocation exactly once to replace the TableLocationKey placeholder that was
// added in handleTableLocationKey.
if (current instanceof TableLocation) {
return (TableLocation) current;
}
synchronized (tableLocations) {
current = tableLocations.get(tableLocationKey);
if (current instanceof TableLocation) {
return (TableLocation) current;
if (!(current instanceof TableLocation)) {
final TableLocationKey immutableKey = (TableLocationKey) current;
// noinspection SynchronizationOnLocalVariableOrMethodParameter
synchronized (immutableKey) {
current = tableLocations.get(immutableKey);
if (immutableKey == current) {
// Note, this may contend for the lock on tableLocations
tableLocations.add(current = makeTableLocation(immutableKey));
}
}
final TableLocation result = makeTableLocation((TableLocationKey) current);
tableLocations.add(result);
return result;
}
return (TableLocation) current;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ protected ColumnLocation makeColumnLocation(@NotNull final String name) {

@Override
@Nullable
protected BasicDataIndex loadDataIndex(@NotNull final String... columns) {
public BasicDataIndex loadDataIndex(@NotNull final String... columns) {
throw new UnsupportedOperationException();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -183,12 +183,6 @@ public boolean exists() {
throw new UnsupportedOperationException();
}

@Nullable
@Override
public <METADATA_TYPE> METADATA_TYPE getMetadata(@NotNull final ColumnDefinition<?> columnDefinition) {
throw new UnsupportedOperationException();
}

@Override
public ColumnRegionChar<Values> makeColumnRegionChar(
@NotNull final ColumnDefinition<?> columnDefinition) {
Expand Down Expand Up @@ -241,7 +235,8 @@ public <TYPE> ColumnRegionObject<TYPE, Values> makeColumnRegionObject(
}

@Override
protected @Nullable BasicDataIndex loadDataIndex(@NotNull final String... columns) {
@Nullable
public BasicDataIndex loadDataIndex(@NotNull final String... columns) {
throw new UnsupportedOperationException();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import io.deephaven.engine.table.impl.sources.regioned.*;
import io.deephaven.generic.region.*;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

public final class TableBackedColumnLocation
extends AbstractColumnLocation
Expand All @@ -39,12 +38,6 @@ public boolean exists() {
return columnSource != null;
}

@Override
public <METADATA_TYPE> @Nullable METADATA_TYPE getMetadata(
@NotNull final ColumnDefinition<?> columnDefinition) {
return null;
}

@Override
public ColumnRegionChar<Values> makeColumnRegionChar(@NotNull final ColumnDefinition<?> columnDefinition) {
return new AppendOnlyFixedSizePageRegionChar<>(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ protected ColumnLocation makeColumnLocation(@NotNull final String name) {

@Override
@Nullable
protected BasicDataIndex loadDataIndex(@NotNull final String... columns) {
public BasicDataIndex loadDataIndex(@NotNull final String... columns) {
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -120,12 +120,6 @@ private ParquetTableLocation tl() {
return (ParquetTableLocation) getTableLocation();
}

@Override
@Nullable
public <METADATA_TYPE> METADATA_TYPE getMetadata(@NotNull final ColumnDefinition<?> columnDefinition) {
return null;
}

private <SOURCE, REGION_TYPE> REGION_TYPE makeColumnRegion(
@NotNull final Function<ColumnDefinition<?>, SOURCE[]> sourceArrayFactory,
@NotNull final ColumnDefinition<?> columnDefinition,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -243,8 +243,8 @@ private static boolean parquetFileExists(@NotNull final URI fileURI) {
return !fileURI.getScheme().equals(FILE_URI_SCHEME) || Files.exists(Path.of(fileURI));
}

@Nullable
@Override
@Nullable
public BasicDataIndex loadDataIndex(@NotNull final String... columns) {
if (tableInfo == null) {
return null;
Expand Down

0 comments on commit af8847a

Please sign in to comment.