Skip to content

Conversation

@sebheitzmann
Copy link
Contributor

The contains of the readers already handle the "tombstone" case.

The contains of the readers already handle the "tombstone" case.
if !keyExist {
continue
if keyExist {
return true, nil
Copy link
Owner

Choose a reason for hiding this comment

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

do you mind to add a unit test for this too? I wonder why this just passes :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will check if the test cover this

Copy link
Owner

Choose a reason for hiding this comment

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

added this as part of 44b5b67

thanks for fixing this as well, I'll see whether we need some more test coverage around this right now

@sebheitzmann
Copy link
Contributor Author

Just a question :

func (m SSTableMerger) Merge(iterators []SSTableMergeIteratorContext, writer SSTableStreamWriterI) (err error) {

func (m SSTableMerger) MergeCompactIterator(iterators []SSTableMergeIteratorContext, reduce ReduceFunc) (SSTableIteratorI, error) {

There is two merge method. I don't really understand the difference. And I don't understand the need of the context.

@thomasjungblut
Copy link
Owner

of course. Merge just merges the given iterators into a single file, so it will have duplicated key entries with their respective values.

MergeCompactIterator returns an iterator that merges and runs the reduce function on the same keys, so the iterator always returns a key and all the matching values.

@sebheitzmann
Copy link
Contributor Author

Thanks for the response. But what about the context ?

@thomasjungblut
Copy link
Owner

thomasjungblut commented Apr 10, 2025

The context is to understand which sstable the iterator originated from, so you can do selection based on this too, e.g. check out: https://github.com/thomasjungblut/go-sstables/blob/main/sstables/super_sstable_reader.go#L57-L63

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.

2 participants