Skip to content

Conversation

not-napoleon
Copy link
Member

This wires up the randomized testing for DateFormat. Prior to this PR, none of the randomized testing was hitting the one parameter version of the function, so I wired that up as well. This required some compromises on the type signatures, see comments in line.

@not-napoleon not-napoleon added >test Issues or PRs that are addressing/adding tests auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v9.0.0 v8.18.0 labels Jan 10, 2025
Copy link
Contributor

Documentation preview:

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 10, 2025
)
public DateFormat(
Source source,
@Param(optional = true, name = "dateFormat", type = { "keyword", "text" }, description = """
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first positional argument can be a date, in the one parameter version of the function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's quite a weird way to do it, but that's how the tests work.

We could, optionally, hack around this in the tests somehow. Or teach them about first position optional arguments. It's just that we don't have that many!

*/
public static List<TypedDataSupplier> dateFormatCases() {
return List.of(
new TypedDataSupplier("<format as KEYWORD>", () -> new BytesRef(ESTestCase.randomDateFormatterPattern()), DataType.KEYWORD),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only generates the named formats, as far as I can tell. Would be good to write something that generates arbitrary format strings too, but I didn't see such a thing and this is still more coverage than we had.

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's an improvement. I agree the extra thing on the parameter is lame.

)
public DateFormat(
Source source,
@Param(optional = true, name = "dateFormat", type = { "keyword", "text" }, description = """
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's quite a weird way to do it, but that's how the tests work.

We could, optionally, hack around this in the tests somehow. Or teach them about first position optional arguments. It's just that we don't have that many!

@not-napoleon not-napoleon merged commit e9f2d78 into elastic:main Jan 13, 2025
16 checks passed
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

not-napoleon added a commit to not-napoleon/elasticsearch that referenced this pull request Jan 13, 2025
This wires up the randomized testing for DateFormat. Prior to this PR, none of the randomized testing was hitting the one parameter version of the function, so I wired that up as well. This required some compromises on the type signatures, see comments in line.less

---------

Co-authored-by: elasticsearchmachine <[email protected]>
elasticsearchmachine pushed a commit that referenced this pull request Jan 13, 2025
This wires up the randomized testing for DateFormat. Prior to this PR, none of the randomized testing was hitting the one parameter version of the function, so I wired that up as well. This required some compromises on the type signatures, see comments in line.less

---------

Co-authored-by: elasticsearchmachine <[email protected]>
martijnvg pushed a commit to martijnvg/elasticsearch that referenced this pull request Jan 14, 2025
This wires up the randomized testing for DateFormat. Prior to this PR, none of the randomized testing was hitting the one parameter version of the function, so I wired that up as well. This required some compromises on the type signatures, see comments in line.less

---------

Co-authored-by: elasticsearchmachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v8.18.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants