[jira] [Created] (LUCENE-3526) preflex codec returns wrong terms if you use an empty field name

[ https://issues.apache.org/jira/browse/LUCENE-3526?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Robert Muir updated LUCENE-3526:
--------------------------------

Attachment: LUCENE-3526_test.patch

test, fails under preflex codec

[ https://issues.apache.org/jira/browse/LUCENE-3526?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13133810#comment-13133810 ]

Robert Muir commented on LUCENE-3526:
-------------------------------------

I committed this test to 3.x, it passes there.

[ https://issues.apache.org/jira/browse/LUCENE-3526?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Robert Muir updated LUCENE-3526:
--------------------------------

Attachment: LUCENE-3526_test.patch

same test but with out-of-bounds checks for docFreq and totalTermFreq in checkIndex.

These would have found the bug, too (i committed already to 3.x's checkIndex)

[ https://issues.apache.org/jira/browse/LUCENE-3526?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Robert Muir updated LUCENE-3526:
--------------------------------

Attachment: LUCENE-3526_test.patch

Updated set of tests, I changed TestRegexpRandom2 to sometimes use an empty field name for better testing.

this seems to trigger its own problems:
{noformat}
[junit] Testcase: testRegexps(org.apache.lucene.search.TestRegexpRandom2): FAILED
[junit] Terms are out of order: field= (number 0) lastField= (number -1) text= lastText=
[junit] junit.framework.AssertionFailedError: Terms are out of order: field= (number 0) lastField= (number -1) text= lastText=
[junit] at org.apache.lucene.util.LuceneTestCaseRunner.runChild(LuceneTestCaseRunner.java:149)
[junit] at org.apache.lucene.util.LuceneTestCaseRunner.runChild(LuceneTestCaseRunner.java:51)
[junit] at org.apache.lucene.index.codecs.preflexrw.TermInfosWriter.add(TermInfosWriter.java:213)
[junit] at org.apache.lucene.index.codecs.preflexrw.PreFlexFieldsWriter$PreFlexTermsWriter.finishTerm(PreFlexFieldsWriter.java:192)
[junit] at org.apache.lucene.index.FreqProxTermsWriterPerField.flush(FreqProxTermsWriterPerField.java:409)
[junit] at org.apache.lucene.index.FreqProxTermsWriter.flush(FreqProxTermsWriter.java:92)
{noformat}

I had thought to workaround this original issue with this hack-patch, but i still get that fail... perhaps its a bad assert/something unrelated?
{noformat}
Index: src/java/org/apache/lucene/index/codecs/preflex/PreFlexFields.java
===================================================================
--- src/java/org/apache/lucene/index/codecs/preflex/PreFlexFields.java (revision 1188010)
+++ src/java/org/apache/lucene/index/codecs/preflex/PreFlexFields.java (working copy)
@@ -711,7 +711,12 @@
} else {
getTermsDict().seekEnum(termEnum, term, true);
}
- skipNext = true;
+ if (internedFieldName == "") {
+ // hackedy-hack: we aren't actually positioned yet
+ skipNext = false;
+ } else {
+ skipNext = true;
+ }

unicodeSortOrder = sortTermsByUnicode();
{noformat}

[ https://issues.apache.org/jira/browse/LUCENE-3526?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Robert Muir updated LUCENE-3526:
--------------------------------

Attachment: LUCENE-3526_test.patch

ok, here's a patch... all tests pass now.

The assert fail in the writer was a bad assert, we previously had:
{noformat}
// If there is a field named "" (empty string) then we
// will get 0 on this comparison, yet, it's "OK". But
// it's not OK if two different field numbers map to
// the same name.
if (cmp != 0 || lastFieldNumber != -1)
return cmp;
{noformat}

which is nice, but it doesn't cover the case of empty term PLUS empty string: Term("", ""). in this case we would fall thru and return 0, which is wrong.

[ https://issues.apache.org/jira/browse/LUCENE-3526?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Robert Muir updated LUCENE-3526:
--------------------------------

Attachment: LUCENE-3526.patch

oops, wrong patch. here is the correct one

[ https://issues.apache.org/jira/browse/LUCENE-3526?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134071#comment-13134071 ]

Robert Muir commented on LUCENE-3526:
-------------------------------------

I will add an additional test to 3.x for Term("", "") and see if it has any bad asserts like this, and add it to the patch.

[ https://issues.apache.org/jira/browse/LUCENE-3526?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134083#comment-13134083 ]

Robert Muir commented on LUCENE-3526:
-------------------------------------

There are more serious problems in 3.x here.

* if you create new Field("", ""), you get IllegalArgumentException from Field's ctor: "name and value cannot both be empty"
* But there are tons of other ways to index an empty term for the empty field (for example initially make it "garbage" then .setValue(""), or via tokenstream).
* If you do this, and you have assertions enabled, you will trip the same assert bug i fixed in trunk here.
* If you don't have assertions enabled, you will create a corrupt index: test: terms, freq, prox...ERROR [term : docFreq=1 != num docs seen 0 + num docs deleted 0]

So we need to figure out what the semantics should be for 3.x. is Term("", "") allowed or not?

[ https://issues.apache.org/jira/browse/LUCENE-3526?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134205#comment-13134205 ]

Michael McCandless commented on LUCENE-3526:
--------------------------------------------

I think the hack is actually correct, but maybe change it to check termEnum.position >= 0?

So this was a case we missed from LUCENE-3183 (maybe there are more!?), where we decided for the corner case of empty field and term text, the caller must handle that the returned enum is unpositioned (in exchange for not adding an if per next).

And maybe add the same comment about LUCENE-3183 on top of that logic?

[ https://issues.apache.org/jira/browse/LUCENE-3526?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Michael McCandless updated LUCENE-3526:
---------------------------------------

Attachment: LUCENE-3526.patch

Patch, putting back the safer-but-if-per-scan from LUCENE-3183; this fixed another test failure.

[ https://issues.apache.org/jira/browse/LUCENE-3526?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134211#comment-13134211 ]

Robert Muir commented on LUCENE-3526:
-------------------------------------

+1, i'm running the tests a lot, this seems solid.

[ https://issues.apache.org/jira/browse/LUCENE-3526?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134238#comment-13134238 ]

Robert Muir commented on LUCENE-3526:
-------------------------------------

I committed this, thanks Mike!

Now to figure out wtf to do for 3.x...

[ https://issues.apache.org/jira/browse/LUCENE-3526?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13134278#comment-13134278 ]

Robert Muir commented on LUCENE-3526:
-------------------------------------

I'm gonna close this issue and open a separate issue for Term("", "") on 3.x...

[ https://issues.apache.org/jira/browse/LUCENE-3526?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Robert Muir resolved LUCENE-3526.
---------------------------------

Resolution: Fixed
Fix Version/s: 4.0