Skip to content

Commit c36c6f5

Browse files
woopfeast-ci-bot
woop
authored and
feast-ci-bot
committed
Bug Fixes: Python SDK and Feast Core (feast-dev#353)
* Remove error handling in version and connect methods for Feast Python SDK client * Relax protobuf version constraint * Fix event_time column rename bug * Allow get_feature_set to raise an exception if no feature set found * Add ordering to feature set lists and fix get_feature_set error message * Python SDK: Always update local feature set after apply, and always update source * * Resolve bug with ApplyFeatureSet where it would raise an Unknown exception due to a null source type * Resolve an issue where feature sets with a changing order of fields would increment feature set versions * Python SDK: Only return version information when url is set * Fixed bug in version() method with assignment * Remove exception handling from equalTo method for Feature Set
1 parent e4f2756 commit c36c6f5

12 files changed

Lines changed: 258 additions & 199 deletions

File tree

‎core/src/main/java/feast/core/dao/FeatureSetRepository.java‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,11 @@ public interface FeatureSetRepository extends JpaRepository<FeatureSet, String>
3636
List<FeatureSet> findByName(String name);
3737

3838
// find all versions of featureSets with names matching the regex
39-
@Query(nativeQuery = true, value = "SELECT * FROM feature_sets WHERE name LIKE ?1")
40-
List<FeatureSet> findByNameWithWildcard(String name);
39+
@Query(nativeQuery = true, value = "SELECT * FROM feature_sets "
40+
+ "WHERE name LIKE ?1 ORDER BY name ASC, version ASC")
41+
List<FeatureSet> findByNameWithWildcardOrderByNameAscVersionAsc(String name);
42+
43+
// find all feature sets and order by name and version
44+
List<FeatureSet> findAllByOrderByNameAscVersionAsc();
45+
4146
}

‎core/src/main/java/feast/core/grpc/CoreServiceImpl.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import feast.core.grpc.interceptors.MonitoringInterceptor;
4242
import feast.core.service.JobCoordinatorService;
4343
import feast.core.service.SpecService;
44+
import io.grpc.StatusRuntimeException;
4445
import io.grpc.stub.StreamObserver;
4546
import java.util.HashSet;
4647
import java.util.Set;
@@ -50,7 +51,9 @@
5051
import org.springframework.beans.factory.annotation.Autowired;
5152
import org.springframework.transaction.annotation.Transactional;
5253

53-
/** Implementation of the feast core GRPC service. */
54+
/**
55+
* Implementation of the feast core GRPC service.
56+
*/
5457
@Slf4j
5558
@GRpcService(interceptors = {MonitoringInterceptor.class})
5659
public class CoreServiceImpl extends CoreServiceImplBase {
@@ -78,7 +81,7 @@ public void getFeatureSet(
7881
GetFeatureSetResponse response = specService.getFeatureSet(request);
7982
responseObserver.onNext(response);
8083
responseObserver.onCompleted();
81-
} catch (RetrievalException | InvalidProtocolBufferException e) {
84+
} catch (RetrievalException | InvalidProtocolBufferException | StatusRuntimeException e) {
8285
log.error("Exception has occurred in GetFeatureSet method: ", e);
8386
responseObserver.onError(e);
8487
}

‎core/src/main/java/feast/core/model/FeatureSet.java‎

Lines changed: 52 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,9 @@
2323
import feast.core.FeatureSetProto.FeatureSpec;
2424
import feast.types.ValueProto.ValueType;
2525
import java.util.ArrayList;
26+
import java.util.HashMap;
2627
import java.util.List;
28+
import java.util.Map;
2729
import javax.persistence.CascadeType;
2830
import javax.persistence.Column;
2931
import javax.persistence.Entity;
@@ -152,12 +154,56 @@ public FeatureSetSpec toProto() throws InvalidProtocolBufferException {
152154
* @param other FeatureSet to compare to
153155
* @return boolean denoting if the source or schema have changed.
154156
*/
155-
public boolean equalTo(FeatureSet other) throws InvalidProtocolBufferException {
156-
return name.equals(other.getName())
157-
&& entities.equals(other.entities)
158-
&& features.equals(other.features)
159-
&& source.equalTo(other.getSource())
160-
&& maxAgeSeconds == other.maxAgeSeconds;
157+
public boolean equalTo(FeatureSet other) {
158+
if(!name.equals(other.getName())){
159+
return false;
160+
}
161+
162+
if (!source.equalTo(other.getSource())){
163+
return false;
164+
}
165+
166+
if (maxAgeSeconds != other.maxAgeSeconds){
167+
return false;
168+
}
169+
170+
// Create a map of all fields in this feature set
171+
Map<String, Field> fields = new HashMap<>();
172+
173+
for (Field e : entities){
174+
fields.putIfAbsent(e.getName(), e);
175+
}
176+
177+
for (Field f : features){
178+
fields.putIfAbsent(f.getName(), f);
179+
}
180+
181+
// Ensure map size is consistent with existing fields
182+
if (fields.size() != other.features.size() + other.entities.size())
183+
{
184+
return false;
185+
}
186+
187+
// Ensure the other entities and fields exist in the field map
188+
for (Field e : other.entities){
189+
if(!fields.containsKey(e.getName())){
190+
return false;
191+
}
192+
if (!e.equals(fields.get(e.getName()))){
193+
return false;
194+
}
195+
}
196+
197+
for (Field f : features){
198+
if(!fields.containsKey(f.getName())){
199+
return false;
200+
}
201+
if (!f.equals(fields.get(f.getName()))){
202+
return false;
203+
}
204+
}
205+
206+
return true;
161207
}
162208

163209
@Override

‎core/src/main/java/feast/core/model/Field.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ public Field() {
5757
}
5858

5959
public Field(String featureSetId, String name, ValueType.Enum type) {
60+
// TODO: Remove all mention of feature sets inside of this class!
6061
FeatureSet featureSet = new FeatureSet();
6162
featureSet.setId(featureSetId);
6263
this.featureSet = featureSet;

‎core/src/main/java/feast/core/model/Source.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -173,11 +173,11 @@ public boolean isDefault() {
173173
* @return boolean equal
174174
*/
175175
public boolean equalTo(Source other) {
176-
if (other.isDefault && isDefault) {
176+
if (other.isDefault && isDefault || (type == null && other.type == null)) {
177177
return true;
178178
}
179179

180-
if (!type.equals(other.type)) {
180+
if ((type == null || !type.equals(other.type))) {
181181
return false;
182182
}
183183

‎core/src/main/java/feast/core/service/SpecService.java‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -107,18 +107,27 @@ public GetFeatureSetResponse getFeatureSet(GetFeatureSetRequest request)
107107
if (request.getVersion() == 0) {
108108
featureSet =
109109
featureSetRepository.findFirstFeatureSetByNameOrderByVersionDesc(request.getName());
110+
111+
if (featureSet == null) {
112+
throw io.grpc.Status.NOT_FOUND
113+
.withDescription(String.format("Feature set with name \"%s\" could not be found.",
114+
request.getName()))
115+
.asRuntimeException();
116+
}
110117
} else {
111118
featureSet =
112119
featureSetRepository.findFeatureSetByNameAndVersion(
113120
request.getName(), request.getVersion());
114-
}
115121

116-
if (featureSet == null) {
117-
throw io.grpc.Status.NOT_FOUND
118-
.withDescription("Feature set could not be found")
119-
.asRuntimeException();
122+
if (featureSet == null) {
123+
throw io.grpc.Status.NOT_FOUND
124+
.withDescription(String.format("Feature set with name \"%s\" and version \"%s\" could "
125+
+ "not be found.", request.getName(), request.getVersion()))
126+
.asRuntimeException();
127+
}
120128
}
121129

130+
122131
// Only a single item in list, return successfully
123132
return GetFeatureSetResponse.newBuilder().setFeatureSet(featureSet.toProto()).build();
124133
}
@@ -143,9 +152,9 @@ public ListFeatureSetsResponse listFeatureSets(ListFeatureSetsRequest.Filter fil
143152
checkValidFeatureSetFilterName(name, "featureSetName");
144153
List<FeatureSet> featureSets;
145154
if (name.equals("")) {
146-
featureSets = featureSetRepository.findAll();
155+
featureSets = featureSetRepository.findAllByOrderByNameAscVersionAsc();
147156
} else {
148-
featureSets = featureSetRepository.findByNameWithWildcard(name.replace('*', '%'));
157+
featureSets = featureSetRepository.findByNameWithWildcardOrderByNameAscVersionAsc(name.replace('*', '%'));
149158
featureSets =
150159
featureSets.stream()
151160
.filter(getVersionFilter(filter.getFeatureSetVersion()))
@@ -208,6 +217,7 @@ public ApplyFeatureSetResponse applyFeatureSet(FeatureSetSpec newFeatureSetSpec)
208217
FeatureSetValidator.validateSpec(newFeatureSetSpec);
209218
List<FeatureSet> existingFeatureSets =
210219
featureSetRepository.findByName(newFeatureSetSpec.getName());
220+
211221
if (existingFeatureSets.size() == 0) {
212222
newFeatureSetSpec = newFeatureSetSpec.toBuilder().setVersion(1).build();
213223
} else {

‎core/src/test/java/feast/core/service/SpecServiceTest.java‎

Lines changed: 45 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import feast.core.CoreServiceProto.ListStoresResponse;
3636
import feast.core.CoreServiceProto.UpdateStoreRequest;
3737
import feast.core.CoreServiceProto.UpdateStoreResponse;
38+
import feast.core.FeatureSetProto;
3839
import feast.core.FeatureSetProto.FeatureSetSpec;
3940
import feast.core.FeatureSetProto.FeatureSpec;
4041
import feast.core.SourceProto.KafkaSourceConfig;
@@ -67,11 +68,14 @@
6768

6869
public class SpecServiceTest {
6970

70-
@Mock private FeatureSetRepository featureSetRepository;
71+
@Mock
72+
private FeatureSetRepository featureSetRepository;
7173

72-
@Mock private StoreRepository storeRepository;
74+
@Mock
75+
private StoreRepository storeRepository;
7376

74-
@Rule public final ExpectedException expectedException = ExpectedException.none();
77+
@Rule
78+
public final ExpectedException expectedException = ExpectedException.none();
7579

7680
private SpecService specService;
7781
private List<FeatureSet> featureSets;
@@ -95,14 +99,25 @@ public void setUp() {
9599
FeatureSet featureSet1v3 = newDummyFeatureSet("f1", 3);
96100
FeatureSet featureSet2v1 = newDummyFeatureSet("f2", 1);
97101

98-
featureSets = Arrays.asList(featureSet1v1, featureSet1v2, featureSet1v3, featureSet2v1);
102+
Field f3f1 = new Field("f3", "f3f1", Enum.INT64);
103+
Field f3f2 = new Field("f3", "f3f2", Enum.INT64);
104+
Field f3e1 = new Field("f3", "f3e1", Enum.STRING);
105+
FeatureSet featureSet3v1 = new FeatureSet(
106+
"f3", 1, 100L, Arrays.asList(f3e1), Arrays.asList(f3f2, f3f1), defaultSource);
107+
108+
featureSets = Arrays
109+
.asList(featureSet1v1, featureSet1v2, featureSet1v3, featureSet2v1, featureSet3v1);
99110
when(featureSetRepository.findAll()).thenReturn(featureSets);
111+
when(featureSetRepository.findAllByOrderByNameAscVersionAsc()).thenReturn(featureSets);
100112
when(featureSetRepository.findByName("f1")).thenReturn(featureSets.subList(0, 3));
113+
when(featureSetRepository.findByName("f3")).thenReturn(featureSets.subList(4, 5));
101114
when(featureSetRepository.findFirstFeatureSetByNameOrderByVersionDesc("f1"))
102115
.thenReturn(featureSet1v3);
103-
when(featureSetRepository.findByNameWithWildcard("f1")).thenReturn(featureSets.subList(0, 3));
116+
when(featureSetRepository.findByNameWithWildcardOrderByNameAscVersionAsc("f1"))
117+
.thenReturn(featureSets.subList(0, 3));
104118
when(featureSetRepository.findByName("asd")).thenReturn(Lists.newArrayList());
105-
when(featureSetRepository.findByNameWithWildcard("f%")).thenReturn(featureSets);
119+
when(featureSetRepository.findByNameWithWildcardOrderByNameAscVersionAsc("f%"))
120+
.thenReturn(featureSets);
106121

107122
Store store1 = newDummyStore("SERVING");
108123
Store store2 = newDummyStore("WAREHOUSE");
@@ -238,7 +253,8 @@ public void shouldThrowExceptionGivenMissingFeatureSetName()
238253
@Test
239254
public void shouldThrowExceptionGivenMissingFeatureSet() throws InvalidProtocolBufferException {
240255
expectedException.expect(StatusRuntimeException.class);
241-
expectedException.expectMessage("NOT_FOUND: Feature set could not be found");
256+
expectedException.expectMessage(
257+
"NOT_FOUND: Feature set with name \"f1000\" and version \"2\" could not be found.");
242258
specService.getFeatureSet(
243259
GetFeatureSetRequest.newBuilder().setName("f1000").setVersion(2).build());
244260
}
@@ -331,6 +347,28 @@ public void applyFeatureSetShouldIncrementFeatureSetVersionIfAlreadyExists()
331347
assertThat(applyFeatureSetResponse.getFeatureSet(), equalTo(expected));
332348
}
333349

350+
351+
@Test
352+
public void applyFeatureSetShouldNotCreateFeatureSetIfFieldsUnordered()
353+
throws InvalidProtocolBufferException {
354+
355+
Field f3f1 = new Field("f3", "f3f1", Enum.INT64);
356+
Field f3f2 = new Field("f3", "f3f2", Enum.INT64);
357+
Field f3e1 = new Field("f3", "f3e1", Enum.STRING);
358+
FeatureSetProto.FeatureSetSpec incomingFeatureSet = (new FeatureSet(
359+
"f3", 5, 100L, Arrays.asList(f3e1), Arrays.asList(f3f2, f3f1), defaultSource)).toProto();
360+
361+
FeatureSetSpec expected = incomingFeatureSet;
362+
ApplyFeatureSetResponse applyFeatureSetResponse =
363+
specService.applyFeatureSet(incomingFeatureSet);
364+
assertThat(applyFeatureSetResponse.getStatus(), equalTo(Status.NO_CHANGE));
365+
assertThat(applyFeatureSetResponse.getFeatureSet().getMaxAge(), equalTo(expected.getMaxAge()));
366+
assertThat(applyFeatureSetResponse.getFeatureSet().getEntities(0),
367+
equalTo(expected.getEntities(0)));
368+
assertThat(applyFeatureSetResponse.getFeatureSet().getName(), equalTo(expected.getName()));
369+
}
370+
371+
334372
@Test
335373
public void shouldUpdateStoreIfConfigChanges() throws InvalidProtocolBufferException {
336374
when(storeRepository.findById("SERVING")).thenReturn(Optional.of(stores.get(0)));

0 commit comments

Comments
 (0)