SQL: Introduce HISTOGRAM grouping function #36510

costin · 2018-12-11T21:44:28Z

Introduce Histogram grouping function for bucketing/grouping data based
on a given range. Both date and numeric histograms are supported using
the appropriate range declaration (numbers vs intervals).

SELECT HISTOGRAM(number, 50) AS h FROM index GROUP BY h
SELECT HISTOGRAM(date, INTERVAL 1 YEAR) AS h FROM index GROUP BY h

In addition add multiply operator for Intervals
Add docs for intervals and histogram

Fix #36509

Note: Histograms don't work as arguments to functions nor inside Having yet. That's planned as a separate PR.

elasticmachine · 2018-12-11T21:44:29Z

Pinging @elastic/es-search

Introduce Histogram grouping function for bucketing/grouping data based on a given range. Both date and numeric histograms are supported using the appropriate range declaration (numbers vs intervals). SELECT HISTOGRAM(number, 50) AS h FROM index GROUP BY h SELECT HISTOGRAM(date, INTERVAL 1 YEAR) AS h FROM index GROUP BY h In addition add multiply operator for Intervals Add docs for intervals and histogram Fix elastic#36509

astefan

Left few cosmetic comments. Still review-in-progress.

docs/reference/sql/concepts.asciidoc

docs/reference/sql/functions/date-time.asciidoc

docs/reference/sql/functions/grouping.asciidoc

matriv

Nice work! Left a first batch of comments.

matriv · 2018-12-12T09:40:17Z

docs/reference/sql/functions/date-time.asciidoc

@@ -1,7 +1,99 @@
 [role="xpack"]
 [testenv="basic"]
 [[sql-functions-datetime]]
-=== Date and Time Functions
+=== Date/Time and Interval Functions and Operators
+


please add the beta[] marker

matriv · 2018-12-12T09:41:24Z

docs/reference/sql/functions/date-time.asciidoc

+A common requirement when dealing with date/time in general is date/time math which resolves around 
+the notion of ``interval``s, a topic that is worth exploring in the context of {es} and {es-sql}.
+
+{es} has comprehensive support for <<date-math, date math>> both inside <<date-math-index-names,index names>> and <<mapping-date-format, queries>>.


add a space after comma , index names

matriv · 2018-12-12T10:50:52Z

docs/reference/sql/functions/grouping.asciidoc

+<2> numeric interval
+<3> date/time expression (typically a field)
+<4> date/time <<sql-functions-datetime-interval, interval>>
+...


why do you need the ... here?

matriv · 2018-12-12T10:52:14Z

docs/reference/sql/functions/grouping.asciidoc

+include-tagged::{sql-specs}/docs.csv-spec[histogramNumeric]
+----
+
+and date/time fields:


I would say or instead of and since it continues from: either

matriv · 2018-12-12T11:03:16Z

...a/single-node/src/test/java/org/elasticsearch/xpack/sql/qa/single_node/JdbcDocCsvSpecIT.java

@@ -68,8 +68,8 @@ protected void assertResults(ResultSet expected, ResultSet elastic) throws SQLEx
        //
        // uncomment this to printout the result set and create new CSV tests
        //
-        //JdbcTestUtils.logLikeCLI(elastic, log);
-        JdbcAssert.assertResultSets(expected, elastic, log, true);
+        JdbcTestUtils.logLikeCLI(elastic, log);


revert this change?

matriv · 2018-12-12T11:09:25Z

x-pack/plugin/sql/qa/src/main/resources/datetime-interval.csv-spec

 SELECT INTERVAL '163:59.163' MINUTE TO SECOND AS interval;

 interval
 ---------------
 +0 02:43:59.163  
 ;

-testDatePlusInterval
+intervalPlusInterval


Maybe add Local suffix to all of them since further below there are tests on table columns.

Maybe we should remove them from here as they are also tested in the docs csv spec.

I'd like to keep things separate - while there is some duplication the doc lifecycle is different from that of the test. In fact, docs were added to avoid having obscure queries that make sense for testing show up in our documentation.

matriv · 2018-12-12T11:11:06Z

x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec

+//
+
+
+histogramNumeric


I think we can remove these 2 tests since they are also in the docs csv spec.

matriv · 2018-12-12T11:16:51Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java

@@ -108,7 +108,11 @@ public Analyzer(Configuration configuration, FunctionRegistry functionRegistry,
                new ResolveAggsInHaving()
                //new ImplicitCasting()
                );
-        return Arrays.asList(substitution, resolution);
+        Batch finish = new Batch("Finish Analysis",
+                new PruneSubqueryAliases(),


Why don't we merge them in one rule?

Because they have different concerns - I would expect the second one to change when we add full support for subselects.

matriv · 2018-12-12T11:17:34Z

x-pack/plugin/sql/qa/src/main/resources/math.csv-spec

-30404          |58715          |1993           |3.0                      |3.0                         |3              
-35742          |67492          |1994           |2.8                      |2.7                         |4              
-45656          |45656          |1996           |3.0                      |3.0                         |1           
+25324          |70011          |1986           |3.0                      |3.0                         |15             


where is this -1 year change come from?

Relative vs absolute time computing. ES allows both absolute offsets (24h) or relative (1d) which takes into account if the date/time is within a day.
Based on the intervals given, ES keeps going back and forth but that's surprising and unexpected - in SQL as the intervals vary, I've used the absolute one so that INTERVAL 1 YEAR and INTERVAL 12 MONTHS, etc... gives you the same results.

matriv · 2018-12-12T11:19:32Z

docs/reference/sql/functions/date-time.asciidoc

+| `INTERVAL '3 4' DAYS TO HOURS`                | 3 days and 4 hours
+| `INTERVAL '5 6:12' DAYS TO MINUTES`           | 5 days, 6 hours and 12 minutes
+| `INTERVAL '3 4:56:01' DAY TO SECOND`          | 3 days, 4 hours, 56 minutes and 1 second
+| `INTERVAL '2 3:45:01.23456789' DAY TO SECOND` | 2 days, 3 hours, 45 minutes, 1 second and 234567890 milliseconds


this should be nanoseconds, also below.

matriv

I'm through with the review, nice work overall!

Maybe we could add an integ test for interval multiplication on table columns.
What happens if the histogram is used in the WHERE clause? should we catch this case?

matriv · 2018-12-12T11:35:53Z

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

@@ -527,4 +552,4 @@ private static FunctionDefinition def(Class<? extends Function> function, Functi
    private interface CastFunctionBuilder<T> {
        T build(Location location, Expression expression, DataType dataType);
    }
-}
+        }


please revert.

Fixed. git merging.

matriv · 2018-12-12T11:36:55Z

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

+     * operates on a datetime.
+     */
+    @SuppressWarnings("overloads") // These are ambiguous if you aren't using ctor references but we always do
+    static <T extends Function> FunctionDefinition def(Class<T> function, DatetimeBinaryFunctionBuilder<T> ctorRef, String... names) {


I don't get why do we need a special overload.

Because no other def class matches - the constructor signature of Histogram is different then everything else - it's the only function with 2 Expression arguments and a TimeZone.

hm, thx! I missed something there, sorry.

matriv · 2018-12-12T11:37:11Z

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

@@ -415,6 +418,28 @@ public boolean functionExists(String functionName) {
        T build(Location location, Expression target, TimeZone tz);
    }

+    /**
+     * Build a {@linkplain FunctionDefinition} for a plur/n-ary function that


do you mean binary?

Initially I wanted to go for multi/variable arguments but that's not the case. Will update it.

matriv · 2018-12-12T11:39:32Z

...src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/GroupingFunction.java

+
+    @Override
+    public ScriptTemplate asScript() {
+        throw new SqlIllegalArgumentException("Grouping functions cannot be scripted");


Not important for now. I think we could introduce an SqlIllegalOperationException for such cases.

That implies an operation that somehow was accepted but it's not implemented, which I would consider a bug.

But this is somehow the case here, no? We end up calling this method while we shouldn't. The SqlIllegalArgumentException marks calling something (e.g.: sql function from the user perspective, or internal method with wrong artuments). Anyway, this is out of this PR's scope.

matriv · 2018-12-12T11:41:34Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java

-}
+
+    public void testGroupByDateHistogram() {
+


missing test body ;)

matriv · 2018-12-12T11:41:43Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java

+    }
+
+    public void testGroupByHistogram() {
+


matriv · 2018-12-12T12:47:21Z

...ck/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlClearCursorAction.java

@@ -27,7 +27,7 @@

    private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(RestSqlClearCursorAction.class));

-    RestSqlClearCursorAction(Settings settings, RestController controller) {
+    public RestSqlClearCursorAction(Settings settings, RestController controller) {


hm, this is needed for our debug project only, should we revert?

No, this change shouldn't have occurred in the first place but the PR got it before I got a chance to review it.

matriv · 2018-12-12T12:47:30Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java

@@ -38,7 +38,7 @@

    private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(RestSqlQueryAction.class));

-    RestSqlQueryAction(Settings settings, RestController controller) {
+    public RestSqlQueryAction(Settings settings, RestController controller) {


matriv · 2018-12-12T12:53:00Z

.../src/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Mul.java

+            return TypeResolution.TYPE_RESOLVED;
+        }
+
+        return new TypeResolution(format("[{}] has arguments with incompatible types [{}] and [{}]", symbol(), l, r));


Could you add a test for this?

Done - I added one but forgot about it so I created a dedicated test class (at which point I remembered about the previous test).

matriv · 2018-12-12T12:56:22Z

...sticsearch/xpack/sql/expression/predicate/operator/arithmetic/BinaryArithmeticProcessor.java

+            if (l instanceof Number && r instanceof Number) {
+                return Arithmetics.mul((Number) l, (Number) r);
+            }
+            l = unwrapJodaTime(l);


I would add some unit tests for the processor for the intervals.

See BinaryArithmeticTests

astefan · 2018-12-13T07:47:39Z

x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec

+
+histogramDateWithCountAndOrder
+schema::h:ts|c:l
+SELECT HISTOGRAM(birth_date, INTERVAL 1 YEAR) AS h, COUNT(*) as c FROM test_emp GROUP BY h ORDER BY h DESC;


Both tests here seem to return a "continuous" list of years... what happens for an interval where there is no document? For example, if we should have had a 1981 birth_date, would the years between 1964 and 1980 be displayed with count 0 or not displayed at all?

No bucket is returned (as that would break the notion of null group in SQL).

Can you mention this in the documentation, please? Users used to date_histogram (where the "missing" buckets are still returned by ES) will probably expect the same from HISTOGRAM.

astefan · 2018-12-13T07:52:22Z

x-pack/plugin/sql/qa/src/main/resources/datetime-interval.csv-spec

+1 00:53:00.0    
+;
+
+datePlusIntervalInline


Other tests that don't process tables data have constant in their name. Should we keep the same trend here? For example constantDatePlusInterval.

In this csv, constant is not used since for intervals that means repeating for every test so it becomes noise. Considering it has its own namespace (dedicated file) I think it's fine as is.

astefan · 2018-12-13T07:58:26Z

...java/org/elasticsearch/xpack/sql/expression/function/grouping/GroupingFunctionAttribute.java

+
+public class GroupingFunctionAttribute extends FunctionAttribute {
+
+    private final String propertyPath;


Can you add a small comment about the role of propertyPath?

I've removed it

astefan · 2018-12-13T08:08:14Z

...in/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/grouping/Histogram.java

+    }
+
+    @Override
+    public Expression replaceChildren(List<Expression> newChildren) {


Why the replaceChildren(List<Expression>) here and not defined as final in GroupingFunction enforcing the replacement of a single children? GroupingFunction would then define a protected abstract GroupingFunction replaceChildren(Expression newExpression);. Are we expecting more grouping functions with variable list of children in the future?

We don't know what the future will bring - the pattern above is used for Unary[..]Functions which a grouping function might be, but we don't know yet.
Adding it now results in 2 methods without any reuse so it's arguable whether it makes sense to do it or not. In general, I prefer to add a pattern when it applies to at least 2 instances, otherwise it's more of a premature recognition.
I've added it nevertheless.

astefan · 2018-12-13T08:16:38Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/ExpressionBuilder.java

+        boolean negative = false;
+
+        ParserRuleContext parentCtx = interval.getParent();
+        if (parentCtx != null) {


What is this nested if block doing?

Because of the structure of the grammar it's a way to get the - symbol from the immediate parent context in the hierarchy that could possibly contain it.

astefan · 2018-12-13T08:27:10Z

...ugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/agg/GroupByNumericHistogram.java

+    public boolean equals(Object obj) {
+        if (super.equals(obj)) {
+            GroupByNumericHistogram other = (GroupByNumericHistogram) obj;
+            return Objects.equals(interval, other.interval);


Why Objects.equals and not interval == other.interval?

astefan · 2018-12-13T08:31:19Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java

-}
+
+    public void testGroupByDateHistogram() {
+        LogicalPlan p = plan("SELECT MAX(int) FROM test GROUP BY HISTOGRAM(int, 1000)");


Do we have a situation where the histogram is being grouped by very small intervals? Like 1 second for a date interval.

I tried a test with 1 second and it worked :). Our data might not be representative but I think it's a side effect of composite aggs which paginates through data instead of creating all the buckets up-front.

matriv

LGTM! Great work!

astefan

Great stuff! LGTM

…-histograms

Introduce Histogram grouping function for bucketing/grouping data based on a given range. Both date and numeric histograms are supported using the appropriate range declaration (numbers vs intervals). SELECT HISTOGRAM(number, 50) AS h FROM index GROUP BY h SELECT HISTOGRAM(date, INTERVAL 1 YEAR) AS h FROM index GROUP BY h In addition add multiply operator for Intervals Add docs for intervals and histogram Fix #36509 (cherry picked from commit 6ee6bb5)

costin added >feature v7.0.0 :Analytics/SQL SQL querying v6.6.0 labels Dec 11, 2018

costin requested review from astefan and matriv December 11, 2018 21:44

costin force-pushed the introduce-histograms branch 2 times, most recently from 318b716 to 83d76b4 Compare December 11, 2018 23:17

costin force-pushed the introduce-histograms branch from 83d76b4 to 0c1c6d8 Compare December 12, 2018 09:20

astefan reviewed Dec 12, 2018

View reviewed changes

matriv reviewed Dec 12, 2018

View reviewed changes

Address feedback

bc09b38

astefan reviewed Dec 13, 2018

View reviewed changes

costin added 2 commits December 14, 2018 12:27

More feedback

6603ce9

Merge branch 'master' into introduce-histograms

b1d5e62

matriv approved these changes Dec 14, 2018

View reviewed changes

astefan approved these changes Dec 14, 2018

View reviewed changes

Merge remote-tracking branch 'remotes/upstream/master' into introduce…

9256ddf

…-histograms

costin merged commit 6ee6bb5 into elastic:master Dec 14, 2018

costin deleted the introduce-histograms branch December 14, 2018 16:20

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


		public class GroupingFunctionAttribute extends FunctionAttribute {

		private final String propertyPath;

		//


		histogramNumeric

SQL: Introduce HISTOGRAM grouping function #36510

SQL: Introduce HISTOGRAM grouping function #36510

Conversation

costin commented Dec 11, 2018 • edited

elasticmachine commented Dec 11, 2018

astefan left a comment

Choose a reason for hiding this comment

matriv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv Dec 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv left a comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

costin commented Dec 11, 2018 •

edited

matriv Dec 13, 2018 •

edited