ESQL: More docs #98890
ESQL: More docs #98890
Conversation
Adds some more typed docs to the ESQL functions.
|
Documentation preview: |
elasticsearchmachine
added
Team:Docs
|
Pinging @elastic/es-docs (Team:Docs) |
|
Pinging @elastic/es-ql (Team:QL) |
|
Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL) |
|
This also adds a few more type tests for ESQL functions as that's how you make the docs. |
| arg1 | arg2... | result | ||
| boolean | boolean | ||
| boolean | boolean | boolean |
There was a problem hiding this comment.
praise: that is much cleaner and clearer IMO.
There was a problem hiding this comment.
At some point we'll want this to be able to promote as part of these - like COALESCE(12, 123432153) becoming a LONG instead of an error. We'll want some way to say that with these examples - and I think something like this is right for it.
| result | ||
| integer | double |
There was a problem hiding this comment.
thought: Hm, if we still emit this table for constants like e, at all, then I wonder if it wouldn't be better to still place the result type here, i.e.
double
instead of an empty line.
Then, we could consistently add the type table to both constants and functions.
There was a problem hiding this comment.
I hadn't even looked at this table for the constant functions. I had figured we'd build the file because it didn't hurt but that we wouldn't use it. I suppose it's pretty simple to make it useful though.
| @@ -222,34 +226,32 @@ public String toString() { | |||
| } | |||
|
|
|||
| /** | |||
| * Generate positive test cases for binary functions that operate on an {@code numeric} | |||
| * Generate positive test cases for unary functions that operate on an {@code numeric} | |||
| * fields by casting them to {@link DataTypes#DOUBLE}s. | |||
| */ | |||
| public static List<TestCaseSupplier> forUnaryCastingToDouble(String name, String argName, DoubleUnaryOperator expected) { | |||
There was a problem hiding this comment.
thought (highly out of scope for this PR): I wonder if e.g. refactors will be harder in the future because of the fact that AbstractFunctionTestCase has two separate responsibilities at the moment, namely
- serving as base class for function tests, and
- controlling how docs for functions are being auto-generated.
I imagine that we'd maybe want to separate this in the future. For instance, we could add a method to our function classes that returns the allowed type combinations + result; then AbstractFunctionTestCase could generate negative tests for any type combinations that were not marked as allowed, and a separate class could generate docs from the allowed type combinations+results.
There was a problem hiding this comment.
I suppose this could become a problem in a few ways:
- We want to document type combinations we aren't testing.
- We want to test combinations we don't document
- Everything just gets gummed up together
I think 1 is fairly unlikely and generally a bad thing to want. We already have 2 and have some work arounds for it, but they aren't really "pluggable", just heuristics like "the number of arguments is the same". That seems ok for now.
3 is a bigger concern, though one I think I'd like to ignore until it actually happens.
There was a problem hiding this comment.
I agree that we want 1 and 2, and am a bit afraid of 3; I think we're already doing the right thing, I'm just wondering if we can avoid gumming up by having our functions actively declare what type combos are supported, rather than inferring the supported types implicitly from the tests; we can still ensure 1 and 2 this way, I think.
| String read = "Attribute[channel=0]"; | ||
| List<TestCaseSupplier> suppliers = new ArrayList<>(); | ||
| TestCaseSupplier.forUnaryInt(suppliers, "Log10IntEvaluator[val=" + read + "]", DataTypes.DOUBLE, Math::log10); | ||
| TestCaseSupplier.forUnaryLong(suppliers, "Log10LongEvaluator[val=" + read + "]", DataTypes.DOUBLE, Math::log10); | ||
| TestCaseSupplier.forUnaryUnsignedLong( | ||
| suppliers, | ||
| "Log10UnsignedLongEvaluator[val=" + read + "]", | ||
| DataTypes.DOUBLE, | ||
| ul -> Math.log10(ul.doubleValue()) | ||
| ); | ||
| TestCaseSupplier.forUnaryDouble(suppliers, "Log10DoubleEvaluator[val=" + read + "]", DataTypes.DOUBLE, Math::log10); | ||
| return parameterSuppliersFromTypedData(errorsForCasesWithoutExamples(anyNullIsNull(true, suppliers))); |
There was a problem hiding this comment.
praise: this is neat and it's becoming very easy to add quite comprehensive tests to any new function, I like it.
nik9000
added
the
auto-merge-without-approval
Adds some more typed docs to the ESQL functions.