Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ES|QL] Allow DateTime as the third and fourth inputs to auto_bucket #104547

Merged
merged 8 commits into from
Jan 25, 2024
Merged

[ES|QL] Allow DateTime as the third and fourth inputs to auto_bucket #104547

merged 8 commits into from
Jan 25, 2024

Conversation

fang-xing-esql
Copy link
Member

@fang-xing-esql fang-xing-esql commented Jan 18, 2024 •
edited
Loading

Resolves: #102756 #104646

Currently, the AUTO_BUCKET function takes only String data type for the start and end time range. This enhances the AUTO_BUCKET function by allowing data with DateTime type as input to the start and end time range, users do not need to convert the input parameters from DateTime to String type manually.

The AUTO_BUCKET function does not allow variables as input to the start and end time range yet, this will be addressed in a separate PR.

@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) external-contributor Pull request authored by a developer outside the Elasticsearch team labels Jan 18, 2024
@nik9000 nik9000 self-requested a review January 18, 2024 17:03
nik9000
nik9000 requested changes Jan 18, 2024
View reviewed changes
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.

Looks about right! I've left a few comments asking to move some methods around and run the formatter, but it does seem to get the job done!

.../test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/AutoBucketTests.java Outdated Show resolved Hide resolved
...l/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/AutoBucket.java Outdated Show resolved Hide resolved
x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/TypeResolutions.java Outdated Show resolved Hide resolved
@fang-xing-esql
Copy link
Member Author

Looks about right! I've left a few comments asking to move some methods around and run the formatter, but it does seem to get the job done!

Thanks for reviewing! All of the comments are addressed.

@nik9000
Copy link
Member

nik9000 commented Jan 21, 2024

@fang-xing-esql I've made two more small suggestions. That second one about randomization should be fun to learn about. It's quite useful and the way we randomize our tests is fairly unique. Very not-quickcheck. Very low overhead at write and runtime but kind of annoying to reproduce. It works ok though!

costin
costin requested changes Jan 22, 2024
View reviewed changes
Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

Left a couple of comments.

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/expression/TypeResolutions.java Outdated Show resolved Hide resolved
...l/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/AutoBucket.java Outdated Show resolved Hide resolved
@fang-xing-esql
Copy link
Member Author

Left a couple of comments.

Thank you for the suggestions!

costin
costin approved these changes Jan 23, 2024
View reviewed changes
Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

LGTM

costin
costin reviewed Jan 23, 2024
View reviewed changes
Copy link
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

Forgot to mention - please add some basic csv tests, next to the existing auto-bucket ones.

@costin costin mentioned this pull request Jan 23, 2024
Open
bpintea
bpintea approved these changes Jan 23, 2024
View reviewed changes
Copy link
Contributor

@bpintea bpintea left a comment

Choose a reason for hiding this comment

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

LG, only left minor notes.

...l/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/AutoBucket.java Show resolved Hide resolved
...l/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/AutoBucket.java Outdated Show resolved Hide resolved
...l/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/AutoBucket.java Outdated Show resolved Hide resolved
.../test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/AutoBucketTests.java Outdated Show resolved Hide resolved
.../test/java/org/elasticsearch/xpack/esql/expression/function/scalar/math/AutoBucketTests.java Show resolved Hide resolved
@fang-xing-esql
Copy link
Member Author

Forgot to mention - please add some basic csv tests, next to the existing auto-bucket ones.

Added a new test case, and modified a few existing test cases in CsvTest to allow auto_bucket to take DateTime as input.

@fang-xing-esql
Copy link
Member Author

LG, only left minor notes.

Thank you for reviewing, all of the comments are addressed.

@fang-xing-esql fang-xing-esql mentioned this pull request Jan 23, 2024
Closed
@fang-xing-esql
Copy link
Member Author

@elasticmachine run buildkite/docs-build-pr

@fang-xing-esql fang-xing-esql removed the external-contributor Pull request authored by a developer outside the Elasticsearch team label Jan 25, 2024
@fang-xing-esql
Copy link
Member Author

@elasticmachine run buildkite/docs-build-pr

@brianseeders
Copy link
Contributor

run docs-build

@fang-xing-esql fang-xing-esql merged commit e0514f3 into elastic:main Jan 25, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@costin costin costin approved these changes

@bpintea bpintea bpintea approved these changes

@nik9000 nik9000 nik9000 approved these changes

Assignees
No one assigned
Labels
:Analytics/ES|QL AKA ESQL >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ES|QL] AUTO_BUCKET should accept date fields
6 participants
@fang-xing-esql @elasticsearchmachine @nik9000 @brianseeders @costin @bpintea