dateTime specified as sortable field in sorting clause #170

dayamr · 2017-02-15T00:35:01Z

This PR is first one among the 'dateTime' based sort feature.

cdeszaq

Changelog entry needed.

Initial quick-pass review complete. I didn't dig through it in great detail, but left a comments on the things I saw with a quick scan.

cdeszaq · 2017-02-15T19:47:03Z

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/orderby/OrderByColumn.java

@@ -51,6 +51,17 @@ public OrderByColumn(PostAggregation postAggregation, SortDirection direction) {
    }

    /**
+     * Constructor.
+     *
+     * @param dateTime  a dateTime


Since this constructor is quite different from the rest, and it takes very generic parameters, it should get more description as to why it exists and what it's intended to be used for. Additionally, the comment for this parameter is not great... if I give it, for example, 2017-02-01T10:25, something tells me it wouldn't do the right thing.

If we could have a way to create this that didn't require the user to specify the column name, that would be great. A static factory method that takes a direction, for example.

cdeszaq · 2017-02-15T19:47:46Z

fili-core/src/main/java/com/yahoo/bard/webservice/web/DataApiRequest.java

+        if (
+                sorts != null &&
+                        !sorts.isEmpty() &&
+                        sorts.toLowerCase(Locale.ENGLISH).contains(DATE_TIME_STRING.toLowerCase(Locale.ENGLISH))


Can we simplify this conditional, or isolate it's complexity into a helper method?

Also, why are we lower-casing this keyword? Do we do that anywhere else in the API for keywords?

Also also, why use contains, why not use startsWith?

if we use contains, we can educate user with proper error message. If we use starts with, error message would be 'dateTime is not metric......'. It may restrict the user to think sort is allowed only for metrics.

cdeszaq · 2017-02-15T19:49:27Z

fili-core/src/main/java/com/yahoo/bard/webservice/web/DataApiRequest.java

@@ -114,6 +116,8 @@

    private final DruidFilterBuilder filterBuilder;

+    private Optional<OrderByColumn> dateTimeSort = Optional.ofNullable(null);


Rather than defaulting to a null Optional, we should use Optional.emtpy()

cdeszaq · 2017-02-15T19:51:51Z

fili-core/src/main/java/com/yahoo/bard/webservice/web/DataApiRequest.java

+            }
+            return Optional.of(new OrderByColumn(dateTimeWithDirection.get(0), sortDirection));
+        }
+        else {


Should be up a line

cdeszaq · 2017-02-15T19:52:35Z

fili-core/src/main/java/com/yahoo/bard/webservice/web/DataApiRequest.java

+            } catch (IllegalArgumentException ignored) {
+                String sortDirectionName = dateTimeWithDirection.get(1);
+                LOG.debug(SORT_DIRECTION_INVALID.logFormat(sortDirectionName));
+                throw new BadApiRequestException(SORT_DIRECTION_INVALID.format(sortDirectionName));


Is this code duplicated with the existing code that deals with sort ordering?

cdeszaq · 2017-02-15T19:54:29Z

fili-core/src/main/java/com/yahoo/bard/webservice/web/DataApiRequest.java

+            //Requested sort on dateTime - It has to be first field in the string
+            this.dateTimeSort = generateDateTimeSortColumn(sorts.substring(0, sorts.indexOf(",")));
+            //Separating dateTimeSort from the complete api sort value string
+            sorts = sorts.substring(sorts.indexOf(",") + 1, sorts.length());


If sorts is a method parameter, it's generally a bad idea to modify method parameters. They should be treated as immutable.

cdeszaq · 2017-02-15T20:01:17Z

fili-core/src/main/java/com/yahoo/bard/webservice/web/ErrorMessageFormat.java

@@ -73,6 +73,8 @@
    SORT_METRICS_NOT_SORTABLE_FORMAT("Sorting not possible on metric(s) '%s'."),
    SORT_METRICS_UNDEFINED("Metric(s) in sort expression '%s' do not exist."),

+    DATE_TIME_SORT_VALUE_INVALID("dateTime must always be the first field in the sort list"),


Technically, it doesn't always need to be the first field in the list. It only needs to be the 1st one when it's being sorted on...

I thought we have decided to have dateTime as first value for the sort param if we want to sort based on dateTime.

My point is that if it's not sorted on at all, or there are no sorts, it doesn't need to be there.

This error message is displayed only when dateTime is part of the sort list but not the first one. If there is no sort list or if there is no request for dateTime sort, we will not reach till this error.

Right, and that's my point... the error message indicates (emphasis added):

dateTime must always be the first field

By the error message indicating always, it may be confusing. A more accurate error message would remove that confusion.

cdeszaq · 2017-02-15T20:02:16Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/endpoints/ErrorDataServletSpec.groovy

+        Response r = jtb.getHarness().target("data/shapes/day/color")
+                .queryParam("metrics","height")
+                .queryParam("dateTime","2014-09-01%2F2014-09-10")
+                .queryParam("sort","dateTime|DESC,height|ASC")


I feel like this "success" test doesn't belong in the ErrorDataServletSpec, I feel like it belongs in a Spec that's focused on sorting. If there isn't already a place that's testing that, we should make one 😉

Also, we should add a test for sorting by dateTime that doesn't specify the direction, and uses the default.

cdeszaq · 2017-02-27T19:39:27Z

Does #178 supercede this PR? Can this one be closed, or is it different from #178?

dayamr · 2017-02-27T19:53:22Z

PR #178 considers all these changes. Hence closing this PR.

[ADDIGITS-4978] dateTime specified as sortable field in sorting clause

6f296ce

cdeszaq requested changes Feb 15, 2017

View reviewed changes

dayamr changed the title ~~[ADDIGITS-4978] dateTime specified as sortable field in sorting clause~~ dateTime specified as sortable field in sorting clause Feb 15, 2017

Added more test cases and addressed review comments

e2fc470

dayamr added NEED 2 REVIEWS REVIEWABLE labels Feb 17, 2017

Added more test cases

75dbc9b

dayamr mentioned this pull request Feb 21, 2017

Added dateTime based sort feature #178

Merged

Changed the error message

2f06679

dayamr closed this Feb 27, 2017

cdeszaq deleted the ADDIGITS-4978_add_dateTime_sort branch April 4, 2017 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dateTime specified as sortable field in sorting clause #170

dateTime specified as sortable field in sorting clause #170

dayamr commented Feb 15, 2017

cdeszaq left a comment

cdeszaq Feb 15, 2017

cdeszaq Feb 15, 2017

cdeszaq Feb 15, 2017

cdeszaq Feb 15, 2017

dayamr Feb 16, 2017

cdeszaq Feb 15, 2017

cdeszaq Feb 15, 2017

cdeszaq Feb 15, 2017

cdeszaq Feb 15, 2017

cdeszaq Feb 15, 2017

dayamr Feb 16, 2017

cdeszaq Feb 21, 2017

dayamr Feb 22, 2017

cdeszaq Feb 22, 2017

cdeszaq Feb 15, 2017

cdeszaq Feb 15, 2017

cdeszaq commented Feb 27, 2017

dayamr commented Feb 27, 2017

		@@ -114,6 +116,8 @@

		private final DruidFilterBuilder filterBuilder;

		private Optional<OrderByColumn> dateTimeSort = Optional.ofNullable(null);

dateTime specified as sortable field in sorting clause #170

dateTime specified as sortable field in sorting clause #170

Conversation

dayamr commented Feb 15, 2017

cdeszaq 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

cdeszaq commented Feb 27, 2017

dayamr commented Feb 27, 2017