[OPIK-5023] [BE] refactor(filter): introduce ENUM_LEGACY field type for source legacy fallback#5846
Conversation
…or source legacy fallback Centralise the legacy-fallback operator handling inside ANALYTICS_DB_OPERATOR_MAP by introducing FieldType.ENUM_LEGACY. The EQUAL and NOT_EQUAL templates for the new type encode the OR/AND fallback SQL directly in the map, removing the need for the separate LEGACY_FALLBACK_OPERATOR_MAP and buildWithLegacyFallback method. TraceField.SOURCE and SpanField.SOURCE are re-typed to ENUM_LEGACY so the behaviour is driven by the field type, consistent with every other operator/type pair.
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
apps/opik-backend/src/main/java/com/comet/opik/domain/filter/FilterQueryBuilder.java
Outdated
Show resolved
Hide resolved
github-actions
bot
added
the
tests
apps/opik-backend/src/main/java/com/comet/opik/api/filter/TraceField.java
Outdated
Show resolved
Hide resolved
Backend Tests - Integration Group 80 tests 0 ✅ 0s ⏱️ Results for commit a32b412. ♻️ This comment has been updated with latest results. |
Backend Tests - Integration Group 30 tests 0 ✅ 0s ⏱️ Results for commit a32b412. ♻️ This comment has been updated with latest results. |
…tils Replace duplicate getValidValue/getKey/getInvalidValue implementations in FindTraceThreadsResourceTest and GetTracesByProjectResourceTest with delegation to the shared FilterTestUtils utility class.
apps/opik-backend/src/main/java/com/comet/opik/api/filter/TraceField.java
Show resolved
Hide resolved
…ackDbValue Fix MissingFormatArgumentException in toAnalyticsDbFilter when an ENUM_LEGACY field has no legacy fallback for a given value (e.g. source = "ui"): fall back to the plain ENUM template for that operator instead of calling template.formatted with too few arguments. Replace identical legacyFallbackDbValue anonymous overrides on TraceField.SOURCE and SpanField.SOURCE with a constructor-injected Function, eliminating the duplicate enum constant body.
apps/opik-backend/src/main/java/com/comet/opik/api/filter/SpanField.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/filter/SpanField.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/filter/TraceThreadField.java
Outdated
Show resolved
Hide resolved
andrescrz
left a comment
andrescrz
left a comment
There was a problem hiding this comment.
LGTM, just a few optional comments to improve the design in the future.
|
|
||
| private final String queryParamField; | ||
| private final FieldType type; | ||
| private final Function<String, Optional<String>> legacyFallback; |
There was a problem hiding this comment.
It seems this new function is more tied to the types (ENUM_SOURCE having an actual implementation, with the rest having the default) more than to the specific fields. This way, you don't need to DRY for all fields using this new type.
There was a problem hiding this comment.
Commit ac1fa1a addressed this comment by moving the legacy fallback handling into the FieldType enum so that ENUM_LEGACY overrides buildFilter/legacyFallback instead of each field carrying its own Function. SpanField, TraceField, and TraceThreadField now simply reference FieldType.ENUM_LEGACY (with the fallback logic centralized), eliminating the need to duplicate the legacy handling per field.
There was a problem hiding this comment.
Fixed in ac1fa1a — moved the fallback logic to FieldType.ENUM_LEGACY via a buildFilter override. ENUM_LEGACY now calls Source.legacyFallbackDbValue directly and handles the OR/AND clause itself. SpanField.SOURCE, TraceField.SOURCE, and TraceThreadField.SOURCE simply declare FieldType.ENUM_LEGACY — no lambda or extra constructor needed. Adding another field with the same type automatically inherits the behaviour.
🤖 Reply posted via /address-github-pr-comments
| if (filter.field().getType() == FieldType.ENUM_LEGACY) { | ||
| return filter.field().legacyFallbackDbValue(filter.value()) |
There was a problem hiding this comment.
Related to my other comment, it'd be great if we didn't need to discriminate per type here, so we don't need to maintain as we add new types with related functionality.
There was a problem hiding this comment.
Commit ac1fa1a addressed this comment by replacing the explicit FieldType check with a call into FieldType.buildFilter, letting each type handle its own formatting. The new ENUM_LEGACY override encapsulates the legacy fallback logic, so toAnalyticsDbFilter no longer needs discriminating branches.
There was a problem hiding this comment.
Fixed in ac1fa1a — the if (getType() == ENUM_LEGACY) block is gone. toAnalyticsDbFilter now calls filter.field().getType().buildFilter(...) unconditionally; each FieldType constant decides how to format the clause. Only ENUM_LEGACY overrides buildFilter; all other types use the default 2-arg implementation. Adding a new type with special formatting only requires overriding buildFilter in FieldType — the builder stays untouched.
🤖 Reply posted via /address-github-pr-comments
| return filter.field().legacyFallbackDbValue(filter.value()) | ||
| .map(fallback -> "(%s)".formatted(template.formatted(dbField, i, fallback))) | ||
| .orElseGet(() -> "(%s)".formatted( | ||
| ANALYTICS_DB_OPERATOR_MAP.get(filter.operator()).get(FieldType.ENUM).formatted(dbField, |
There was a problem hiding this comment.
Minor: is there a possibility of not having fallback here in spite of having the outer check on ENUM_LEGACY above?
There was a problem hiding this comment.
Yes — Source.legacyFallbackDbValue only returns a value for sdk (maps to the legacy unknown DB value). For every other source (e.g. ui, online_scoring) it returns Optional.empty(), so the orElseGet path is reachable for any valid non-sdk filter. In that case the plain ENUM template is used, which correctly filters only on the new DB column value. This path now lives inside FieldType.ENUM_LEGACY.buildFilter (ac1fa1a) where the context is self-contained.
🤖 Reply posted via /address-github-pr-comments
| public class FilterTestUtils { | ||
|
|
||
| private FilterTestUtils() { |
There was a problem hiding this comment.
Nit: lombok for utils classes.
There was a problem hiding this comment.
Commit ac1fa1a addressed this comment by adding Lombok’s @UtilityClass annotation and removing the manual private constructor, conferring the desired utility-class behavior.
There was a problem hiding this comment.
Fixed in ac1fa1a — replaced the manual private constructor with @UtilityClass.
🤖 Reply posted via /address-github-pr-comments
…dType Per review feedback: - ENUM_LEGACY.buildFilter overrides the FieldType default, encoding the OR/AND legacy-clause logic directly on the type. Source::legacyFallbackDbValue is called once, in one place. - Field enums (SpanField, TraceField, TraceThreadField) no longer carry a Function<String,Optional<String>> lambda; SOURCE just uses ENUM_LEGACY. - FilterQueryBuilder.toAnalyticsDbFilter delegates to the type's buildFilter, removing the if(ENUM_LEGACY) discriminator. Adding a new type with similar behaviour only requires overriding buildFilter in FieldType. - legacyFallbackDbValue removed from Field interface; it lives on FieldType. - FilterTestUtils uses @UtilityClass instead of manual private constructor.
3 participants
Details
Centralises the legacy-fallback operator handling for the
sourcefield insideANALYTICS_DB_OPERATOR_MAP, consistent with how every other operator/type pair works.Before this change,
buildWithLegacyFallbackheld aswitch (operator)that duplicated the EQUAL/NOT_EQUAL SQL already encoded in the map. The fix:FieldType.ENUM_LEGACYwith its own EQUAL and NOT_EQUAL templates that include the OR/AND fallback clause (%3$s= legacy DB value)TraceField.SOURCEandSpanField.SOURCEre-typed toENUM_LEGACY; legacy behaviour is now driven by the field type, not a special-case methodLEGACY_FALLBACK_OPERATOR_MAPandbuildWithLegacyFallbackFiltersFactoryvalidation map updated with the new typeChange checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
mvn compile -DskipTests— cleanmvn spotless:apply— no formatting changes neededSpansResourceTestandTracesResourceTestcover source filtering (EQUAL / NOT_EQUAL) including the legacy fallback pathDocumentation
N/A