Add first and last pagination parameters #502

mpardesh · 2017-08-13T15:59:46Z

I have added the "first" and "last" pagination parameters. #352

kevinhinterlong

Overall pretty good. I think this could be a little more robust instead of using the -1 flag because we wouldn't want people passing that in as a value

kevinhinterlong · 2017-08-14T16:23:26Z

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

+            if (parameter.equals(FIRST)) {
+                return 1;
+            } else if (parameter.equals(LAST)) {
+                return LAST_PAGE;


I think it might be better to make pagination parameters use an Either<Integer, PageResultsFlag>. I know this is only really needed for the LAST page case, but using -1 as a special flag could lead to some confusion later.

enum PageResultsFlag { FIRST("first"), LAST("last"); // You could also have a function to handle #getPage(resultSize) in here for both cases }

I don't know. I think using -1 opens up the possibility for doing python-style paging, where -1 is the last page, -2 is the second to last page, etc.

Ok if we introduce python-style paging I would definitely be ok with this approach. I don't want to start using negative values as special flags though

@archolewa Would you want to change this PR to be python-style paging + first/last keywords?

I would vote yes for that

I wasn't sure what to do about this, so I just left it for now. I would be happy to make any changes.

@kevinhinterlong I don't think we should officially support python-style pages just yet. I think that's a big enough change that we would want to discuss it with others first. Plus, as I think about it, I'm honestly not sure exactly how useful it would be. Really, I think we should hold off on that until some asks for the ability to easily get the "second-to-last" page.

I don't want to add an enum for the first and last page, because then we either need to have some sort of awkward either object that stores either a number or an enum, or we need to have two fields, one of which is guaranteed to be null at any one time.

In my opinion, the first one introduces way too much awkward infrastructure for something that is very internal to the PaginationParameters object, and the second is downright confusing.

Meanwhile, -1 is a commonly used sentinel value to represent the "last" number whatever that may mean(for example, it's often used to represent infinity in configuration parameters).

If we were passing the raw page number to other parts of the system, then the extra infrastructure would be worth it.

Ok I wasn't entirely sure how much the idea of pagination was exposed in the other parts of the system which is why my preference was towards the Either.

I think -1 will be okay since it's not exposed but it should definitely be internal only and not be allowed in the api.

kevinhinterlong · 2017-08-14T16:25:06Z

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

     */
    private void validate(int parameter, String parameterName) throws BadPaginationException {
-        if (parameter < MINIMAL_VALUE) {
+        if (parameter < MINIMAL_VALUE && parameter != LAST_PAGE) {


For example one place this gets confusing is the validation. As it is now this lets us pass in perPage=-1 as a valid query parameter

Which makes sense if you think about negatives as counting backwards.

For sure but I would imagine this should still only apply to what page your asking for not the elements perPage

Yeah, I wasn't thinking about perPage when I changed this method. I will fix it 😄

kevinhinterlong · 2017-08-14T16:30:55Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/util/PaginationParametersSpec.groovy

@@ -19,12 +19,15 @@ class PaginationParametersSpec extends Specification {

        then: "The handler's perPage and page are as expected"
        handler.getPerPage() == expectedPerPage
-        handler.getPage() == expectedPage
+        handler.getPage(10) == expectedPage


Make this a parameter in the where: section to add more tests

kevinhinterlong · 2017-08-14T16:32:30Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/util/PaginationParametersSpec.groovy

+        invalidPerPage << ["0", "-2", "1.0", "2.3", "$Long.MAX_VALUE", "1a", "AndrewIsMostlyHarmless."]
+    }
+
+    def "If the result set is empty and the page is last then getPage returns the first page"() {


You could combine this with build the correct handler when 'perPage' is #perPage and 'page' is #page if you make the resultSize a parameter

archolewa

We may want to move up our validation some. While it's simpler to just have some undocumented behavior that allows -1 to be a valid value, that does result in an API change (rather than an API addition).

This could lead to weird behavior if say a UI doesn't allow negative page numbers and relies on Fili to perform that check.

Instead, we should probably validate before we parse the page number. So while we internally use -1 to represent the last page, we don't allow the customer to specify a -1.

archolewa · 2017-08-17T21:53:19Z

fili-core/src/main/java/com/yahoo/bard/webservice/data/dimension/impl/LuceneSearchProvider.java

                );
                hits = hitDocs.scoreDocs;
                documentCount = hitDocs.totalHits;
+                int requestedPageNumber = paginationParameters.getPage(documentCount);


As I think about it, I'm not sure this will work.

We'll need to dig into the Lucene library some to be sure, but I'm afraid that the hitDocs contains only the first page of data, so hitDocs.totalHits <= perPage, not the total number of possible documents.

Yay! totalHits is really the total number of results, so this should work.

archolewa · 2017-08-17T21:56:57Z

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

@@ -114,11 +122,37 @@ private void validate(int parameter, String parameterName) throws BadPaginationE
        }
    }

+    /**
+     * Verifies that the page is positive or is -1 (last page).


Rather than saying "or is -1" we should say "or is the last page."

As far as this method is concerned, it shouldn't really care what value we use to represent the last page.

kevinhinterlong

I agree with Andrew's suggested changes, and as long as we can verify/fix it so that it works with Lucene then this will also need a docs/ update.

I'm not really sure if we have something in place already for marking what version a feature became available in the api but we'd need to make a note of it somehow

archolewa · 2017-08-24T15:20:10Z

@mpardesh Can you rebase this on top of master?

kevinhinterlong

Ok I had some comments about the test cases which could be reverted, but I'm pretty sure this has the correct behavior so it's not super necessary and I'm still going to approve it.

Also this should probably update docs/end-user-api.md or at least make a new issue for it

kevinhinterlong · 2017-08-24T16:23:52Z

...rc/test/groovy/com/yahoo/bard/webservice/web/endpoints/DimensionsServletComponentSpec.groovy

@@ -906,7 +908,7 @@ class DimensionsServletComponentSpec extends Specification {

    def "test negative page number is handled correctly"() {
        setup:
-        Map<String, String> queryParams = [page: -1, perPage: 3]
+        Map<String, String> queryParams = [page: -2, perPage: 3]


Couldn't this be reverted to -1

kevinhinterlong · 2017-08-24T16:24:58Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/util/PaginationParametersSpec.groovy

@@ -38,11 +45,11 @@ class PaginationParametersSpec extends Specification {

        where:
        perPage | page  || expectedError
-        "-1"  | "-1"    || errorMessage("perPage", "-1")


Same for these test cases

kevinhinterlong · 2017-08-24T16:25:21Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/util/PaginationParametersSpec.groovy

@@ -115,7 +122,7 @@ class PaginationParametersSpec extends Specification {
        thrown(BadPaginationException)

        where:
-        invalidPage << ["0", "-1", "1.0", "2.3", "$Long.MAX_VALUE", "1a", "AndrewIsMostlyHarmless."]
+        invalidPage << ["0", "-2", "1.0", "2.3", "$Long.MAX_VALUE", "1a", "AndrewIsMostlyHarmless."]


kevinhinterlong · 2017-08-24T16:25:27Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/util/PaginationParametersSpec.groovy

@@ -130,7 +137,7 @@ class PaginationParametersSpec extends Specification {
        thrown(BadPaginationException)

        where:
-        invalidPerPage << ["0", "-1", "1.0", "2.3", "$Long.MAX_VALUE", "1a", "AndrewIsMostlyHarmless."]
+        invalidPerPage << ["0", "-2", "1.0", "2.3", "$Long.MAX_VALUE", "1a", "AndrewIsMostlyHarmless."]


archolewa · 2017-08-24T20:09:04Z

@mpardesh Could you please rebase this on top of master?

yahoo deleted a comment Aug 13, 2017

kevinhinterlong added NEED 2 REVIEWS REVIEWABLE labels Aug 14, 2017

kevinhinterlong requested changes Aug 14, 2017

View reviewed changes

yahoo deleted a comment Aug 15, 2017

archolewa requested changes Aug 17, 2017

View reviewed changes

kevinhinterlong requested changes Aug 17, 2017

View reviewed changes

yahoo deleted a comment Aug 18, 2017

archolewa approved these changes Aug 24, 2017

View reviewed changes

archolewa added NEED 1 REVIEW and removed NEED 2 REVIEWS labels Aug 24, 2017

mpardesh force-pushed the pagination branch from 6f219aa to a6db85b Compare August 24, 2017 15:41

yahoo deleted a comment Aug 24, 2017

kevinhinterlong approved these changes Aug 24, 2017

View reviewed changes

kevinhinterlong added MERGEABLE and removed NEED 1 REVIEW REVIEWABLE labels Aug 24, 2017

yahoo deleted a comment Aug 24, 2017

mpardesh added 6 commits August 24, 2017 15:36

added first and last page

8f1affd

added first and last tests to DataPaginationSpec

8116d42

add first and last pagination parameters

9755896

uncommented tests

cb9304b

fixed validate function and cleaned tests

15c41df

moved validation into parsing

9c8bb16

mpardesh added 2 commits August 24, 2017 15:36

edited documentation

231d6a0

reverted tests

150843f

mpardesh force-pushed the pagination branch from c5a091c to 150843f Compare August 24, 2017 20:37

yahoo deleted a comment Aug 24, 2017

Merge branch 'master' into pagination

88f006f

yahoo deleted a comment Aug 24, 2017

Merge branch 'master' into pagination

8037ab7

yahoo deleted a comment Aug 24, 2017

archolewa merged commit 118a4ff into yahoo:master Aug 25, 2017

Add first and last pagination parameters #502

Add first and last pagination parameters #502

Uh oh!

Conversation

mpardesh commented Aug 13, 2017

Uh oh!

kevinhinterlong 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!

kevinhinterlong Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

archolewa Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

archolewa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpardesh Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinhinterlong left a comment

Choose a reason for hiding this comment

Uh oh!

archolewa commented Aug 24, 2017

Uh oh!

kevinhinterlong left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

archolewa commented Aug 24, 2017

Uh oh!

Uh oh!

kevinhinterlong Aug 15, 2017 •

edited

Loading

archolewa Aug 17, 2017 •

edited

Loading

mpardesh Aug 17, 2017 •

edited

Loading

kevinhinterlong left a comment •

edited

Loading