SQL: Adds MONTHNAME, DAYNAME and QUARTER functions by astefan · Pull Request #33411 · elastic/elasticsearch

astefan · 2018-09-05T08:33:40Z

This PR adds three functions from those listed here.

elasticmachine · 2018-09-05T08:33:41Z

Pinging @elastic/es-search-aggs

matriv

Nice work! Left some comments.

matriv · 2018-09-05T10:34:38Z

...ugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/FunctionRegistry.java

            def(MonthOfYear.class, MonthOfYear::new, "MONTH"),
            def(Year.class, Year::new),
            def(WeekOfYear.class, WeekOfYear::new, "WEEK"),
+            def(DayName.class, DayName::new, "DAYNAME"),


Why do we need alias here and next line?

ah, ignore this, Checked already. It's because DayName becomes day_name.

Should we make the class Dayname instead then? Do we want to support DAY_NAME as well as DAYNAME?

@nik9000 I chose it like this because of consistency. There are other date/time functions that have aliases, even though the spec mentions only one name. For example WEEK (WEEK_OF_YEAR), HOUR (HOUR_OF_DAY) etc.

matriv · 2018-09-05T11:04:24Z

.../java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeProcessor.java

-            throw new SqlIllegalArgumentException("A string or a date is required; received {}", l);
-        }
+    public Object doProcess(long millis) {
+        ReadableDateTime dt = new DateTime(millis, DateTimeZone.forTimeZone(timeZone()));


With the introduction of the abstract class, in case of 6.3- we already have a ReadableDateTime which we now convert to millis and then back to ReadableDateTime. Could this be a perf issue?

Since this is operating on the data returned by the search it isn't as much of a problem as it could be. And it'll only come up during the rolling upgrade too. Since we're using the opportunity to switch from Joda time classes to Java 8 time classes I'm on board with it.

matriv · 2018-09-05T11:08:48Z

x-pack/plugin/sql/src/main/resources/org/elasticsearch/xpack/sql/plugin/sql_whitelist.txt

 class org.elasticsearch.xpack.sql.expression.function.scalar.whitelist.InternalSqlScriptUtils {

  Integer dateTimeChrono(long, String, String)
+  String dayName(long,String)


Minor: add a whitespace after comma here and below.

matriv · 2018-09-05T11:24:50Z

...a/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/QuarterProcessorTests.java

+        assertEquals(1, proc.process("0"));
+        assertEquals(3, proc.process("-64164233612338"));
+        assertEquals(2, proc.process("64164233612338"));
+    }


Shouldn't we add nonUTC tests here as well?

matriv · 2018-09-05T13:20:13Z

As discussed, maybe consider to add some tests with the functions used in ORDER BY clause.

costin

Left some minor comments. Can you please add more integration tests with the functions in ORDER BY and HAVING?

costin · 2018-09-05T13:08:45Z

...va/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BaseDateTimeFunction.java

+
+import java.util.TimeZone;
+
+public abstract class BaseDateTimeFunction extends UnaryScalarFunction {


The class visibility could be package as the constructor.

costin · 2018-09-05T13:13:21Z

...a/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/NamedDateTimeFunction.java

+import static org.elasticsearch.xpack.sql.expression.function.scalar.script.ParamsBuilder.paramsBuilder;
+import static org.elasticsearch.xpack.sql.expression.function.scalar.script.ScriptTemplate.formatTemplate;
+
+public abstract class NamedDateTimeFunction extends BaseDateTimeFunction {


Restrict class visibility to be inline with that of the constructor.
Plus javadocs.

costin · 2018-09-05T15:05:00Z

...va/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/BaseDateTimeFunction.java

+        return timeZone;
+    }
+
+    // add tz along the rest of the params


I find this comment confusing, like a TODO. The intent is clear - the name has been overwritten, the method is needed.

matriv · 2018-09-10T21:11:57Z

...a/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/NamedDateTimeFunction.java

+import static org.elasticsearch.xpack.sql.expression.function.scalar.script.ScriptTemplate.formatTemplate;
+
+/*
+ * Base class for the two "naming" date/time functions: month_name and day_name


To make it more "future-proof" maybe:
Base class for "naming" data/time functions like: month_namd and day-name.

astefan · 2018-09-11T06:24:46Z

@matriv @costin I've incorporated your review in a follow up commit. Can you please, have another look?

…o 33092_fix

matriv

LGTM

* Added monthname, dayname and quarter functions * Updated docs tests with the new functions

astefan · 2018-09-11T12:09:41Z

6.x commit: 6832248

* master: Preserve cluster settings on full restart tests (elastic#33590) Use IndexWriter.getFlushingBytes() rather than tracking it ourselves (elastic#33582) Fix upgrading of list settings (elastic#33589) Add read-only Engine (elastic#33563) HLRC: Add ML get categories API (elastic#33465) SQL: Adds MONTHNAME, DAYNAME and QUARTER functions (elastic#33411) Add predicate_token_filter (elastic#33431) Fix Replace function. Adds more tests to all string functions. (elastic#33478) [ML] Rename input_fields to column_names in file structure (elastic#33568)

* master: (91 commits) Preserve cluster settings on full restart tests (elastic#33590) Use IndexWriter.getFlushingBytes() rather than tracking it ourselves (elastic#33582) Fix upgrading of list settings (elastic#33589) Add read-only Engine (elastic#33563) HLRC: Add ML get categories API (elastic#33465) SQL: Adds MONTHNAME, DAYNAME and QUARTER functions (elastic#33411) Add predicate_token_filter (elastic#33431) Fix Replace function. Adds more tests to all string functions. (elastic#33478) [ML] Rename input_fields to column_names in file structure (elastic#33568) Add full cluster restart base class (elastic#33577) Validate list values for settings (elastic#33503) Copy and validatie soft-deletes setting on resize (elastic#33517) Test: Fix package name SQL: Fix result column names for arithmetic functions (elastic#33500) Upgrade to latest Lucene snapshot (elastic#33505) Enable not wiping cluster settings after REST test (elastic#33575) MINOR: Remove Dead Code in SearchScript (elastic#33569) [Test] Remove duplicate method in TestShardRouting (elastic#32815) mute test on windows Update beats template to include apm-server metrics (elastic#33286) ...

astefan added >feature v7.0.0 :Analytics/SQL SQL querying v6.5.0 labels Sep 5, 2018

astefan requested review from costin, matriv and nik9000 September 5, 2018 08:33

matriv reviewed Sep 5, 2018

View reviewed changes

costin reviewed Sep 5, 2018

View reviewed changes

astefan added 5 commits September 10, 2018 15:20

Added monthname, dayname and quarter functions

3641b55

More tests, minor adjustements

52241ae

Another case where the marked bug reports needs a fix for

7e27176

Added monthname, dayname and quarter functions

3dbfaa1

Updated docs tests with the new functions

6df006a

astefan force-pushed the 33092_fix branch from 17c6330 to 6df006a Compare September 10, 2018 16:13

astefan added 2 commits September 10, 2018 19:21

More tests, minor adjustements

0ec7108

Another case where the marked bug reports needs a fix for

53cf547

matriv reviewed Sep 10, 2018

View reviewed changes

astefan added 2 commits September 11, 2018 09:36

Minor javadoc adjustement

8395375

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

010e40c

…o 33092_fix

matriv approved these changes Sep 11, 2018

View reviewed changes

astefan merged commit a3e1f1e into elastic:master Sep 11, 2018

astefan added a commit to astefan/elasticsearch that referenced this pull request Sep 11, 2018

SQL: Adds MONTHNAME, DAYNAME and QUARTER functions (elastic#33411)

6832248

* Added monthname, dayname and quarter functions * Updated docs tests with the new functions

astefan deleted the 33092_fix branch September 11, 2018 12:10

vladimirdolzhenko mentioned this pull request Sep 12, 2018

[CI] NamedDateTimeProcessorTests fails #33621

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019


		import java.util.TimeZone;

		public abstract class BaseDateTimeFunction extends UnaryScalarFunction {

Conversation

astefan commented Sep 5, 2018

Uh oh!

elasticmachine commented Sep 5, 2018

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv commented Sep 5, 2018

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan commented Sep 11, 2018

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

astefan commented Sep 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Comments