From 1aa13622cc654482e07d39bc8906762199641a49 Mon Sep 17 00:00:00 2001 From: Terence Date: Thu, 2 Jul 2020 11:57:20 +0800 Subject: [PATCH 1/2] Fix applyFeatureSet to propagate label updates --- .../main/java/feast/core/model/Feature.java | 3 +- .../java/feast/core/model/FeatureSet.java | 3 +- .../feast/core/service/SpecServiceTest.java | 36 +++++++++++++++++++ 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/feast/core/model/Feature.java b/core/src/main/java/feast/core/model/Feature.java index 7229f13af87..83b82b48e7e 100644 --- a/core/src/main/java/feast/core/model/Feature.java +++ b/core/src/main/java/feast/core/model/Feature.java @@ -235,11 +235,12 @@ public void archive() { } /** - * Update the feature object with a valid feature spec. Only schema changes are allowed. + * Update the feature object with a valid feature spec. * * @param featureSpec {@link FeatureSpec} containing schema changes. */ public void updateFromProto(FeatureSpec featureSpec) { + this.setLabels(TypeConversion.convertMapToJsonString(featureSpec.getLabelsMap())); if (isArchived()) { throw new IllegalArgumentException( String.format( diff --git a/core/src/main/java/feast/core/model/FeatureSet.java b/core/src/main/java/feast/core/model/FeatureSet.java index 89d8c65440c..e56c010a3eb 100644 --- a/core/src/main/java/feast/core/model/FeatureSet.java +++ b/core/src/main/java/feast/core/model/FeatureSet.java @@ -314,9 +314,10 @@ public void updateFromProto(FeatureSetProto.FeatureSet featureSetProto) spec.getEntitiesList(), existingEntities)); } - // 4. Update max age and source. + // 4. Update max age, source and labels. maxAgeSeconds = spec.getMaxAge().getSeconds(); source = Source.fromProto(spec.getSource()); + this.setLabels(TypeConversion.convertMapToJsonString(spec.getLabelsMap())); Map updatedFeatures = spec.getFeaturesList().stream().collect(Collectors.toMap(FeatureSpec::getName, fs -> fs)); diff --git a/core/src/test/java/feast/core/service/SpecServiceTest.java b/core/src/test/java/feast/core/service/SpecServiceTest.java index 3d0a3a75d5d..48572ddcecf 100644 --- a/core/src/test/java/feast/core/service/SpecServiceTest.java +++ b/core/src/test/java/feast/core/service/SpecServiceTest.java @@ -700,6 +700,42 @@ public void applyFeatureSetShouldAcceptFeatureLabels() throws InvalidProtocolBuf assertEquals(appliedFeatureSpecsLabels, featureLabels); } + @Test + public void applyFeatureSetShouldUpdateLabels() throws InvalidProtocolBufferException { + FeatureSpec updatedFeature = + FeatureSpec.newBuilder().setName("feature").setValueType(Enum.STRING).build(); + + FeatureSet featureSet = featureSets.get(0); + FeatureSetSpec featureSetSpec = featureSet.toProto().getSpec().toBuilder().build(); + Map featureSetLabels = + new HashMap<>() { + { + put("fsLabel1", "fsValue1"); + } + }; + + FeatureSetProto.FeatureSet incomingFeatureSet = + FeatureSetProto.FeatureSet.newBuilder() + .setSpec( + featureSetSpec + .toBuilder() + .setFeatures(0, updatedFeature) + .putAllLabels(featureSetLabels) + .build()) + .build(); + + ApplyFeatureSetResponse applyFeatureSetResponse = + specService.applyFeatureSet(incomingFeatureSet); + FeatureSetProto.FeatureSet updatedFs = applyFeatureSetResponse.getFeatureSet(); + Map updatedFsLabels = updatedFs.getSpec().getLabelsMap(); + + Map updatedFeatureLabels = updatedFs.getSpec().getFeatures(0).getLabelsMap(); + Map emptyFeatureLabels = new HashMap<>(); + + assertEquals(featureSetLabels, updatedFsLabels); + assertEquals(emptyFeatureLabels, updatedFeatureLabels); + } + @Test public void applyFeatureSetShouldAcceptFeatureSetLabels() throws InvalidProtocolBufferException { Map featureSetLabels = From c51fb4f3db496492bed6edbb9f1caedfedc3718b Mon Sep 17 00:00:00 2001 From: Terence Date: Thu, 2 Jul 2020 14:19:10 +0800 Subject: [PATCH 2/2] Address PR comments --- core/src/main/java/feast/core/model/Feature.java | 2 +- core/src/main/java/feast/core/model/FeatureSet.java | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/feast/core/model/Feature.java b/core/src/main/java/feast/core/model/Feature.java index 83b82b48e7e..b387f3403bb 100644 --- a/core/src/main/java/feast/core/model/Feature.java +++ b/core/src/main/java/feast/core/model/Feature.java @@ -240,7 +240,6 @@ public void archive() { * @param featureSpec {@link FeatureSpec} containing schema changes. */ public void updateFromProto(FeatureSpec featureSpec) { - this.setLabels(TypeConversion.convertMapToJsonString(featureSpec.getLabelsMap())); if (isArchived()) { throw new IllegalArgumentException( String.format( @@ -253,6 +252,7 @@ public void updateFromProto(FeatureSpec featureSpec) { "You are attempting to change the type of feature %s from %s to %s. This isn't allowed. Please create a new feature.", featureSpec.getName(), type, featureSpec.getValueType())); } + this.setLabels(TypeConversion.convertMapToJsonString(featureSpec.getLabelsMap())); updateSchema(featureSpec); } diff --git a/core/src/main/java/feast/core/model/FeatureSet.java b/core/src/main/java/feast/core/model/FeatureSet.java index e56c010a3eb..41008fa6625 100644 --- a/core/src/main/java/feast/core/model/FeatureSet.java +++ b/core/src/main/java/feast/core/model/FeatureSet.java @@ -314,9 +314,9 @@ public void updateFromProto(FeatureSetProto.FeatureSet featureSetProto) spec.getEntitiesList(), existingEntities)); } - // 4. Update max age, source and labels. - maxAgeSeconds = spec.getMaxAge().getSeconds(); - source = Source.fromProto(spec.getSource()); + // 2. Update max age, source and labels. + this.maxAgeSeconds = spec.getMaxAge().getSeconds(); + this.source = Source.fromProto(spec.getSource()); this.setLabels(TypeConversion.convertMapToJsonString(spec.getLabelsMap())); Map updatedFeatures =