[OPIK-622] Remove deprecated dataset item fields #1003
Conversation
…mn-named-input-with-non-dictionary-type-values
There was a problem hiding this comment.
For the implementation part, we should keep removing a few more related things that become unused.
Please review the tests carefully. Filtering might still be legitimate on those fields, but as dynamic fields within data. Also, tests related to the migration or the deprecated fields might go away entirely. But please double check that my guidance and assumptions here are correct.
| @@ -9,12 +9,12 @@ public class DatasetItemInputValidator implements ConstraintValidator<DatasetIte | |||
|
|
|||
| @Override | |||
| public boolean isValid(DatasetItem datasetItem, ConstraintValidatorContext context) { | |||
| boolean result = datasetItem.input() != null || MapUtils.isNotEmpty(datasetItem.data()); | |||
| boolean result = MapUtils.isNotEmpty(datasetItem.data()); | |||
There was a problem hiding this comment.
I believe this whole class doesn't make sense anymore. You should completely remove it, along with the related annotation and then simply add a @NotEmpty annotation to the data field of DatasetItem, which works with collections (null or not empty on the map).
Please make sure there's a test covering that this works fine.
|
|
||
| getItemAndAssert(item, TEST_WORKSPACE, API_KEY); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("when input is null but data is present, the accept the request") |
There was a problem hiding this comment.
Please review the display name of the test and everything else.
| arguments(new ExperimentsComparisonFilter("output", | ||
| FieldType.STRING, Operator.CONTAINS, null, "sql_cost")), | ||
| arguments(new ExperimentsComparisonFilter("expected_output", | ||
| FieldType.DICTIONARY, Operator.CONTAINS, "output", "sql_cost")), | ||
| arguments(new ExperimentsComparisonFilter("metadata", | ||
| FieldType.DICTIONARY, Operator.EQUAL, "sql_cost", "10")), |
There was a problem hiding this comment.
I believe you shouldn't remove these tests, as it's still legitimate to filter on those fields, but within data field.
| .input(item.input()) | ||
| .output(item.expectedOutput()) |
There was a problem hiding this comment.
Related: legitimate payload in the trace, but should come from data.
| .input(JsonUtils | ||
| .getJsonNodeFromString(JsonUtils.writeValueAsString(Map.of("input", "sql_cost")))) | ||
| .expectedOutput(JsonUtils | ||
| .getJsonNodeFromString(JsonUtils.writeValueAsString(Map.of("output", "sql_cost")))) | ||
| .metadata(JsonUtils | ||
| .getJsonNodeFromString(JsonUtils.writeValueAsString(Map.of("sql_cost", 10)))) |
There was a problem hiding this comment.
It seems legitimate, but within data.
| .bind("input", datasetItem.input().toString()) | ||
| .bind("expected_output", datasetItem.expectedOutput().toString()) | ||
| .bind("metadata", datasetItem.metadata().toString()) |
There was a problem hiding this comment.
Probably the whole TestNoMigratedDatasetItemRetrieval doesn't make sense anymore, as we're removing the deprecated fields.
| columns.add(Column.builder().name("input").types(Set.of(ColumnType.OBJECT)).filterFieldPrefix("data").build()); | ||
| columns.add(Column.builder().name("expected_output").types(Set.of(ColumnType.OBJECT)).filterFieldPrefix("data") | ||
| .build()); | ||
| columns.add( | ||
| Column.builder().name("metadata").types(Set.of(ColumnType.OBJECT)).filterFieldPrefix("data").build()); |
There was a problem hiding this comment.
Probably the whole addDeprecatedFields doesn't make sense anymore, as we're removing the deprecated fields entirely.
| JsonNode input = getJsonNode(row, data, "input"); | ||
| JsonNode expectedOutput = getJsonNode(row, data, "expected_output"); | ||
| JsonNode metadata = getJsonNode(row, data, "metadata"); |
There was a problem hiding this comment.
The getJsonNode method is no longer used. Please clean up all unused methods in this class.
Details
Remove deprecated dataset item fields
Testing
Covered by existing integration tests