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

New DayName SQL function, optional timezone & locale sql and painless… #30453

Closed
wants to merge 1 commit into from
Closed

New DayName SQL function, optional timezone & locale sql and painless… #30453

wants to merge 1 commit into from

Conversation

mP1
Copy link

@mP1 mP1 commented May 8, 2018

… date getters #29981

  • introduced new DayName sql function
  • ALL date sql functions now support optional timezone.
  • DayName includes optional tz + optional locale.
  • all painless date getters are overloaded to match supported param lists of
    sql functions also take optional tz + dayname optional locale
  • introduced a pass a "context" to all date functions which hold
    • "default tz"
    • "default" locale
  • introduced new sub class of ReadableDateTime called PainlessDateTime,
    supporting overloads for all date getters (optional tz etc).
  • Fixed Definition when processing whitelist to allow covariant return types when checking
    class vs implementing interface etc.
  • SQL: Support for DATENAME function or eqv #29981 (Implement sql DayName fn)
  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS that we support?
  • If you are submitting this code for a class then read our policy for that.

yes to all, except classwork.

@mP1
Copy link
Author

mP1 commented May 8, 2018

Sample from kibana showing

  • ALL sql date functions without tz, and with tz.
  • DayName in 3 forms
    -- without tz
    -- with tz
    -- with tz and locale
    image

@mP1
Copy link
Author

mP1 commented May 8, 2018

painless getters for all date "getters" now also allow an optional tz, and for dayName optional tz, and optional locale.

This means no longer need all that boilerplate formerly shown as samples for sql date functions to "use" a timezone other than the systems.

Since the tz and locale are now parameters to the getters the user can pick their own tz and locale as they please.

image

@mP1
Copy link
Author

mP1 commented May 8, 2018

I had to introduce a new sub classes of ReadableDateTime in Dates so i could add the a dayName getter and add overloads to support "getting" these date values with/without tz/locale etc.

The class is called PainlessDateTime.

@@ -1011,7 +1011,8 @@ private void addMethod(ClassLoader whitelistClassLoader, String ownerStructName,
"and parameters " + whitelistMethod.painlessParameterTypeNames, iae);
}

if (javaMethod.getReturnType() != defClassToObjectClass(painlessReturnClass)) {
// allow convariant return types in implementing classes.
if (!defClassToObjectClass(painlessReturnClass).isAssignableFrom(javaMethod.getReturnType())) {
Copy link
Author

Choose a reason for hiding this comment

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

Had to fix this because i had to introduce a new sub class of ReadableDateTime and i wanted to change the return types from RDT to my new class.

org.joda.time.ReadableDateTime getDate()
List getDates()
}

Copy link
Author

Choose a reason for hiding this comment

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

the new sub class of RDT, now whitelisted and with overloads to take tz, and locale (for dayname)

@@ -95,7 +95,7 @@ public void testDates() throws IOException {
assertEquals(dates[d][i], longs.getDates().get(i));
}

Copy link
Author

Choose a reason for hiding this comment

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

this test only wants to verify the list is frozen, doesnt care about the "value".

this.functionRegistry = functionRegistry;
this.indexResolution = results;
this.timeZone = timeZone;
this.context = context;
Copy link
Author

Choose a reason for hiding this comment

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

Previously a bunch of classes passed a "default" tz. To make dayName with locale work it became necessary to support a "default" locale, so i replaced tz with a context that holds both "defaults".

def(SecondOfMinute.class, SecondOfMinute::new, "SECOND"),
def(MonthOfYear.class, MonthOfYear::new, "MONTH"),
def(Year.class, Year::new),
def(WeekOfYear.class, WeekOfYear::new, "WEEK"),
Copy link
Author

Choose a reason for hiding this comment

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

because all sql date functions now take more than one parameter, had to change definition to match this new detail.

def(MonthOfYear.class, (Location location, List<Expression> field, FunctionContext context) -> new MonthOfYear(location, field, context), "MONTH"),
def(Year.class, (Location location, List<Expression> field, FunctionContext context) -> new Year(location, field, context)),
def(WeekOfYear.class, (Location location, List<Expression> field, FunctionContext context) -> new WeekOfYear(location, field, context), "WEEK"),
def(DayName.class, (Location location, List<Expression> field, FunctionContext context) -> new DayName(location, field, context), "DAYNAME"),
Copy link
Author

Choose a reason for hiding this comment

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

DayName - the new sql function, that started this work


public abstract class DateTimeFunction extends UnaryScalarFunction {
Copy link
Author

Choose a reason for hiding this comment

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

DateTimeFunction is no longer a unary scalar function, it is now var args...

1= date
2=optional string holding timezoneid
3=optional string holding locale.

Note the right arg count is enforced based on the "type" of sql function in the ctor

}

@Override
protected ScriptTemplate asScriptFrom(FieldAttribute field) {
ParamsBuilder params = paramsBuilder();
public final Object fold() {
Copy link
Author

Choose a reason for hiding this comment

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

big improvement now users can call date.getXXX with optional timezone and locale where applicable, instead of horrible boilerplate from before.

// no overload involving timezone
int maxArgumentCount() {
return 1;
}
Copy link
Author

Choose a reason for hiding this comment

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

doesnt make any ssense to allow second of minute function to specify a tz.

@@ -6,43 +6,38 @@
package org.elasticsearch.xpack.sql.expression.function.scalar.datetime;
Copy link
Author

Choose a reason for hiding this comment

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

briefly all other sql date functions are no var args, changes were made because no longer inherit from unaryscalarfunction. Nothing else really changed.

import java.util.Locale;
import java.util.TimeZone;

public abstract class DateTimeFunctionTestcase<F extends DateTimeFunction> extends ESTestCase {
Copy link
Author

Choose a reason for hiding this comment

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

nice test case for datetime sql functions to make each test a one liner without any boilerplate.

}

public void testApply() {
Copy link
Author

Choose a reason for hiding this comment

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

Extracted these unit tests for different sql functions into separate test classes. Putting thigns in proper place.

… date getters #29981

- introduced new DayName sql function
- ALL date sql functions now support optional timezone.
- DayName includes optional tz + optional locale.
- all painless date getters are overloaded to match supported param lists of
   sql functions also take optional tz + dayname optional locale
- introduced a pass a "context" to all date functions which hold
  - "default tz"
  - "default" locale
- introduced new sub class of ReadableDateTime called PainlessDateTime,
  supporting overloads for all date getters (optional tz etc).
- Fixed Definition when processing whitelist to allow covariant return types when checking
  class vs implementing interface etc.
- #29981 (Implement sql DayName fn)
@mP1
Copy link
Author

mP1 commented May 8, 2018

to play in kibana

PUT /library/book/_bulk?refresh
{"index":{"_id": "Leviathan Wakes"}}
{"name": "Leviathan Wakes", "author": "James S.A. Corey", "release_date": "2011-06-02", "page_count": 561}
{"index":{"_id": "Hyperion"}}
{"name": "Hyperion", "author": "Dan Simmons", "release_date": "1989-05-26", "page_count": 482}
{"index":{"_id": "Dune"}}
{"name": "Dune", "author": "Frank Herbert", "release_date": "1965-06-01", "page_count": 604}

POST /_xpack/sql?format=txt
{
    "query": "SELECT * FROM library WHERE release_date < '2000-01-01'"
}

POST /_xpack/sql?format=txt
{
    "query": "SELECT                                                                                      DAY_OF_MONTH(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP)) AS dayOfMonth,                                    DAY_OF_MONTH(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP), 'AET') AS dayOfMonthAET,                           DAY_OF_WEEK(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP)) AS dayOfWeek,                                       DAY_OF_WEEK(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP), 'AET') AS dayOfWeekAET,                            DAY_OF_YEAR(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP)) AS dayOfYear,                                      DAY_OF_YEAR(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP), 'AET') AS dayOfYearAET,                            HOUR_OF_DAY(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP)) AS hourOfDay,                                        HOUR_OF_DAY(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP), 'AET') AS hourOfDayAET,                                MINUTE_OF_DAY(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP)) AS minuteOfDay,                                  MINUTE_OF_DAY(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP), 'AET') AS minuteOfDayAET,                        MINUTE_OF_HOUR(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP)) AS minuteOfHour,                                   MINUTE_OF_HOUR(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP), 'AET') AS minuteOfHourAET,                         MONTH_OF_YEAR(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP)) AS monthOfYear,                                   MONTH_OF_YEAR(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP), 'AET') AS monthOfYearAET,                           SECOND_OF_MINUTE(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP)) AS secondOfMinute,                                WEEK_OF_YEAR(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP)) AS weekOfYear,                                     WEEK_OF_YEAR(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP), 'AET') AS weekOfYearAET,                                  YEAR(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP)) AS year,                                                        YEAR(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP), 'AET') AS yearAET,                                         DAY_NAME(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP)) AS dayName,                                             DAY_NAME(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP), 'AET') AS dayNameAET,                                   DAY_NAME(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP), 'AET', 'FR') AS dayNameAETFr"
}

POST /_xpack/sql?format=txt
{
    "query": "SELECT DAYNAME(CAST('2018-05-03T10:23:27Z' AS TIMESTAMP)) AS dayname"
}

POST /_xpack/sql?format=txt
{
    "query": "SELECT EXTRACT(DAY_OF_YEAR FROM CAST('2018-02-19T10:23:27Z' AS TIMESTAMP)) AS day"
}

POST /_xpack/sql?format=txt
{
    "query": "SELECT EXTRACT(DAYNAME FROM CAST('2018-02-19T10:23:27Z' AS TIMESTAMP)) AS day"
}

POST /_xpack/sql?format=txt
{
    "query": "SELECT DAY_OF_YEAR(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP)) AS day"
}

POST /_xpack/sql?format=txt
{
    "query": "SELECT HOUR_OF_DAY(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP), 'AET') AS day"
}

POST /_xpack/sql?format=txt
{
    "query": "SELECT DAYNAME(CAST('2018-02-19T10:23:27Z' AS TIMESTAMP), 'AET', 'FR') AS day"
}

GET library/_search

GET library/_search
{
  "query": {
    "match_all": {}
  },
  "script_fields": {
    "total_goals": {
      "script": {
        "lang": "painless",
        "source": """
          return doc["release_date"].value.getDayName("AET", "en_au") + "," +
                 doc["release_date"].value.getDayName("AET", "fr_fr") + "," +
                 doc["release_date"].value.getDayOfMonth("AET") 
        """
      }
    }
  }
}

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

1 similar comment
@mP1
Copy link
Author

mP1 commented May 22, 2018

Pinging @elastic/es-search-aggs

@costin
Copy link
Member

costin commented Oct 30, 2018

Thank you for the contribution @mP1.
As there hasn't been any update on this PR, I'll close it.

Cheers,

@costin costin closed this Oct 30, 2018
@mP1
Copy link
Author

mP1 commented Nov 12, 2018

Im sorry I cant afford to spend more time on more pull requests for any Elastic product, when the vast majority of them are closed (like this one) or ignored for months on end. There are over a thousand tickets, many of which have many community members watching them, and many open for years and months, and large numbers remain ignored.

If Elastic is not interested in contributions or does not have the resources, it should be honest and make a public statement on the main project page, so people dont waste time trying to provide enhancements, fixes and so on to the project.

If you want to pay me to work on this and other items feel free to contact me, but I wont be putting in wasted effort for my work or others to be ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants