Skip to content

Conversation

@Thejas-bhat
Copy link
Member

@Thejas-bhat Thejas-bhat commented Jun 9, 2022

No description provided.

@Thejas-bhat Thejas-bhat marked this pull request as ready for review June 30, 2022 13:02
Copy link

@sreekanth-cb sreekanth-cb left a comment

Choose a reason for hiding this comment

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

.

return fdvIter, nil
}

func (di *docValueReader) BytesRead() uint64 {

Choose a reason for hiding this comment

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

commentary indicating that these methods are an interface implementation and the reference to that interface here would be useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

intDecoder.go Outdated
return rv
}

func (d *chunkedIntDecoder) bytesOffDisk() uint64 {

Choose a reason for hiding this comment

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

older func name (bytesOffDisk) lingering around?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

return sizeInBytes
}

func (i *PostingsIterator) SetBytesRead(val uint64) {

Choose a reason for hiding this comment

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

commentary for the interface implementations..

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

intDecoder.go Outdated
return rv
}

func (d *chunkedIntDecoder) BytesRead() uint64 {
Copy link
Member

Choose a reason for hiding this comment

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

Similar commentary here like with docValueReader.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

// interface, as the intention is to retrieve the bytes
// read from the on-disk segment as part of the current
// query.
func (s *Segment) SetBytesRead(val uint64) {
Copy link
Member

Choose a reason for hiding this comment

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

Now that I'm looking at this - feels a bit distasteful to allow another library to just overwrite this parameter.

Choose a reason for hiding this comment

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

I agree with you @abhinavdangeti,
@Thejas-bhat ,

Why do we need this API? This would be inherent task within the merge process.
Meaning the the merge process would take care of tracking the prev_bytes from segments and carrying it further to the newly formed segment.

Wouldn't that suffice the requirement?

Choose a reason for hiding this comment

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

hm.. it may not possible without making disk format changes.

chunkOffsetsLen := binary.BigEndian.Uint64(s.mem[fieldDvLocEnd-16 : fieldDvLocEnd-8])
// acquire position of chunk offsets
chunkOffsetsPosition = (fieldDvLocEnd - 16) - chunkOffsetsLen
s.bytesRead += uint64(16)
Copy link
Member

Choose a reason for hiding this comment

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

Some commentary here on why 16 here.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

Copy link

@sreekanth-cb sreekanth-cb left a comment

Choose a reason for hiding this comment

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

looks ok to me.

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.

3 participants