diff --git a/core/src/main/java/feast/core/model/FeatureTable.java b/core/src/main/java/feast/core/model/FeatureTable.java index 6d9c414644..90cd79e6cf 100644 --- a/core/src/main/java/feast/core/model/FeatureTable.java +++ b/core/src/main/java/feast/core/model/FeatureTable.java @@ -25,8 +25,6 @@ import feast.proto.core.FeatureTableProto; import feast.proto.core.FeatureTableProto.FeatureTableSpec; import java.util.Collection; -import java.util.Collections; -import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; @@ -158,7 +156,8 @@ public static FeatureTable fromProto( * @param spec the Protobuf spec to update the FeatureTable from. * @throws IllegalArgumentException if the update will make prohibited changes. */ - public void updateFromProto(FeatureTableSpec spec) { + public void updateFromProto( + String projectName, FeatureTableSpec spec, EntityRepository entityRepo) { // Check for prohibited changes made in spec: // - Name cannot be changed if (!getName().equals(spec.getName())) { @@ -167,16 +166,11 @@ public void updateFromProto(FeatureTableSpec spec) { "Updating the name of a registered FeatureTable is not allowed: %s to %s", getName(), spec.getName())); } - // - Entities cannot be changed - List entityNames = - getEntities().stream().map(EntityV2::getName).collect(Collectors.toList()); - if (!new HashSet<>(entityNames).equals(new HashSet<>(spec.getEntitiesList()))) { - Collections.sort(entityNames); - throw new IllegalArgumentException( - String.format( - "Updating the entities of a registered FeatureTable is not allowed: %s to %s", - entityNames, spec.getEntitiesList())); - } + // Update Entities if changed + Set entities = + FeatureTable.resolveEntities( + projectName, spec.getName(), entityRepo, spec.getEntitiesList()); + this.setEntities(entities); // Update FeatureTable based on spec // Update existing features, create new feature, drop missing features diff --git a/core/src/main/java/feast/core/model/FeatureV2.java b/core/src/main/java/feast/core/model/FeatureV2.java index ee969ece99..e10d51647c 100644 --- a/core/src/main/java/feast/core/model/FeatureV2.java +++ b/core/src/main/java/feast/core/model/FeatureV2.java @@ -23,11 +23,14 @@ import java.util.Objects; import javax.persistence.*; import javax.persistence.Entity; +import lombok.AccessLevel; import lombok.Getter; +import lombok.Setter; /** Defines a single Feature defined in a {@link FeatureTable} */ @Getter @Entity +@Setter(AccessLevel.PRIVATE) @Table( name = "features_v2", uniqueConstraints = @UniqueConstraint(columnNames = {"name", "feature_table_id"})) @@ -96,12 +99,8 @@ public void updateFromProto(FeatureSpecV2 spec) { "Updating the name of a registered Feature is not allowed: %s to %s", getName(), spec.getName())); } - if (!getType().equals(spec.getValueType())) { - throw new IllegalArgumentException( - String.format( - "Updating the value type of a registered Feature is not allowed: %s to %s", - getType(), spec.getValueType())); - } + // Update feature type + this.setType(spec.getValueType()); // Update Feature based on spec this.labelsJSON = TypeConversion.convertMapToJsonString(spec.getLabelsMap()); diff --git a/core/src/main/java/feast/core/service/SpecService.java b/core/src/main/java/feast/core/service/SpecService.java index f570167c21..0db898d12e 100644 --- a/core/src/main/java/feast/core/service/SpecService.java +++ b/core/src/main/java/feast/core/service/SpecService.java @@ -671,7 +671,7 @@ public ApplyFeatureTableResponse applyFeatureTable(ApplyFeatureTableRequest requ return ApplyFeatureTableResponse.newBuilder().setTable(existingTable.get().toProto()).build(); } if (existingTable.isPresent()) { - existingTable.get().updateFromProto(applySpec); + existingTable.get().updateFromProto(projectName, applySpec, entityRepository); table = existingTable.get(); } diff --git a/core/src/test/java/feast/core/service/SpecServiceIT.java b/core/src/test/java/feast/core/service/SpecServiceIT.java index 35535d0715..541580718c 100644 --- a/core/src/test/java/feast/core/service/SpecServiceIT.java +++ b/core/src/test/java/feast/core/service/SpecServiceIT.java @@ -1108,39 +1108,9 @@ public void shouldUpdateExistingTableWithValidSpec() { } @Test - public void shouldNotUpdateIfNoChanges() { - FeatureTableProto.FeatureTable table = apiClient.applyFeatureTable("default", getTestSpec()); - FeatureTableProto.FeatureTable updatedTable = - apiClient.applyFeatureTable("default", getTestSpec()); - - assertThat(updatedTable.getMeta().getRevision(), equalTo(table.getMeta().getRevision())); - } - - @Test - public void shouldErrorOnMissingBatchSource() { - FeatureTableProto.FeatureTableSpec spec = - DataGenerator.createFeatureTableSpec( - "ft", - List.of("entity1"), - Map.of("event_timestamp", ValueProto.ValueType.Enum.INT64), - 3600, - Map.of()) - .toBuilder() - .build(); - - StatusRuntimeException exc = - assertThrows( - StatusRuntimeException.class, () -> apiClient.applyFeatureTable("default", spec)); - - assertThat( - exc.getMessage(), - equalTo("INVALID_ARGUMENT: FeatureTable batch source cannot be empty.")); - } - - @Test - public void shouldErrorIfEntityChangeOnUpdate() { + public void shouldUpdateFeatureTableOnEntityChange() { List entities = Arrays.asList("entity1", "entity2"); - FeatureTableProto.FeatureTableSpec spec = + FeatureTableProto.FeatureTableSpec updatedSpec = DataGenerator.createFeatureTableSpec( "featuretable1", Arrays.asList("entity1"), @@ -1157,21 +1127,15 @@ public void shouldErrorIfEntityChangeOnUpdate() { DataGenerator.createFileDataSourceSpec("file:///path/to/file", "ts_col", "")) .build(); - StatusRuntimeException exc = - assertThrows( - StatusRuntimeException.class, () -> apiClient.applyFeatureTable("default", spec)); + FeatureTableProto.FeatureTable updatedTable = + apiClient.applyFeatureTable("default", updatedSpec); - assertThat( - exc.getMessage(), - equalTo( - String.format( - "INVALID_ARGUMENT: Updating the entities of a registered FeatureTable is not allowed: %s to %s", - entities, spec.getEntitiesList()))); + assertTrue(TestUtil.compareFeatureTableSpec(updatedTable.getSpec(), updatedSpec)); } @Test - public void shouldErrorIfFeatureValueTypeChangeOnUpdate() { - FeatureTableProto.FeatureTableSpec spec = + public void shouldUpdateFeatureTableOnFeatureTypeChange() { + FeatureTableProto.FeatureTableSpec updatedSpec = DataGenerator.createFeatureTableSpec( "featuretable1", Arrays.asList("entity1", "entity2"), @@ -1188,16 +1152,40 @@ public void shouldErrorIfFeatureValueTypeChangeOnUpdate() { DataGenerator.createFileDataSourceSpec("file:///path/to/file", "ts_col", "")) .build(); + FeatureTableProto.FeatureTable updatedTable = + apiClient.applyFeatureTable("default", updatedSpec); + + assertTrue(TestUtil.compareFeatureTableSpec(updatedTable.getSpec(), updatedSpec)); + } + + @Test + public void shouldNotUpdateIfNoChanges() { + FeatureTableProto.FeatureTable table = apiClient.applyFeatureTable("default", getTestSpec()); + FeatureTableProto.FeatureTable updatedTable = + apiClient.applyFeatureTable("default", getTestSpec()); + + assertThat(updatedTable.getMeta().getRevision(), equalTo(table.getMeta().getRevision())); + } + + @Test + public void shouldErrorOnMissingBatchSource() { + FeatureTableProto.FeatureTableSpec spec = + DataGenerator.createFeatureTableSpec( + "ft", + List.of("entity1"), + Map.of("event_timestamp", ValueProto.ValueType.Enum.INT64), + 3600, + Map.of()) + .toBuilder() + .build(); + StatusRuntimeException exc = assertThrows( StatusRuntimeException.class, () -> apiClient.applyFeatureTable("default", spec)); assertThat( exc.getMessage(), - equalTo( - String.format( - "INVALID_ARGUMENT: Updating the value type of a registered Feature is not allowed: %s to %s", - ValueProto.ValueType.Enum.FLOAT, ValueProto.ValueType.Enum.STRING_LIST))); + equalTo("INVALID_ARGUMENT: FeatureTable batch source cannot be empty.")); } @Test