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

Gives the previous cursor in the scroll block #38

Merged
merged 15 commits into from
Aug 29, 2024

Conversation

GCorbel
Copy link
Contributor

@GCorbel GCorbel commented Aug 22, 2024

In addition to the next cursor, blocks in Mongo::Scrollable#scroll and Mongoid::Criteria::Scrollable gives an additional cursor with allow to fetch records before the first one returned.

The previous cursor is created based on the first record with a previous flag set to true. For example, if we have Feed::Item with a name from 0 to 3, we have this behavior :

cursor = nil
first_previous_cursor = nil

Feed::Item.asc(:name).limit(2).scroll do |record, next_cursor|
  puts record.name # => 0, 1
  cursor = next_cursor
end

Feed::Item.asc(:name).limit(2).scroll(cursor) do |record, _, previous_record|
  puts record.name # => 2, 3
  first_previous_cursor = previous_cursor
end

Feed::Item.asc(:name).limit(2).scroll(first_previous_cursor) do |record|
  puts record.name # => 0, 1
  second_previous_cursor = previous_cursor
end

Copy link
Contributor Author

@GCorbel GCorbel left a comment

Choose a reason for hiding this comment

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

This is far from being perfect, but it is backward compatible and works for the case I need.

Improvements can be done, as give an hash in the block as :


Feed::Item.asc(:name).limit(2).scroll do |record, meta_data|
  meta_data[:next_cursor]
  meta_data[:previous_cursor]
  meta_data[:total]
  # ...
end

But it is not backward compatible and needs more work.

lib/mongo/scrollable.rb Outdated Show resolved Hide resolved
@@ -3,17 +3,18 @@
describe Mongoid::Scroll::Base64EncodedCursor do
context 'new' do
context 'an empty cursor' do
let(:base64_string) { 'eyJ2YWx1ZSI6bnVsbCwiZmllbGRfdHlwZSI6IlN0cmluZyIsImZpZWxkX25hbWUiOiJhX3N0cmluZyIsImRpcmVjdGlvbiI6MSwiaW5jbHVkZV9jdXJyZW50IjpmYWxzZSwidGllYnJlYWtfaWQiOm51bGx9' }
let(:base64_string) { 'eyJ2YWx1ZSI6bnVsbCwiZmllbGRfdHlwZSI6IlN0cmluZyIsImZpZWxkX25hbWUiOiJhX3N0cmluZyIsImRpcmVjdGlvbiI6MSwiaW5jbHVkZV9jdXJyZW50IjpmYWxzZSwidGllYnJlYWtfaWQiOm51bGwsInByZXZpb3VzIjpmYWxzZX0=' }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tokens changed because previous: false is encoded.

@@ -1,7 +1,7 @@
module Mongoid
module Scroll
class BaseCursor
attr_accessor :value, :tiebreak_id, :field_type, :field_name, :direction, :include_current
attr_accessor :value, :tiebreak_id, :field_type, :field_name, :direction, :include_current, :previous
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about the naming of the previous flag.

Copy link
Collaborator

@dblock dblock Aug 26, 2024

Choose a reason for hiding this comment

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

Per my comment in #38 (comment), this could be an optional cursor itself, previous_cursor and it would be deserialized into a BaseCursor.

@dblock
Copy link
Collaborator

dblock commented Aug 22, 2024

Impressive that you got it working :)

Let's start by adding to README? I'll do a thorough review when I get a moment.

Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I took a detailed look. I could be missing something but I think this is a different feature of scrolling backwards. I understand how that yields the previous cursor, but I believe this is more complicated than necessary. I am not sure of the value of scrolling backwards by making inverse queries, a caller can simply inverse the order by which to scroll.

So, a super inefficient and naive scroll backwards reaches the end saving the list of cursors, then yielding them in reverse order. In some way we want that for the previous cursor minus the inefficiencies of making all the queries.

A possibly much simpler implementation:

  1. Start scroll, the starting cursor is nil.
  2. Every page we may have the current cursor that we unpack and make a view with, and generate a cursor (aka a link to the next page with the last item in our page).
  3. Include the information about the current cursor in the generated next page one.
  4. Add a property to the unpacked cursor/page called previous that returns (3), now you can make queries with it.

Basically, instead of previous being a boolean, make it the actual previous cursor "as is".

WDYT?

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/mongo/scrollable.rb Outdated Show resolved Hide resolved
@GCorbel
Copy link
Contributor Author

GCorbel commented Aug 26, 2024

Thanks for the fast answer.

I can explain what I need more in details.

ATM, In an API payload, we return the hash of a cursor as :

{
    "records": [
       # ...
    ],
    "paging": {
        "pageToken": "eyJ2YWx1ZSI6MTQwNTA4MTQ4Ni4yNDUsImZpZWxkX3R5cGUiOiJUaW1lIiwiZmllbGRfbmFtZSI6ImNyZWF0ZWRfYXQiLCJkaXJlY3Rpb24iOjEsImluY2x1ZGVfY3VycmVudCI6dHJ1ZSwidGllYnJlYWtfaWQiOiI1M2JmZDc4ZTdhYTU4ZDlkOWUwMDAwYjYifQ==",
        "perPage": 2,
        "nextPageToken": "eyJ2YWx1ZSI6MTQwNTA4Mzc2Ni44MzQsImZpZWxkX3R5cGUiOiJUaW1lIiwiZmllbGRfbmFtZSI6ImNyZWF0ZWRfYXQiLCJkaXJlY3Rpb24iOjEsImluY2x1ZGVfY3VycmVudCI6ZmFsc2UsInRpZWJyZWFrX2lkIjoiNTNiZmUwNzYwZjRjYTE5MWZiMDAwMDQ4In0="
    }
}

We can take the hash in nextPageToken and add it in the request parameter as https://test.com?pageToken=eyJ2YWx1ZSI6MTQwNTA4Mzc2Ni44MzQsImZpZWxkX3R5cGUiOiJUaW1lIiwiZmllbGRfbmFtZSI6ImNyZWF0ZWRfYXQiLCJkaXJlY3Rpb24iOjEsImluY2x1ZGVfY3VycmVudCI6ZmFsc2UsInRpZWJyZWFrX2lkIjoiNTNiZmUwNzYwZjRjYTE5MWZiMDAwMDQ4In0=

Then we take this param as :

cursor = ::Mongoid::Scroll::Base64EncodedCursor.new(params["page_token"])
criteria.limit(limit).scroll(cursor) do |record, next_cursor|
   # ....
end

My need is to be able to do the same, but with the records before the records listed.

Given that, as we don't loop over each record, your proposal does not work.

@GCorbel GCorbel requested a review from dblock August 26, 2024 18:32
@dblock
Copy link
Collaborator

dblock commented Aug 26, 2024

@GCorbel I'm a little slow on this one, so please bear with me.

I think I understand what you're trying to do. First, question: can one call your API and land randomly somewhere in the records or does it always require a cursor to land in the middle of data, except for page 1 where the cursor is nil?

If you don't want to start a cursor in a random position, I think you don't need all this previous logic. Embed the previous cursor in every cursor in this library.

- Your /api yields currently page2-cursor, instead yield a pair (page2-cursor, nil).
- Your /api?cursor=(page2-cursor, nil) currently yields page3-cursor, instead yield (page3-cursor,page2-cursor).
- Your /api?cursor=(page3-cursor, page2-cursor) currently yields page4-cursor, instead yield (page4-cursor,page3-cursor).
- etc.

What I am proposing above is to change cursor to (optionally) store its previous cursor. Of course you can burden your API to return and remember it the same way, too, but I think this is a good feature for this library.

Now, if you do want to produce a cursor from a random position in data (one that wasn't reached with the above iteration), so then your solution of reversing the order, making a database query, fetching the new position, re-generating a cursor in the scroll API works. But that's obviously very inefficient, so let's make sure we need this.

@dblock
Copy link
Collaborator

dblock commented Aug 26, 2024

.... actually now that I think about it more, my suggestion might not work - we'd need to store the entire history of cursors for every cursor, and that's going to be ... a lot 🤦‍♂️

@dblock
Copy link
Collaborator

dblock commented Aug 26, 2024

So, a possibly simpler idea - cursor.previous would do the aggregation and return the previous cursor? This way we don't need to yield anything or add another option?

@GCorbel
Copy link
Contributor Author

GCorbel commented Aug 26, 2024

ok so, to be consistent, we should also have cursor.next ?

@GCorbel
Copy link
Contributor Author

GCorbel commented Aug 26, 2024

cursor.previous would do the aggregation and return the previous cursor?

It means we will have an extra query each time, right ?

@GCorbel
Copy link
Contributor Author

GCorbel commented Aug 26, 2024

I tried very rapidly to implement a method Mongoid::Scroll::Cursor#previous that build a cursor base on the first record returned with the aggregation and wasn't able as we need Mongoid::Criteria#options and Mongoid::Criteria#view that are not available in instances of Mongoid::Scroll::Cursor. We need to alter the implementation to make it works.

Anyway, as it require an extra call each time to use Mongoid::Criteria::Scrollable#scroll, I'm not sure if it's a good idea.

Let me know what you think.

Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Ok, I think you had it right the first time, your implementation is better. Thanks for hanging in here with me.

You're right to ask about naming previous. It's confusing because it's about the nature of the cursor. How about we introduce a type and give it a number (0 for next, 1 for previous)? Other possibility is to all this azimuth or navigation - I'd use a symbol or a number either way. Who knows what kind of other cursors we'll come up with :)

README.md Show resolved Hide resolved
@GCorbel
Copy link
Contributor Author

GCorbel commented Aug 27, 2024

How about we introduce a type and give it a number (0 for next, 1 for previous)?

I will try with type that can be :next or :previous. It may be :first, :current, etc.

Who knows what kind of other cursors we'll come up with :)

I may need to have a "firstPageToken" so I may add a "first_cursor". That's for another PR.

lib/mongo/scrollable.rb Outdated Show resolved Hide resolved
Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Looks great. Some typo-things below.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
UPGRADING.md Outdated Show resolved Hide resolved
GCorbel and others added 2 commits August 29, 2024 09:55
@GCorbel
Copy link
Contributor Author

GCorbel commented Aug 29, 2024

Typos are fixed. Thanks.

@GCorbel GCorbel requested a review from dblock August 29, 2024 14:29
@dblock dblock merged commit 56f04f5 into mongoid:master Aug 29, 2024
12 checks passed
@dblock
Copy link
Collaborator

dblock commented Aug 29, 2024

Merged, 👏

Now that you wrote half of this library, would you like to join in as a (co)maintainer? Maybe make the next release?

If yes, drop me a note to dblock[at]dblock[dot]org with your rugygems username.

@dblock dblock mentioned this pull request Aug 29, 2024
@GCorbel
Copy link
Contributor Author

GCorbel commented Sep 3, 2024

Thanks for the merge. I enjoy working with you.

I'm not very interested in being a maintainer, unless you don't want to be anymore.

@dblock
Copy link
Collaborator

dblock commented Sep 3, 2024

I'm not very interested in being a maintainer, unless you don't want to be anymore.

Too bad :( I don't mind, just trying to reduce bus factor. Appreciate any help you can give like on a few issues I opened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants