ESQL: Instructions for functions #98648
Conversation
Adds package javadoc for ESLQ's `function.scalar` package with instructions on how to write a function.
|
Pinging @elastic/es-docs (Team:Docs) |
elasticsearchmachine
added
Team:Docs
|
Pinging @elastic/es-ql (Team:QL) |
|
Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL) |
|
This renders as: Though I expect most folks will read it in javadoc form. |
alex-spies
left a comment
alex-spies
left a comment
There was a problem hiding this comment.
Looks good, only one minor remark regarding formatting.
| * <li>Fork the <a href="github.com/elastic/elasticsearch">Elasticsearch repo</a>.</li> | ||
| * <li>Clone your fork locally.</li> | ||
| * <li>Add Elastic's remote, it should look a little like: | ||
| * {@code<pre> |
There was a problem hiding this comment.
I don't think this is rendered correctly in the docs; the <pre> tag is printed verbatim and everything is squished into one line, going by your screenshot. I guess the tag needs to be outside the {@code ... } block?
Also applies in other sections below.
There was a problem hiding this comment.
Also applies in other sections below.
Ah! I was mostly checking within intellij and just quickly eyeballed the screenshot. I'll fix.
| * </li> | ||
| * <li> | ||
| * Find a function in this package similar to the one you are working on and copy it to build | ||
| * yours. There's come ceremony required in each function class to make it constant foldable |
There was a problem hiding this comment.
There's come -> There's some
| * serializable over the wire. Mostly you can copy existing implementations for both. | ||
| * </li> | ||
| * <li> | ||
| * Rerun the {@code CsvTests}. They should find your function and maybe even pass. Add a |
There was a problem hiding this comment.
Imo, we should instruct authors to also consider one-two more complex tests with the newly added functions (functions passed as parameters to other functions for example), these types of queries can sometimes reveal some flaws in optimization rules, or indicate that some core methods of that specific function were not implemented/overriden or poorly implemented.
| * {@code<pre>./gradlew -p x-pack/plugin/esql/ spotlessApply</pre>} | ||
| * </li> | ||
| * <li> | ||
| * Now you can run all of the ESQL tests liks CI: |
There was a problem hiding this comment.
tests liks CI -> tests like CI
astefan
left a comment
astefan
left a comment
There was a problem hiding this comment.
Looking good. Left some minor comments.
nik9000
added
the
auto-merge-without-approval
bpintea
left a comment
bpintea
left a comment
There was a problem hiding this comment.
LGTM, just one noteworthy suggestion.
| * an {@link org.elasticsearch.compute.operator.EvalOperator.ExpressionEvaluator} implementation | ||
| * calling the method annotated with {@link org.elasticsearch.compute.ann.Evaluator}. | ||
| * <li> | ||
| * Once your evaluator is generated you can implement {@link org.elasticsearch.xpack.esql.planner.Mappable#toEvaluator}, |
There was a problem hiding this comment.
Past #98628:
| * Once your evaluator is generated you can implement {@link org.elasticsearch.xpack.esql.planner.Mappable#toEvaluator}, | |
| * Once your evaluator is generated you can implement {@link org.elasticsearch.xpack.esql.evaluator.mapper.EvaluatorMapper#toEvaluator}, |
| * Now it's time to make a unit test! The infrastructure for these is under some flux at | ||
| * the moment, but it's good to extend from {@code AbstractScalarFunctionTestCase}. All of |
There was a problem hiding this comment.
| * Now it's time to make a unit test! The infrastructure for these is under some flux at | |
| * the moment, but it's good to extend from {@code AbstractScalarFunctionTestCase}. All of | |
| * Now it's time to make a unit test! The infrastructure for these might change, but it's good to extend from {@code AbstractScalarFunctionTestCase}. All of |
While admittedly everything about this guide might change, it's likely we'll forget to update this sentence short term and this will remain in flux for a longer while.
| * isn't a strong guiding principle in the theme. | ||
| * </li> | ||
| * <li> | ||
| * Rerun the {@code CsvTests} and watch your new test fail. Yay, TDD doing it's job. |
There was a problem hiding this comment.
| * Rerun the {@code CsvTests} and watch your new test fail. Yay, TDD doing it's job. | |
| * Rerun the {@code CsvTests} and watch your new test fail. Yay, TDD FTW. |
😎
6 participants
Adds package javadoc for ESQL's
function.scalarpackage with instructions on how to write a function.