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

Adding suppport for reverse iteration #109

Open
2 tasks
ethe opened this issue Aug 26, 2024 · 5 comments
Open
2 tasks

Adding suppport for reverse iteration #109

ethe opened this issue Aug 26, 2024 · 5 comments
Labels
enhancement New feature or request good first issue Good for newcomers M - Medium Adding a new feature, refactoring a module, performance optimization.

Comments

@ethe
Copy link
Member

ethe commented Aug 26, 2024

In current, Transaction::scan and DB::scan only supports ascending order, it should be better to support reverse order iteration. This includes two steps tasks:

  • API design
  • the reverse iteration implentation
@ethe ethe added enhancement New feature or request good first issue Good for newcomers labels Aug 26, 2024
@tonbo-io tonbo-io deleted a comment Aug 26, 2024
@vaibhawvipul
Copy link

Taking this up

@vaibhawvipul
Copy link

This is what I am thinkging, please let me know if this is okay or feel free to mention changes.

Api design -

enum ScanOrder {
    Ascending,
    Descending,

This can be passed to DB::Scan

pub async fn scan<'scan, T: 'scan>(
        &'scan self,
        range: (Bound<&'scan R::Key>, Bound<&'scan R::Key>),
        mut f: impl FnMut(TransactionEntry<'_, R>) -> T + 'scan,
        order: ScanOrder
    ) -> impl Stream<Item = Result<T, CommitError<R>>> + 'scan

and

Also to Transaction::Scan

pub async fn scan<'scan>(
       &'scan self,
       range: (Bound<&'scan R::Key>, Bound<&'scan R::Key>),
       order: ScanOrder
   ) -> Scan<'scan, R, FP> {
       let streams = vec![TransactionScan {
           inner: self.local.range(range), // match here to wrt to scanorder to do ascending(default) or descending
           ts: self.ts,
       }
       .into()];
       Scan::new(&self.share, range, self.ts, &self.version, streams)
   }

@ethe
Copy link
Member Author

ethe commented Sep 2, 2024

Is it better to have a builder pattern here? Because I think iteration is much more common than reverse iteration, we could keep:

pub async fn scan<'scan>(
       &'scan self,
       range: (Bound<&'scan R::Key>, Bound<&'scan R::Key>),
   ) -> Scan<'scan, R, FP>

then we add reverse method on Scan, users are able to use it like

            let mut scan = txn
                .scan((Bound::Included(&name), Bound::Excluded(&upper)))
                .await
                .reverse()
                .take()
                .await
                .unwrap();

In this way, if users do not explicitly declare reverse iterating, Tonbo could use iterating as default.

@vaibhawvipul
Copy link

Aligned.

@KKould
Copy link
Contributor

KKould commented Sep 3, 2024

MergeScan consists of multiple iterators

  • MutableScan
  • ImmutableScan
  • LevelStream
  • SsTableScan
    • ParquetRecordBatchStream

These should all implement reverse

@KKould KKould added the M - Medium Adding a new feature, refactoring a module, performance optimization. label Sep 3, 2024
@ethe ethe added this to Tonbo Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers M - Medium Adding a new feature, refactoring a module, performance optimization.
Projects
Status: Todo
Development

No branches or pull requests

3 participants