Skip to content

Gives the previous cursor in the scroll block #1

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

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

GCorbel
Copy link

@GCorbel GCorbel commented Aug 21, 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
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 gem was hard to understand. I don't know why there is a code for mongo and another for mongoid instead of calling the mongo code with mongoid. Conventions used, with no space, are hard to read.

I still have to add a cursor to get the first page but I think it will be in another PR.

Comment on lines +23 to +30
pipeline = [
{ '$match' => view.selector.merge(cursor.criteria) },
{ '$sort' => { scroll_field => -scroll_direction } },
{ '$limit' => limit },
{ '$sort' => { scroll_field => scroll_direction } }
]
aggregation_options = view.options.except(:sort)
records = view.aggregate(pipeline, aggregation_options)
Copy link
Author

@GCorbel GCorbel Aug 21, 2024

Choose a reason for hiding this comment

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

This :

  1. Select records before the limit in the criteria
  2. Reverse the sort
  3. Take only the number of records in limit
  4. Resort the order

Given that, if we have this list :

1
2
3
4
5 # <= Cursor here
6 

We want to have 2 records before the cursor and keep the order. Returned values have to be 3, 4.

To do that, we first select only records be the cursor :

1
2
3
4

We reverse the selection :

4
3
2
1

Take only the two first values :

4
3

And finally revert the sorting :

3
4

To avoid using aggregation, another solution was possible by sorting first with mongo and the reverse the records to display in the correct order
Another possible solution was to do the same in pure Ruby :

view.sort[scroll_field] = -scroll_direction if cursor.previous
records = Mongo::Collection::View.new(
  view.collection,
  view.selector.merge(cursor.criteria),
  sort: (view.sort || {}).merge(_id: -scroll_direction),
  skip: skip,
  limit: limit
)
records.to_a.sort { |h1, h2| h1[scroll_field] <=> h2[scroll_field] }.each do |record|
  # ...
end

Even if this version is simpler, I chose to use aggregates for performance reason.

@@ -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
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.

@@ -32,7 +33,8 @@ def to_s
field_name: field_name,
direction: direction,
include_current: include_current,
tiebreak_id: tiebreak_id && tiebreak_id.to_s
tiebreak_id: tiebreak_id && tiebreak_id.to_s,
previous: previous
Copy link
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.

@GCorbel
Copy link
Author

GCorbel commented Aug 22, 2024

I opened mongoid#38. Let me know if you want to close this PR in favor of the other one.

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.

1 participant