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.

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

SQL: non ISO 8601 versions of DAY_OF_WEEK and WEEK_OF_YEAR functions #36358

Merged
merged 12 commits into from Dec 12, 2018

Conversation

astefan
Copy link
Contributor

@astefan astefan commented Dec 7, 2018

Renamed DAY_OF_WEEK and WEEK_OF_YEAR functions to their ISO version and added the same functions with different functionality. Rewritten the datetime functions documentation to follow the format of the other functions documentation pages.

Fixes #36263

added the same functions with different functionality. Rewritten the
datetime functions documentation to follow the format of the other
functions documentation pages.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@astefan astefan requested review from costin and matriv December 7, 2018 11:22
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 but I would wait for @matriv's PR that fixes the naming convention inside FunctionRegistry.

@@ -40,6 +40,12 @@ DOW |SCALAR
DOY |SCALAR
HOUR |SCALAR
HOUR_OF_DAY |SCALAR
ISODAYOFWEEK |SCALAR
Copy link
Member

Choose a reason for hiding this comment

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

I think we also need to add these shortcuts:
isodayofweek/isodow ->idow
isoweek -> iw
isoweekofyear -> iwoy

@astefan
Copy link
Contributor Author

astefan commented Dec 10, 2018

run the gradle build tests 1

int dayOfWeek = zdt.get(ChronoField.DAY_OF_WEEK) + 1;
return dayOfWeek == 8 ? 1 : dayOfWeek;
}),
WEEK_OF_YEAR(zdt -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add here also the difference between iso and non-iso?

@@ -98,8 +98,10 @@ class org.elasticsearch.xpack.sql.expression.function.scalar.whitelist.InternalS
#
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems we are missing the existing non-iso methods here, and relevant tests that would catch those cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The iso methods are handled through the dateTimeChrono method which also handles the rest of the date time functions. I've renamed the DateTimeExtractor enum values for ISO day_of_week and week_of_year functions, hopefully making this less confusing.


.Description:

Extract fields from datetimes by specifying the name of a <<sql-functions-datetime,datetime function>>.
Copy link
Contributor

@matriv matriv Dec 10, 2018

Choose a reason for hiding this comment

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

I would rephrase: ... from a datetime field or so, I don't like the datetimes plural so much :-)
It's also inconsistent since you use from a datetime elsewhere.

Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

Looks good! Left some comments, and will wait for the missing methods in painless.

@costin
Copy link
Member

costin commented Dec 10, 2018

Relates: #36417

Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

LGTM! You need to adjust the function registration on top of the change with mandatory primary name.

@astefan
Copy link
Contributor Author

astefan commented Dec 11, 2018

@elasticmachine run default distro tests

@astefan astefan merged commit de37306 into elastic:master Dec 12, 2018
astefan added a commit that referenced this pull request Dec 12, 2018
…36358)

* Renamed DAY_OF_WEEK and WEEK_OF_YEAR functions to their ISO version and
added the same functions with different functionality.
* Rewritten the datetime functions documentation to follow the format of the other
functions documentation pages.

(cherry picked from commit de37306)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants