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: Fix ORDER BY YEAR() function #51562

Merged
merged 7 commits into from
Jan 30, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -39,17 +39,15 @@ protected ScalarFunction(Source source, List<Expression> fields) {
super(source, fields);
}

// used if the function is monotonic and thus does not have to be computed for ordering purposes
// null means the script needs to be used; expression means the field/expression to be used instead
// Every function needs to be translated to a script
// which will be used for the ordering
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 the method can be removed entirely - is it used anywhere else?

public Expression orderBy() {
return null;
}


//
// Script generation
//

public ScriptTemplate asScript(Expression exp) {
if (exp.foldable()) {
return scriptWithFoldable(exp);
Expand All @@ -73,7 +71,6 @@ public ScriptTemplate asScript(Expression exp) {
throw new QlIllegalArgumentException("Cannot evaluate script for expression {}", exp);
}


protected ScriptTemplate scriptWithFoldable(Expression foldable) {
Object fold = foldable.fold();

Expand Down Expand Up @@ -144,4 +141,4 @@ protected String processScript(String script) {
protected String formatTemplate(String template) {
return Scripts.formatTemplate(template);
}
}
}
6 changes: 6 additions & 0 deletions x-pack/plugin/sql/qa/src/main/resources/datetime.sql-spec
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,12 @@ SELECT first_name FROM test_emp ORDER BY NOW(), first_name NULLS LAST LIMIT 5;
groupByCurrentTimestamp
SELECT MAX(salary) AS max FROM test_emp GROUP BY NOW();

//
// ORDER BY
//
orderByYear
SELECT YEAR(birth_date) as year, emp_no FROM test_emp ORDER BY year NULLS FIRST, emp_no ASC;

// ES will consider a TIME in UTC, if no other indication is given and H2 doesn't consider the timezone for times, so no TZSync'ing needed.
hourFromStringTime
SELECT HOUR(CAST('18:09:03' AS TIME)) AS result;
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/sql/qa/src/main/resources/docs/docs.csv-spec
Original file line number Diff line number Diff line change
Expand Up @@ -939,7 +939,7 @@ SELECT -2 * INTERVAL '3' YEARS AS result;

///////////////////////////////
//
// Order by
// Order By
//
///////////////////////////////

Expand Down
20 changes: 10 additions & 10 deletions x-pack/plugin/sql/qa/src/main/resources/nested.csv-spec
Original file line number Diff line number Diff line change
Expand Up @@ -106,23 +106,23 @@ SELECT first_name f, last_name l, YEAR(dep.from_date) start FROM test_emp WHERE

f:s | l:s | start:i

Sreekrishna |Servieres |1985
Zhongwei |Rosen |1986
Chirstian |Koblick |1986
null |Chappelet |1988
Zvonko |Nyanchama |1989
Parto |Bamford |1995
Copy link
Contributor

Choose a reason for hiding this comment

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

Ordering here is completely wrong. 1995, 1986, 1996.... The query has ORDER BY start where start is YEAR(dep.from_date).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooops! I rushed here, sorry, need to check it. Thx for catching!

Chirstian |Koblick |1986
Duangkaew |Piveteau |1996
Kazuhide |Peha |1992
null |Demeyer |1994
;

selectWithScalarOnNestedWithoutProjection
SELECT first_name f, last_name l FROM test_emp WHERE dep.dep_name = 'Production' AND languages > 1 ORDER BY YEAR(dep.from_date) LIMIT 5;

f:s | l:s

Sreekrishna |Servieres
Zhongwei |Rosen
Chirstian |Koblick
null |Chappelet
Zvonko |Nyanchama
Parto |Bamford
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is not relevant (anymore). Can you add dep.from_date to the projections?
Why did the order change since the PR should address ONLY the case of ordering by year and something else?

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 order can change because previously we would have a strict order on the whole from_date (including day and month) but now simply on the YEAR, so (apart from the YEAR) can be affected by the order of the doc entries when inserted to the index.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so how does the query dsl look like now? Previously I think it contained a "sort" : [{"from_date" : {"order" : "asc"}}] section. We don't have tests that check the generated ES query with a sort by a date field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, since a scalar is used, it needs to be translated into a scripted sorting, which currently is not supported for nested fields.

Chirstian |Koblick
Duangkaew |Piveteau
Kazuhide |Peha
null |Demeyer
;

//
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,6 @@ public String dateTimeFormat() {
return "year";
}

@Override
public Expression orderBy() {
return field();
}

@Override
public String calendarInterval() {
return YEAR_INTERVAL;
Expand Down