Skip to content
This repository has been archived by the owner on Feb 3, 2023. It is now read-only.

Fix pagination by time to filter entries from_time into the past (desc) #2144

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

lorenjohnson
Copy link

@lorenjohnson lorenjohnson commented Mar 6, 2020

PR summary

changelog

Added to CHANGELOG-UNRELEASED:

- Pagination option for get_links now retrieves entries before from_time, not after

documentation

  • holochain_101 summary updated to reflect change

@lorenjohnson lorenjohnson requested a review from zippy March 13, 2020 22:14
Copy link
Member

@artbrock artbrock left a comment

Choose a reason for hiding this comment

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

@lorenjohnson Please note my comment about which timestamp we're comparing to, and also which < > operator we want to be using.

@@ -13,6 +13,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
### Removed

### Fixed
- Pagination option for get_links now retrieves entries before `from_time`, not after [PR#2144](https://github.com/holochain/holochain-rust/pull/2144)
Copy link
Member

Choose a reason for hiding this comment

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

Well I certainly agree with the intention of the PR and I though we were supposed to be defaulting to descending dates in the sort order which should get you before... but I guess if that was happening, you wouldn't be trying to fix it. :)

@@ -188,7 +188,7 @@ impl DhtStore {
.skip_while(move |eavi| {
let from_time: DateTime<FixedOffset> =
paginated_time.from_time.into();
from_time.timestamp_nanos() >= eavi.index()
from_time.timestamp_nanos() <= eavi.index()
Copy link
Member

Choose a reason for hiding this comment

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

This is the line that I see a couple problems with:

  1. If the specified timestamp is LESS than the target times we're trying to retrieve, then we're going to get entries NEWER than specified, and I thought you were trying to make this go backwards to get OLDER ones? Am I misunderstanding your intent?
  2. But the bigger issue I see is that it looks like we're comparing the specified date to the eavi.index instead of the timestamp in the header of the LinkAdd entry. Normally the eavi.index is an indicator of when this node received it via gossip which may have no correlation to the order that links were published, and I'm assuming you want them in time order by when they were created, not by when a random node happened to receive the gossip about them.

Copy link
Author

@lorenjohnson lorenjohnson Mar 15, 2020

Choose a reason for hiding this comment

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

  • If the specified timestamp is LESS than the target times we're trying to retrieve, then we're going to get entries NEWER than specified, and I thought you were trying to make this go backwards to get OLDER ones? Am I misunderstanding your intent?
  1. Reversing this comparison did get me the results I was needing. I've been locally testing against a core build off this branch which did make everything work nicely all the way through to my front-end code. I believe the thing to pay attention to is that this is a skip_while loop so the expected logic can seem a bit inverted on first blush. I would still be second guessing myself if it weren't for the fact that it's simply working with this change :) I think this explanation will help, let me know if we're on the same page here yet:
Given these Entries
    8
    7
    6
    5
    4
    3

Entries from: 10, limit 3
    8 Include: 10 <= 8 is FALSE
    7 Include: 10 <= 7 is FALSE
    6 Include: 10 <= 6 is FALSE, LIMIT 3 <
    5
    4
    3

Entries from: 6, limit 3
    8 Skip: 6 <= 8 is TRUE
    7 Skip: 6 <= 7 is TRUE
    6 Skip: 6 <= 6 IS TRUE
    5 Include: 6 <= 5 IS FALSE
    4 Include: 6 <= 4 IS FALSE
    3 Include: 6 <= 3 IS FALSE, LIMIT 3 <
  1. @AshantiMutinta will know more about why she chose to implement against eavi.index, it sure does sound like what you're pointing to is an important correction though.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the line that I see a couple problems with:

1. If the specified timestamp is LESS than the target times we're trying to retrieve, then we're going to get entries NEWER than specified, and I thought you were trying to make this go backwards to get OLDER ones? Am I misunderstanding your intent?

2. But the bigger issue I see is that it looks like we're comparing the specified date to the eavi.index instead of the timestamp in the header of the LinkAdd entry. Normally the eavi.index is an indicator of when this node received it via gossip which may have no correlation to the order that links were published, and I'm assuming you want them in time order by when they were created, not by when a random node happened to receive the gossip about them.

I can make the change, eavi has a constructor that allows you to create it with the index so any node that recieves a link and stores it on their dht will do that according to the timestamp in the link topchainheader. Which means from the link perspective, there is no explicit timestamp generation on the eavi side. The reasoning was because if a node has the base it could use the eavi system to query based on timestamps.

Pro of this method is that you wouldn't have to do an extra get entry for the lazy pagination.

E.G. eavi.index is essentially link timestamp but if a distinction has to be made that I could change that around no problem

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

Successfully merging this pull request may close these issues.

4 participants