Returning bytesRead as part of searchResult #1752

Thejas-bhat · 2022-11-04T16:32:15Z

Refactoring the bytesRead accounting to now be accounted at a per query level. The searchResult returned by SearchInContext now has an extra field called bytesRead which helps to track the cost associated with the query.

The implementation to achieve this basically involves passing a context.Context which has a callback that is used to track or update the bytes read associated with some operation. This change of adding an extra parameter context to the searcher is an invasive one, and hence involves updating a lot of existing files to accommodate the same. However, the good thing is that the adding of context to searcher is in a way a future proof change, which hopefully might be useful to do any more useful things down at the searcher level and relay them back to the indexReader level.

…ation

- bug fixes with respect to bytes read tracking in collector, tfr and docValueReader - fixing boolean searcher's BytesRead() API

with numbers after the bug fixes.

with a SetBytesRead() API. Currently the only valid implmentations is with disjunct and conjunct searchers. - introducing BytesRead for newSnapshotIndexFieldDict. This is necessary for fuzzy, regex like queries. - updating the tests with the changes - updating rest of interface dependencies with no-op APIs in their implementations

regexp queries in the seacher - minor code refactor with respect to tfr

the concept of context right at the searcher level. this will essentially help in bytesRead tracking as well.

and reporting the same as part of search result. - and yes, it works :D - removed the BytesRead and SetBytesRead API from searcher

- fixed a race condition around the newSnapshotIndexFieldDict - using the callbacks to return the bytes read value with respect to fieldDict in regexp, fuzzy etc type of queries, where there is a index.FieldDict creation involved (which loads the termDictionary for that field) - updated the tests reflecting the bug fixes. - removing the BytesRead API for index.TermFieldreader

abhinavdangeti

Mostly nits, I'll make another pass over this soon.
Meanwhile you should get some perf results with these changes.

abhinavdangeti · 2022-11-04T19:42:08Z

document/document.go

+	storedFieldsBytes uint64
+}
+
+func (d *Document) SetStoredFieldsBytes(val uint64) {


Drop this method you can updated storedFieldsBytes by simply exporting it (like CompositeFields above).

abhinavdangeti · 2022-11-04T19:42:24Z

go.mod

@@ -40,3 +40,17 @@ require (
 	github.com/spf13/pflag v1.0.5 // indirect
 	golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a // indirect
 )
+
+replace github.com/blevesearch/zapx/v15 => ../zapx


Remove all these replaces.

Yeah just got pushed to the review as part of "git add .". Removing them.

abhinavdangeti · 2022-11-04T19:42:53Z

index/scorch/snapshot_index.go

@@ -430,13 +432,17 @@ func (i *IndexSnapshot) Document(id string) (rv index.Document, err error) {
 	segmentIndex, localDocNum := i.segmentIndexAndLocalDocNumFromGlobal(docNum)

 	rvd := document.NewDocument(id)
-	prevBytesRead := i.segment[segmentIndex].segment.BytesRead()
+	var totalStoredBytes int


remove variable.

abhinavdangeti · 2022-11-04T19:43:42Z

index/scorch/snapshot_index.go

+		// track uncompressed stored fields bytes as part of IO stats.
+		// However, ideally we'd need to track the compressed on-disk value
+		// Keeping that TODO for now until we have a cleaner way.
+		totalStoredBytes += len(val)


rvd.StoredFieldsBytes += len(val)

Separate question - shouldn't you be doing this only when the field is actually "stored"?

This callback is called only for the stored fields. The VisitDocument API ends up calling the VisitStoredFields in the zap layer which loads the bytes pertaining to the stored fields, uncompresses them and iterates over each of these fields and applies this callback.

abhinavdangeti · 2022-11-04T19:44:48Z

index/upsidedown/index_reader.go

 	"reflect"

 	"github.com/blevesearch/bleve/v2/document"
 	index "github.com/blevesearch/bleve_index_api"
-	"github.com/blevesearch/upsidedown_store_api"
+	store "github.com/blevesearch/upsidedown_store_api"


This changes seems unrelated?

This popped up because of the vscode extension I believe, removing it.

abhinavdangeti · 2022-11-04T19:44:57Z

index/upsidedown/reader.go

@@ -22,7 +22,7 @@ import (

 	"github.com/blevesearch/bleve/v2/size"
 	index "github.com/blevesearch/bleve_index_api"
-	"github.com/blevesearch/upsidedown_store_api"
+	store "github.com/blevesearch/upsidedown_store_api"


Unrelated change?

This popped up because of the vscode extension I believe, removing it.

abhinavdangeti · 2022-11-04T20:40:46Z

document/document.go

+	d.storedFieldsBytes = val
+}
+
+func (d *Document) GetStoredFieldsBytes() uint64 {


This method can go away too, with storedFieldsBytes exported.

This Get type of API might be necessary since the struct is hidden behind the Document interface.

- removing bytesRead from search.DocumentMatch

abhinavdangeti · 2022-11-09T20:22:56Z

@Thejas-bhat

abhinavdangeti

Looking good, couple minor comments.

abhinavdangeti · 2022-11-10T17:03:10Z

index/scorch/snapshot_index.go

@@ -37,6 +38,7 @@ import (
 // re usable, threadsafe levenshtein builders
 var lb1, lb2 *lev.LevenshteinAutomatonBuilder

+type diskStatsReporter segment.DiskStatsReporter


Is this step necessary, can't we just type assert to segment.DiskStatsReporter?

Yeah at a couple of places, there are library name and variable name conflicts, for eg. indexSnapshot.segment.segment .

Ah variable and import conflict. Should update the variable name then (it looks quite confusing :)). I'll make that change separately.

abhinavdangeti · 2022-11-10T17:08:04Z

index_impl.go

+	//  2. the docvalues portion (accounted in collector) and the retrieval
+	//     of stored fields bytes (by LoadAndHighlightFields)
+	var totalBytesRead uint64
+	SendBytesRead := func(bytesRead uint64) {


Does this closure need to be exported? Would the compiler complain on sendBytesRead?

Aah, my bad.

abhinavdangeti · 2022-11-11T16:39:40Z

index/scorch/snapshot_index.go

@@ -37,6 +38,7 @@ import (
 // re usable, threadsafe levenshtein builders
 var lb1, lb2 *lev.LevenshteinAutomatonBuilder

+type diskStatsReporter segment.DiskStatsReporter


Ah variable and import conflict. Should update the variable name then (it looks quite confusing :)). I'll make that change separately.

Thejas-bhat added 10 commits October 26, 2022 19:36

refactoring the bytes read while querying

34a4f10

bug fix: incrementing the bytes read at the right time

05c82ee

bug fix: fixing doc value and stored fields section bytes read comput…

11459c9

…ation

- updating stored fields' bytes read logic

104149b

- bug fixes with respect to bytes read tracking in collector, tfr and docValueReader - fixing boolean searcher's BytesRead() API

- updating TestBytesRead and TestBytesReadStored

aa11596

with numbers after the bug fixes.

- accounting the bytes read for prefix, range

c1dce84

regexp queries in the seacher - minor code refactor with respect to tfr

a huge experimental change by introducing

659efd0

the concept of context right at the searcher level. this will essentially help in bytesRead tracking as well.

- using the IOstats reporter callback in term field reader

466a965

and reporting the same as part of search result. - and yes, it works :D - removed the BytesRead and SetBytesRead API from searcher

Thejas-bhat requested review from abhinavdangeti, metonymic-smokey and moshaad7 November 4, 2022 16:32

Thejas-bhat marked this pull request as ready for review November 4, 2022 16:32

abhinavdangeti requested changes Nov 4, 2022

View reviewed changes

abhinavdangeti reviewed Nov 4, 2022

View reviewed changes

- including the review comments and minor code cleanup

6d4cf3f

Thejas-bhat force-pushed the bytesReadPerQuery branch from 611130d to 6d4cf3f Compare November 7, 2022 04:34

Thejas-bhat added 2 commits November 7, 2022 11:02

- addressing review comments, minor code cleanup

fbd1340

- removing bytesRead from search.DocumentMatch

bug fixes with respect to reporting of the bytesRead

1b24b8b

abhinavdangeti added this to the v2.3.6 milestone Nov 8, 2022

updating test plans

584b697

updating go.mod and go.sum

d6f6845

Thejas-bhat requested a review from abhinavdangeti November 10, 2022 06:12

added some comments

6d20330

abhinavdangeti reviewed Nov 10, 2022

View reviewed changes

minor renaming of function name

79ecddd

abhinavdangeti approved these changes Nov 11, 2022

View reviewed changes

abhinavdangeti merged commit 1643dc3 into blevesearch:master Nov 11, 2022

abhinavdangeti mentioned this pull request Nov 11, 2022

add BytesRead to SearchResult #1725

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Returning bytesRead as part of searchResult #1752

Returning bytesRead as part of searchResult #1752

Thejas-bhat commented Nov 4, 2022 •

edited

Loading

abhinavdangeti left a comment

abhinavdangeti Nov 4, 2022

Thejas-bhat Nov 7, 2022

abhinavdangeti Nov 4, 2022

Thejas-bhat Nov 7, 2022

abhinavdangeti Nov 4, 2022

Thejas-bhat Nov 7, 2022

abhinavdangeti Nov 4, 2022

Thejas-bhat Nov 7, 2022

abhinavdangeti Nov 4, 2022

Thejas-bhat Nov 7, 2022

abhinavdangeti Nov 4, 2022

Thejas-bhat Nov 7, 2022

abhinavdangeti Nov 4, 2022

Thejas-bhat Nov 7, 2022

abhinavdangeti commented Nov 9, 2022

abhinavdangeti left a comment

abhinavdangeti Nov 10, 2022

Thejas-bhat Nov 11, 2022

abhinavdangeti Nov 11, 2022

abhinavdangeti Nov 10, 2022

Thejas-bhat Nov 11, 2022

abhinavdangeti Nov 11, 2022

Returning bytesRead as part of searchResult #1752

Returning bytesRead as part of searchResult #1752

Conversation

Thejas-bhat commented Nov 4, 2022 • edited Loading

abhinavdangeti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinavdangeti commented Nov 9, 2022

abhinavdangeti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thejas-bhat commented Nov 4, 2022 •

edited

Loading