ES|QL: Linear combination in FUSE#134543
Conversation
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
| }; | ||
| } | ||
|
|
||
| private abstract class Normalizer { |
There was a problem hiding this comment.
Nit - Is this an interface, as it does not provide any method implementation?
| public record Factory(int forkPosition, int scorePosition, double rankConstant, Map<String, Double> weights) | ||
| implements | ||
| OperatorFactory { | ||
| public record Factory(int discriminatorPosition, int scorePosition, RrfConfig rrfConfig) implements OperatorFactory { |
There was a problem hiding this comment.
Nice to see the Config classes used ❤️
| } | ||
| } | ||
|
|
||
| public void testLinearInvalidOptions() { |
There was a problem hiding this comment.
Can we move these to the AnalyzerTests / StatementParserTests? It would be nice not needing to do an IT for checking the options
There was a problem hiding this comment.
I think I have tests for these in AnalyzerTests so I can just remove them
| }); | ||
| } | ||
|
|
||
| private void validateLinearOptions(Failures failures) { |
There was a problem hiding this comment.
I'm thinking on moving these to the enum themselves - it's sad that we're dealing with parsing concerns, but could be pretty neat to delegate the parsing to them. WDYT?
There was a problem hiding this comment.
I need to think a bit more about this 🤔 . There are other commands that have options too and I am not sure if there's an opportunity to have a unified way to parse/validate options for commands.
Okay if I follow up on this separately?
There was a problem hiding this comment.
Okay if I follow up on this separately?
Yes, it's unclear to me if that's a good option or not. As you say, let's keep an eye on this and do some refactoring if it makes sense in the future.
meta issue: #123389
Adds linear combination support for FUSE: