ESQL Javadoc for creating new data types #117520
Conversation
not-napoleon
added
>non-issue
auto-backport
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Bogdan Pintea <pintea@mailbox.org>
Co-authored-by: Bogdan Pintea <pintea@mailbox.org>
Co-authored-by: Bogdan Pintea <pintea@mailbox.org>
Co-authored-by: Bogdan Pintea <pintea@mailbox.org>
| * supporting specific functions. This is fine, and expected.</li> | ||
| * <li>Create a new CSV test file for the new type. Depending on the type, | ||
| * you may also need to create a new sample data file if none of the | ||
| * existing sample data files have fields of that type. See |
There was a problem hiding this comment.
Won't they sort of definitionally not be in the test data? They are new and couldn't have been loaded before at all.
| * and from the ROW command. It should also include functions that support | ||
| * "every" type, such as Case or MvFirst.</li> | ||
| * <li>Add the new type to the CsvTestUtils#Type enum, if it isn't already | ||
| * there. You may also need to modify CsvAssert to support reading values |
There was a problem hiding this comment.
How could it be there already if it's new?
| * <li>Add the new data type to this enum. This will cause a bunch of | ||
| * compile errors for switch statements throughout the code. Resolve those | ||
| * as appropriate. That is the main way in which the new type will be tied | ||
| * into the framework.</li> |
There was a problem hiding this comment.
Maybe a note that throw new UnsupportedOperationException("TODO " + YOUR_FEATURE_FLAG) is fine because we're behind a feature flag.
There was a problem hiding this comment.
My recollection is that most of the places this step will fail are trivial to resolve, so I don't really think that's necessary? I can add it in if you think it's a good idea, but my gut feeling is that's likely to cause more headache than it solves.
| * collection. This is used by the test framework to disable some checks | ||
| * around how functions report their supported types, which would otherwise | ||
| * generate a lot of noise while the type is still in development.</li> | ||
| * <li>Add typed data generators to TestCaseSupplier, and make sure all |
There was a problem hiding this comment.
I think your first PR should end somewhere around here, right? Maybe a note about that?
There was a problem hiding this comment.
I was trying to avoid being prescriptive about how to break up PRs.
There was a problem hiding this comment.
Yeah. I agree on not being prescriptive. But maybe "this is a good place to stop the first PR" or, I dunno. Something. Or not. It's fine.
| * doesn't support, but require special handling anyway (e.g. | ||
| * {@link DataType#OBJECT}) | ||
| * | ||
| * <h2>Process for adding a new data type</h2> |
There was a problem hiding this comment.
Just a comment: Should this be here in the class javadoc? I think we have "how to's" all spread around different places (package-infos, docs?, now classes).
Better here than nowhere I guess, but it feels strange to have this kind of docs in a class
There was a problem hiding this comment.
I'm open to suggestions for a better place to put it. This seemed the most findable to me at the time.
There was a problem hiding this comment.
Maybe we should link it from the main esql package-info root docs? So it will be easier to find, and we won't forget about it
| * appropriate. New data types are complex, and smaller PRs will make reviews | ||
| * easier. | ||
| * <ul> | ||
| * <li>Create a new feature flag for the type in {@link EsqlCorePlugin}. We |
There was a problem hiding this comment.
nit: If the <li>'s content is indented, it's more readable as plain text 👀
There was a problem hiding this comment.
I think I've done what you want here. Please let me know if that's not what you meant.
There was a problem hiding this comment.
Yep, looks better. Those </li> being sometimes in the same line and sometimes in a new line tho... :hehe:
This adds some java doc to the DataType enum, listing out the steps I followed for adding DateNanos. Hopefully it's helpful to future folks adding data types. --------- Co-authored-by: Bogdan Pintea <pintea@mailbox.org>
💚 Backport successful
|
This adds some java doc to the
DataTypeenum, listing out the steps I followed for adding DateNanos. Hopefully it's helpful to future folks adding data types.