From 62252f7216d0001b73f374617b7acb6c37cbeb74 Mon Sep 17 00:00:00 2001 From: Luc Mcgrady Date: Tue, 16 Dec 2025 16:51:13 +0000 Subject: [PATCH] Fix/Retrievability SQL (#4424) * Fix/SQL Retrievability Underflow * Added: Reminder * ./check * Apply code diff changes Co-authored-by: user1823 <92206575+user1823@users.noreply.github.com> * Slight cleanup * fix bug again * extra label comment * Update rslib/src/storage/sqlite.rs Co-authored-by: user1823 <92206575+user1823@users.noreply.github.com> * Fix: Ignore new cards * use const --------- Co-authored-by: user1823 <92206575+user1823@users.noreply.github.com> --- rslib/src/search/sqlwriter.rs | 3 +- rslib/src/storage/card/filtered.rs | 2 +- rslib/src/storage/card/mod.rs | 2 +- rslib/src/storage/sqlite.rs | 65 ++++++++++++++++-------------- 4 files changed, 38 insertions(+), 34 deletions(-) diff --git a/rslib/src/search/sqlwriter.rs b/rslib/src/search/sqlwriter.rs index f6237d6fd..2479b87ae 100644 --- a/rslib/src/search/sqlwriter.rs +++ b/rslib/src/search/sqlwriter.rs @@ -434,9 +434,10 @@ impl SqlWriter<'_> { let timing = self.col.timing_today()?; (timing.days_elapsed, timing.next_day_at, timing.now) }; + const NEW_TYPE: i8 = CardType::New as i8; write!( self.sql, - "extract_fsrs_retrievability(c.data, case when c.odue !=0 then c.odue else c.due end, c.ivl, {elap}, {next_day_at}, {now}) {op} {r}" + "case when c.type = {NEW_TYPE} then false else (extract_fsrs_retrievability(c.data, case when c.odue !=0 then c.odue else c.due end, c.ivl, {elap}, {next_day_at}, {now}) {op} {r}) end" ) .unwrap() } diff --git a/rslib/src/storage/card/filtered.rs b/rslib/src/storage/card/filtered.rs index ef436f6e8..03f845f4e 100644 --- a/rslib/src/storage/card/filtered.rs +++ b/rslib/src/storage/card/filtered.rs @@ -54,7 +54,7 @@ fn build_retrievability_query( ) -> String { if fsrs { format!( - "extract_fsrs_relative_retrievability(c.data, case when c.odue !=0 then c.odue else c.due end, {today}, ivl, {next_day_at}, {now}) {order}" + "extract_fsrs_relative_retrievability(c.data, case when c.odue !=0 then c.odue else c.due end, ivl, {today}, {next_day_at}, {now}) {order}" ) } else { format!( diff --git a/rslib/src/storage/card/mod.rs b/rslib/src/storage/card/mod.rs index 3a5066ff4..9e06edf07 100644 --- a/rslib/src/storage/card/mod.rs +++ b/rslib/src/storage/card/mod.rs @@ -837,7 +837,7 @@ impl fmt::Display for ReviewOrderSubclause { let next_day_at = timing.next_day_at.0; let now = timing.now.0; temp_string = - format!("extract_fsrs_relative_retrievability(data, case when odue !=0 then odue else due end, {today}, ivl, {next_day_at}, {now}) {order}"); + format!("extract_fsrs_relative_retrievability(data, case when odue !=0 then odue else due end, ivl, {today}, {next_day_at}, {now}) {order}"); &temp_string } ReviewOrderSubclause::Added => "nid asc, ord asc", diff --git a/rslib/src/storage/sqlite.rs b/rslib/src/storage/sqlite.rs index 3ce1baff0..95853afc9 100644 --- a/rslib/src/storage/sqlite.rs +++ b/rslib/src/storage/sqlite.rs @@ -332,23 +332,30 @@ fn add_extract_fsrs_retrievability(db: &Connection) -> rusqlite::Result<()> { return Ok(None); }; let seconds_elapsed = if let Some(last_review_time) = card_data.last_review_time { - now.saturating_sub(last_review_time.0) as u32 + // This and any following + // (x as u32).saturating_sub(y as u32) + // must not be changed to + // x.saturating_sub(y) as u32 + // as x and y are i64's and saturating_sub will therfore allow negative numbers + // before converting to u32 in the latter example. + (now as u32).saturating_sub(last_review_time.0 as u32) } else if due > 365_000 { // (re)learning card in seconds let Ok(ivl) = ctx.get_raw(2).as_i64() else { return Ok(None); }; - let last_review_time = due.saturating_sub(ivl); - now.saturating_sub(last_review_time) as u32 + let last_review_time = (due as u32).saturating_sub(ivl as u32); + (now as u32).saturating_sub(last_review_time) } else { let Ok(ivl) = ctx.get_raw(2).as_i64() else { return Ok(None); }; - let Ok(days_elapsed) = ctx.get_raw(3).as_i64() else { + // timing.days_elapsed + let Ok(today) = ctx.get_raw(3).as_i64() else { return Ok(None); }; - let review_day = due.saturating_sub(ivl); - days_elapsed.saturating_sub(review_day) as u32 * 86_400 + let review_day = (due as u32).saturating_sub(ivl as u32); + (today as u32).saturating_sub(review_day) * 86_400 }; let decay = card_data.decay.unwrap_or(FSRS5_DEFAULT_DECAY); let retrievability = card_data.memory_state().map(|state| { @@ -364,7 +371,7 @@ fn add_extract_fsrs_retrievability(db: &Connection) -> rusqlite::Result<()> { } /// eg. extract_fsrs_relative_retrievability(card.data, card.due, -/// timing.days_elapsed, card.ivl, timing.next_day_at, timing.now) -> float | +/// card.ivl, timing.days_elapsed, timing.next_day_at, timing.now) -> float | /// null. The higher the number, the higher the card's retrievability relative /// to the configured desired retention. fn add_extract_fsrs_relative_retrievability(db: &Connection) -> rusqlite::Result<()> { @@ -378,25 +385,32 @@ fn add_extract_fsrs_relative_retrievability(db: &Connection) -> rusqlite::Result let Ok(due) = ctx.get_raw(1).as_i64() else { return Ok(None); }; - let Ok(interval) = ctx.get_raw(3).as_i64() else { + let Ok(interval) = ctx.get_raw(2).as_i64() else { return Ok(None); }; + /* + // Unused let Ok(next_day_at) = ctx.get_raw(4).as_i64() else { return Ok(None); }; + */ let Ok(now) = ctx.get_raw(5).as_i64() else { return Ok(None); }; - let days_elapsed = if due > 365_000 { - // (re)learning - (next_day_at as u32).saturating_sub(due as u32) / 86_400 + let secs_elapsed = if due > 365_000 { + // (re)learning card with due in seconds + + // Don't change this to now.subtracting_sub(due) as u32 + // for the same reasons listed in the comment + // in add_extract_fsrs_retrievability + (now as u32).saturating_sub(due as u32) } else { - let Ok(days_elapsed) = ctx.get_raw(2).as_i64() else { + // timing.days_elapsed + let Ok(today) = ctx.get_raw(3).as_i64() else { return Ok(None); }; let review_day = due.saturating_sub(interval); - - (days_elapsed as u32).saturating_sub(review_day as u32) + (today as u32).saturating_sub(review_day as u32) * 86_400 }; if let Ok(card_data) = ctx.get_raw(0).as_str() { if !card_data.is_empty() { @@ -410,23 +424,12 @@ fn add_extract_fsrs_relative_retrievability(db: &Connection) -> rusqlite::Result let seconds_elapsed = if let Some(last_review_time) = card_data.last_review_time { - now.saturating_sub(last_review_time.0) as u32 - } else if due > 365_000 { - // (re)learning card in seconds - let Ok(ivl) = ctx.get_raw(2).as_i64() else { - return Ok(None); - }; - let last_review_time = due.saturating_sub(ivl); - now.saturating_sub(last_review_time) as u32 + // Don't change this to now.subtracting_sub(due) as u32 + // for the same reasons listed in the comment + // in add_extract_fsrs_retrievability + (now as u32).saturating_sub(last_review_time.0 as u32) } else { - let Ok(ivl) = ctx.get_raw(2).as_i64() else { - return Ok(None); - }; - let Ok(days_elapsed) = ctx.get_raw(3).as_i64() else { - return Ok(None); - }; - let review_day = due.saturating_sub(ivl); - days_elapsed.saturating_sub(review_day) as u32 * 86_400 + secs_elapsed }; let current_retrievability = FSRS::new(None) @@ -441,7 +444,7 @@ fn add_extract_fsrs_relative_retrievability(db: &Connection) -> rusqlite::Result } } } - + let days_elapsed = secs_elapsed / 86_400; // FSRS data missing; fall back to SM2 ordering Ok(Some( -((days_elapsed as f32) + 0.001) / (interval as f32).max(1.0),