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

naming inconsistency between "ranking" and "interval" #2

Open
matthme opened this issue Mar 31, 2022 · 0 comments
Open

naming inconsistency between "ranking" and "interval" #2

matthme opened this issue Mar 31, 2022 · 0 comments

Comments

@matthme
Copy link
Contributor

matthme commented Mar 31, 2022

Provided that I understand the code correctly, the term ranking is sometimes referred to the ranking of an entry and sometimes to a ranking interval which is confusing.

Here a ranking is an arbitrary integer used to rank entries:
lib.rs:28

    pub fn create_entry_ranking(
        &self,
        entry_hash: EntryHash,
        ranking: i64,
        tag: Option<SerializedBytes>,
    ) -> ExternResult<()>

According to the code below, the leaf of the path is a then a ranking interval, not a ranking.
lib.rs:178:

    fn ranking_interval(&self, ranking: i64) -> i64 {
        ranking / (self.index_interval as i64)
    }

    fn get_ranking_path(&self, ranking: i64) -> Path {
        Path::from(format!(
            "{}.{}",
            self.root_path_str(),
            self.ranking_interval(ranking)
        ))
    }

A cursor has a from_ranking attribute:

types.rs:18:

#[derive(Serialize, Deserialize, Debug, Clone)]
pub struct GetRankingCursor {
    pub from_ranking: i64,
}

However in initial_interval_index() the from_ranking attribute of the cursor is compared to the ranking interval (i.e. ranking divided by index_interval) in order to get the initial interval index:

lib.rs:235

fn initial_interval_index(
    interval_paths: &BTreeMap<i64, Path>,
    direction: GetRankingDirection,
    maybe_cursor: Option<GetRankingCursor>,
) -> usize {
    match maybe_cursor {
        None => match direction {
            GetRankingDirection::Ascendent => 0,
            GetRankingDirection::Descendent => interval_paths.len() - 1,
        },
        Some(cursor) => {
            let ordered_keys: Vec<i64> = interval_paths.keys().into_iter().cloned().collect();
            for i in 0..(interval_paths.len() - 1) {
                if ordered_keys[i] <= cursor.from_ranking //      <<<<<<<<<<<      HERE (line 248)     <<<<<<<<<<<
                    && cursor.from_ranking < ordered_keys[i + 1]
                {
                    return i;
                }
            }
            return 0;
        }
    }
}

I would propose to either rename from_ranking to from_ranking_interval or actually compare rankings with each other in line 248 by multiplying ordered_keys[i] and ordered_keys[ i+1 ] with self.index_interval. The latter is probably more intuitive for users of the module since they can provide a ranking instead of a ranking interval when specifying a cursor.

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

No branches or pull requests

1 participant