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.

Sign up for GitHub

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

Jump to bottom

Restrict field access methods #1871

Closed
wants to merge 3 commits into from
Closed

Restrict field access methods #1871

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a6d051
* WIP, refactor IndexKeyValueToPartialRecord is complete.
hatyo Oct 16, 2022
ca24264
* refactoring.
hatyo Oct 17, 2022
f63d3f4
* fix failing tests.
hatyo Oct 17, 2022
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
8 changes: 4 additions & 4 deletions ...rd-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/AvailableFields.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,12 @@ public boolean containsAll(@Nonnull Collection<KeyExpression> requiredFields) {
}

@Nullable
public IndexKeyValueToPartialRecord.Builder buildIndexKeyValueToPartialRecord(@Nonnull RecordType recordType) {
public IndexKeyValueToPartialRecord.Builder<String> buildIndexKeyValueToPartialRecord(@Nonnull RecordType recordType) {
if (fields == null) {
return null;
}

final IndexKeyValueToPartialRecord.Builder builder = IndexKeyValueToPartialRecord.newBuilder(recordType);
final IndexKeyValueToPartialRecord.Builder<String> builder = IndexKeyValueToPartialRecord.newBuilder(recordType);
for (Map.Entry<KeyExpression, FieldData> entry : fields.entrySet()) {
if (!addCoveringField(entry.getKey(), entry.getValue(), builder)) {
return null;
Expand Down Expand Up @@ -143,7 +143,7 @@ public static AvailableFields fromIndex(@Nonnull RecordType recordType,
keyFields.addAll(primaryKeys);

Map<KeyExpression, FieldData> fields = new HashMap<>();
final IndexKeyValueToPartialRecord.Builder builder = IndexKeyValueToPartialRecord.newBuilder(recordType);
final IndexKeyValueToPartialRecord.Builder<String> builder = IndexKeyValueToPartialRecord.newBuilder(recordType);
for (int i = 0; i < keyFields.size(); i++) {
KeyExpression keyField = keyFields.get(i);
FieldData fieldData = FieldData.of(IndexKeyValueToPartialRecord.TupleSource.KEY, i);
Expand Down Expand Up @@ -172,7 +172,7 @@ public static AvailableFields fromIndex(@Nonnull RecordType recordType,

public static boolean addCoveringField(@Nonnull KeyExpression requiredExpr,
@Nonnull FieldData fieldData,
@Nonnull IndexKeyValueToPartialRecord.Builder builder) {
@Nonnull IndexKeyValueToPartialRecord.Builder<String> builder) {
if (fieldData.source == IndexKeyValueToPartialRecord.TupleSource.OTHER) {
return true;
}
Expand Down
194 changes: 158 additions & 36 deletions .../src/main/java/com/apple/foundationdb/record/query/plan/IndexKeyValueToPartialRecord.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,13 @@ public class IndexKeyValueToPartialRecord {
@Nonnull
private final List<Copier> copiers;

private IndexKeyValueToPartialRecord(@Nonnull List<Copier> copiers) {
private IndexKeyValueToPartialRecord(@Nonnull final List<Copier> copiers) {
this.copiers = copiers;
}

public Message toRecord(@Nonnull Descriptors.Descriptor recordDescriptor, @Nonnull IndexEntry kv) {
public Message toRecord(@Nonnull final Descriptors.Descriptor recordDescriptor, @Nonnull final IndexEntry kv) {
Message.Builder recordBuilder = DynamicMessage.newBuilder(recordDescriptor);
for (Copier copier : copiers) {
for (final Copier copier : copiers) {
copier.copy(recordDescriptor, recordBuilder, kv);
}
return recordBuilder.build();
Expand Down Expand Up @@ -98,14 +98,14 @@ void copy(@Nonnull Descriptors.Descriptor recordDescriptor, @Nonnull Message.Bui

static class FieldCopier implements Copier {
@Nonnull
private final String field;
private final FieldAccessor.Bound field;
@Nonnull
private final TupleSource source;
private final int index;
private Descriptors.FieldDescriptor fieldDescriptor;
private Descriptors.Descriptor containingType;

private FieldCopier(@Nonnull final Descriptors.FieldDescriptor fieldDescriptor, @Nonnull String field, @Nonnull TupleSource source, int index) {
private FieldCopier(@Nonnull final Descriptors.FieldDescriptor fieldDescriptor, @Nonnull FieldAccessor.Bound field, @Nonnull TupleSource source, int index) {
this.field = field;
this.source = source;
this.index = index;
Expand All @@ -123,7 +123,7 @@ public void copy(@Nonnull Descriptors.Descriptor recordDescriptor, @Nonnull Mess
}
if (!containingType.equals(recordDescriptor)) {
containingType = recordDescriptor;
fieldDescriptor = recordDescriptor.findFieldByName(field);
fieldDescriptor = field.getField(recordDescriptor);
}
switch (fieldDescriptor.getType()) {
case INT32:
Expand Down Expand Up @@ -174,19 +174,19 @@ public int hashCode() {
// DynamicMessage does not support that.
static class MessageCopier implements Copier {
@Nonnull
private final String field;
private final FieldAccessor.Bound field;
@Nonnull
private final IndexKeyValueToPartialRecord nested;

private MessageCopier(@Nonnull String field, @Nonnull IndexKeyValueToPartialRecord nested) {
private MessageCopier(@Nonnull FieldAccessor.Bound field, @Nonnull IndexKeyValueToPartialRecord nested) {
this.field = field;
this.nested = nested;
}

@Override
public void copy(@Nonnull Descriptors.Descriptor recordDescriptor, @Nonnull Message.Builder recordBuilder,
@Nonnull IndexEntry kv) {
final Descriptors.FieldDescriptor fieldDescriptor = recordDescriptor.findFieldByName(field);
final Descriptors.FieldDescriptor fieldDescriptor = field.getField(recordDescriptor);
switch (fieldDescriptor.getType()) {
case MESSAGE:
break;
Expand Down Expand Up @@ -225,68 +225,86 @@ public int hashCode() {
}
}

public static Builder newBuilder(@Nonnull RecordType recordType) {
return new Builder(recordType);
/**
* Creates a new {@link Builder}.
* @param recordType The record type.
* @return a new {@link Builder}.
* @deprecated this method is deprecated, for new usages use {@link Builder#newBuilderUsingFieldName(RecordType)}.
*/
@Deprecated
public static Builder<String> newBuilder(@Nonnull RecordType recordType) {
return newBuilderUsingFieldName(recordType);
}

public static Builder<String> newBuilderUsingFieldName(@Nonnull RecordType recordType) {
return new Builder<>(recordType, FieldAccessor.byName());
}

public static Builder newBuilder(@Nonnull Descriptors.Descriptor recordDescriptor) {
return new Builder(recordDescriptor);
public static Builder<Integer> newBuilderUsingFieldIndex(@Nonnull RecordType recordType) {
return new Builder<>(recordType, FieldAccessor.byIndex());
}

/**
* A builder for {@link IndexKeyValueToPartialRecord}.
* @param <T> The type of the field-accessing identifier, usually either a {@code String} or {@code Integer}.
*/
public static class Builder {
public static class Builder<T extends Comparable<T>> {
@Nonnull
private final Descriptors.Descriptor recordDescriptor;
@Nonnull
private final Map<String, FieldCopier> fields;
private final Map<T, FieldCopier> fields;
@Nonnull
private final Map<String, Builder> nestedBuilders;
private List<Copier> regularCopiers = new ArrayList<>();
private final Map<T, Builder<T>> nestedBuilders;
@Nonnull
private final List<Copier> regularCopiers = new ArrayList<>();
@Nonnull
private final FieldAccessor<T> fieldAccessor;

private Builder(@Nonnull RecordType recordType) {
this(recordType.getDescriptor());
private Builder(@Nonnull RecordType recordType,
@Nonnull final FieldAccessor<T> fieldAccessor) {
this(recordType.getDescriptor(), fieldAccessor);
}

private Builder(@Nonnull Descriptors.Descriptor recordDescriptor) {
private Builder(@Nonnull Descriptors.Descriptor recordDescriptor,
@Nonnull final FieldAccessor<T> fieldAccessor) {
this.recordDescriptor = recordDescriptor;
this.fieldAccessor = fieldAccessor;
this.fields = new TreeMap<>();
this.nestedBuilders = new TreeMap<>();
}

public boolean hasField(@Nonnull String field) {
public boolean hasField(@Nonnull T field) {
return fields.containsKey(field) || nestedBuilders.containsKey(field);
}

public Builder addField(@Nonnull String field, @Nonnull TupleSource source, int index) {
final Descriptors.FieldDescriptor fieldDescriptor = recordDescriptor.findFieldByName(field);
public Builder<T> addField(@Nonnull T field, @Nonnull TupleSource source, int index) {
final Descriptors.FieldDescriptor fieldDescriptor = fieldAccessor.getField(field, recordDescriptor);
if (fieldDescriptor == null) {
throw new MetaDataException("field not found: " + field);
}
if (fieldDescriptor.getType() == Descriptors.FieldDescriptor.Type.MESSAGE &&
!TupleFieldsHelper.isTupleField(fieldDescriptor.getMessageType())) {
throw new RecordCoreException("must set nested message field-by-field: " + field);
}
FieldCopier copier = new FieldCopier(fieldDescriptor, field, source, index);
FieldCopier copier = new FieldCopier(fieldDescriptor, fieldAccessor.bind(field), source, index);
FieldCopier prev = fields.put(field, copier);
if (prev != null) {
throw new RecordCoreException("setting field more than once: " + field);
}
return this;
}

public Builder getFieldBuilder(@Nonnull String field) {
Builder builder = nestedBuilders.get(field);
public Builder<T> getFieldBuilder(@Nonnull T field) {
Builder<T> builder = nestedBuilders.get(field);
if (builder == null) {
final Descriptors.FieldDescriptor fieldDescriptor = recordDescriptor.findFieldByName(field);
final Descriptors.FieldDescriptor fieldDescriptor = fieldAccessor.getField(field, recordDescriptor);
if (fieldDescriptor == null) {
throw new MetaDataException("field not found: " + field);
}
if (fieldDescriptor.getType() != Descriptors.FieldDescriptor.Type.MESSAGE) {
throw new RecordCoreException("not a nested message: " + field);
}
builder = new Builder(fieldDescriptor.getMessageType());
builder = new Builder<>(fieldDescriptor.getMessageType(), fieldAccessor);
nestedBuilders.put(field, builder);
}
return builder;
Expand All @@ -295,7 +313,8 @@ public Builder getFieldBuilder(@Nonnull String field) {
public void addRequiredMessageFields() {
for (Descriptors.FieldDescriptor fieldDescriptor : recordDescriptor.getFields()) {
if (fieldDescriptor.isRequired() && fieldDescriptor.getType() == Descriptors.FieldDescriptor.Type.MESSAGE) {
nestedBuilders.putIfAbsent(fieldDescriptor.getName(), new Builder(fieldDescriptor.getMessageType()));
nestedBuilders.putIfAbsent(fieldAccessor.getFieldIdentifier(fieldDescriptor),
new Builder<T>(fieldDescriptor.getMessageType(), fieldAccessor));
}
}
}
Expand All @@ -310,15 +329,15 @@ public boolean isValid() {
}

public boolean isValid(boolean allowRepeated) {
for (Descriptors.FieldDescriptor fieldDescriptor : recordDescriptor.getFields()) {
if (fieldDescriptor.isRequired() && !hasField(fieldDescriptor.getName())) {
for (final Descriptors.FieldDescriptor fieldDescriptor : recordDescriptor.getFields()) {
if (fieldDescriptor.isRequired() && !hasField(fieldAccessor.getFieldIdentifier(fieldDescriptor))) {
return false;
}
if (!allowRepeated && fieldDescriptor.isRepeated() && hasField(fieldDescriptor.getName())) {
if (!allowRepeated && fieldDescriptor.isRepeated() && hasField(fieldAccessor.getFieldIdentifier(fieldDescriptor))) {
return false;
}
if (fieldDescriptor.getType() == Descriptors.FieldDescriptor.Type.MESSAGE) {
Builder builder = nestedBuilders.get(fieldDescriptor.getName());
final Builder<T> builder = nestedBuilders.get(fieldAccessor.getFieldIdentifier(fieldDescriptor));
if (builder != null && !builder.isValid(allowRepeated)) {
return false;
}
Expand All @@ -332,12 +351,115 @@ public void addRegularCopier(@Nonnull Copier copier) {
}

public IndexKeyValueToPartialRecord build() {
List<Copier> copiers = new ArrayList<>(fields.values());
for (Map.Entry<String, Builder> entry : nestedBuilders.entrySet()) {
copiers.add(new MessageCopier(entry.getKey(), entry.getValue().build()));
final List<Copier> copiers = new ArrayList<>(fields.values());
for (final Map.Entry<T, Builder<T>> entry : nestedBuilders.entrySet()) {
copiers.add(new MessageCopier(fieldAccessor.bind(entry.getKey()), entry.getValue().build()));
}
copiers.addAll(regularCopiers);
return new IndexKeyValueToPartialRecord(copiers);
}
}

interface FieldAccessor<T extends Comparable<T>> {

interface Bound {

Descriptors.FieldDescriptor getField(@Nonnull Descriptors.Descriptor record);

abstract class BoundFieldAccessorImpl<T extends Comparable<T>> implements Bound {
final T modifier;

private BoundFieldAccessorImpl(final T modifier) {
this.modifier = modifier;
}

@Override
public boolean equals(final Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
final var that = (BoundFieldAccessorImpl<?>)o;
return modifier.equals(that.modifier);
}

@Override
public int hashCode() {
return Objects.hash(modifier);
}

}

static BoundFieldAccessorImpl<Integer> get(int modifier) {
return new BoundFieldAccessorImpl<>(modifier) {
@Override
public Descriptors.FieldDescriptor getField(@Nonnull final Descriptors.Descriptor record) {
return record.findFieldByNumber(modifier);
}
};
}

static BoundFieldAccessorImpl<String> get(@Nonnull final String modifier) {
return new BoundFieldAccessorImpl<>(modifier) {
@Override
public Descriptors.FieldDescriptor getField(@Nonnull final Descriptors.Descriptor record) {
return record.findFieldByName(modifier);
}
};
}

}

Descriptors.FieldDescriptor getField(@Nonnull T modifier, @Nonnull Descriptors.Descriptor record);

T getFieldIdentifier(@Nonnull Descriptors.FieldDescriptor fieldDescriptor);

Bound bind(@Nonnull T modifier);

class ByIndex implements FieldAccessor<Integer> {
@Override
public Descriptors.FieldDescriptor getField(@Nonnull final Integer modifier, @Nonnull final Descriptors.Descriptor record) {
return record.findFieldByNumber(modifier);
}

@Override
public Integer getFieldIdentifier(@Nonnull final Descriptors.FieldDescriptor fieldDescriptor) {
return fieldDescriptor.getNumber();
}

@Override
public Bound bind(@Nonnull final Integer modifier) {
return Bound.get(modifier);
}
}

class ByName implements FieldAccessor<String> {
@Override
public Descriptors.FieldDescriptor getField(@Nonnull final String modifier, @Nonnull final Descriptors.Descriptor record) {
return record.findFieldByName(modifier);
}

@Override
public String getFieldIdentifier(@Nonnull final Descriptors.FieldDescriptor fieldDescriptor) {
return fieldDescriptor.getName();
}

@Override
public Bound bind(@Nonnull final String modifier) {
return Bound.get(modifier);
}
}

@Nonnull
static ByIndex byIndex() {
return new ByIndex();
}

@Nonnull
static ByName byName() {
return new ByName();
}
}
}
4 changes: 2 additions & 2 deletions ...layer-core/src/main/java/com/apple/foundationdb/record/query/plan/RecordQueryPlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1736,7 +1736,7 @@ public RecordQueryCoveringIndexPlan planCoveringAggregateIndex(@Nonnull RecordQu
return null;
}

final IndexKeyValueToPartialRecord.Builder builder = IndexKeyValueToPartialRecord.newBuilder(recordType);
final IndexKeyValueToPartialRecord.Builder<String> builder = IndexKeyValueToPartialRecord.newBuilder(recordType);
final List<KeyExpression> keyFields = index.getRootExpression().normalizeKeyForPositions();
final List<KeyExpression> valueFields = Collections.emptyList();
for (KeyExpression resultField : query.getRequiredResults()) {
Expand All @@ -1756,7 +1756,7 @@ public RecordQueryCoveringIndexPlan planCoveringAggregateIndex(@Nonnull RecordQu
}

private static boolean addCoveringField(@Nonnull KeyExpression requiredExpr,
@Nonnull IndexKeyValueToPartialRecord.Builder builder,
@Nonnull IndexKeyValueToPartialRecord.Builder<String> builder,
@Nonnull List<KeyExpression> keyFields,
@Nonnull List<KeyExpression> valueFields) {
final IndexKeyValueToPartialRecord.TupleSource source;
Expand Down
3 changes: 3 additions & 0 deletions ...rd-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/cascades/Column.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@
* @param <V> type parameter for the {@link Value}
*/
public class Column<V extends Value> {

// (yhatem) it is unclear whether we should this be replaced by {@code FieldDelegate}.
// it seems a {@code Field} is used as a named _alias_ of the {@code Column}'s underlying child.
@Nonnull
private final Field field;
@Nonnull
Expand Down
Loading