Conversation
|
|
||
| public Set<DataType> supportedTypes() { | ||
| return factories().keySet(); | ||
| Set<DataType> supportedTypes = new HashSet<>(); |
There was a problem hiding this comment.
it's either this check here or adding a capabilities check for each function/operator on the evaluators list.
if that's the case we should probably do the same for date_nanos as well.
but I preferred this check than modifying the evaluators list.
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
currently checking a much simpler fix by just removing this check: and instead just have the type should continue to be |
craigtaverner
left a comment
There was a problem hiding this comment.
I expected this change to mostly impact unit tests, which it seems to have done. I did have a suggestion for the supportedTypes method, which you could consider.
| } | ||
| supportedTypes.add(dataType); | ||
| } | ||
| return supportedTypes; |
There was a problem hiding this comment.
Would this be more concise, and perhaps easier to read?
return EsqlCapabilities.Cap.SEMANTIC_TEXT_TYPE.isEnabled()
? factories().keySet()
: factories().keySet().stream().filter(dataType -> dataType != DataType.SEMANTIC_TEXT).collect(Collectors.toSet());
|
closing in favour of #116202 |
Because
semantic_textis available only in snapshots some of the tests fail when running with-Dbuild.snapshot=falsesince I did not add explicit checks forEsqlCapabilities.Cap.SEMANTIC_TEXT.I assumed that adding
semantic_textinUNDER_CONSTRUCTIONwould take care of that.Examples of release test failing on a different PR: #114831
To check I have run: