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

new segments now support 1-hit encoding #1337

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mschoch
Copy link
Contributor

@mschoch mschoch commented Feb 15, 2020

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
Copy link
Contributor Author

mschoch commented Feb 15, 2020

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.

Copy link
Contributor Author

@mschoch mschoch left a comment

Choose a reason for hiding this comment

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

hmmn

@@ -714,6 +731,9 @@ func (s *interim) writeDicts() (fdvIndexOffset uint64, dictOffsets []uint64, err

tfEncoder.Reset()
locEncoder.Reset()
lastDocNum = 0
lastFreq = 0
lastNorm = 0
Copy link
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