Skip to content

new segments now support 1-hit encoding - #1337

Open
mschoch wants to merge 1 commit into
blevesearch:masterfrom
mschoch:new-segment-with-one-hit-encoding
Open

new segments now support 1-hit encoding#1337
mschoch wants to merge 1 commit into
blevesearch:masterfrom
mschoch:new-segment-with-one-hit-encoding

Conversation

@mschoch

@mschoch mschoch commented Feb 15, 2020

Copy link
Copy Markdown
Contributor

This commit alters the path for new segments, such that
they too should now consider, and when appropriate, use
the 1-hit encoding optimization.

This commit alters the path for new segments, such that
they too should now consider, and when appropriate, use
the 1-hit encoding optimization.
@mschoch

mschoch commented Feb 15, 2020

Copy link
Copy Markdown
Contributor Author

First, this was the quick and dirty, minimal plausible change to get it working. It's quite possible this is wrong with a fuller understanding of what is going on. The question still remains why didn't @steveyen wire this up in the first place?

Also, I put the DONOTMERGE label on this, as we may want to wait and merge this into a v2 or v3 of scorch (I don't see any backwards compat issues, but since we have new versions coming anyway, maybe ship this with that?)

UPDATE: I forgot to mention that all bleve unit tests pass with this change, and the optimization test now does involve 1-hit encodings. Though this too is not a panacea, as now I suspect the test case ONLY actually tests 1-hit encodings... so we just changed which path was tested, not adding one.

@mschoch mschoch left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hmmn

locEncoder.Reset()
lastDocNum = 0
lastFreq = 0
lastNorm = 0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

actually these are either in the wrong place, or not necessary in this verison, as these variables go out of scope here...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant