ES|QL: make capabilities an enum#109935
Conversation
| private static final String FN_CBRT = "fn_cbrt"; | ||
| public enum Cap { | ||
| // Support for function {@code CBRT}. Done in #108574. | ||
| FN_CBRT(false), |
There was a problem hiding this comment.
Should these be lowercase since the beginning? Yeah, it's ugly, but it would be handy
There was a problem hiding this comment.
I prefer it the way you did it, with capitalized enum values, and lower case registration.
There was a problem hiding this comment.
👍 on capitalized values.
I'd probably make a second ctor that defaults to false - it's just so so so much more common. Usually I wouldn't do that for things, but for this I think it's worth it.
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
Show resolved
Hide resolved
| for (NodeFeature feature : new EsqlFeatures().getHistoricalFeatures().keySet()) { | ||
| caps.add(cap(feature)); | ||
| } | ||
| caps.add(STRING_LITERAL_AUTO_CASTING_TO_DATETIME_ADD_SUB); |
There was a problem hiding this comment.
I always wondered why this was registered after the others, as if it mattered somehow?
There was a problem hiding this comment.
I have no idea, but my feeling is that the order doesn't really matter
| private static final String FN_CBRT = "fn_cbrt"; | ||
| public enum Cap { | ||
| // Support for function {@code CBRT}. Done in #108574. | ||
| FN_CBRT(false), |
There was a problem hiding this comment.
I prefer it the way you did it, with capitalized enum values, and lower case registration.
alex-spies
left a comment
There was a problem hiding this comment.
Jinx!
Your version is even less verbose than mine, let's go with it!
| */ | ||
| private static final String FN_CBRT = "fn_cbrt"; | ||
| enum Cap { | ||
| // Support for function {@code CBRT}. Done in #108574. |
There was a problem hiding this comment.
Let's leave this as javadoc, so it shows up when hovering.
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| private static final String FN_CBRT = "fn_cbrt"; | ||
| public enum Cap { | ||
| // Support for function {@code CBRT}. Done in #108574. | ||
| FN_CBRT(false), |
There was a problem hiding this comment.
👍 on capitalized values.
I'd probably make a second ctor that defaults to false - it's just so so so much more common. Usually I wouldn't do that for things, but for this I think it's worth it.
So that registration is automatic.
Let's merge it after #109926 to make sure all the tests are good