[jira] Created: (LUCENE-1919) Analysis back compat break

classic Classic list List threaded Threaded
37 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[jira] Created: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
Analysis back compat break
--------------------------

                 Key: LUCENE-1919
                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
             Project: Lucene - Java
          Issue Type: Bug
            Reporter: Yonik Seeley
             Fix For: 2.9


Old and new style token streams don't mix well.


--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org

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

Yonik Seeley commented on LUCENE-1919:
--------------------------------------

Background:
http://search.lucidimagination.com/search/document/4b2b4210e2516769/analysis_back_compat_break
http://search.lucidimagination.com/search/document/26c044ecbce3ed29

> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>             Fix For: 2.9
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Mark Miller commented on LUCENE-1919:
-------------------------------------

At the worst, we can just clone the delegate and not be reusable (the javadoc says you don't have to be reusable)

Not ideal, but it will fix, and cease to be a problem in 3.0 :)

> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>             Fix For: 2.9
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Mark Miller updated LUCENE-1919:
--------------------------------

    Attachment: LUCENE-1919.patch

Now give us some better options Uwe :)

> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>             Fix For: 2.9
>
>         Attachments: LUCENE-1919.patch
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Issue Comment Edited: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Mark Miller edited comment on LUCENE-1919 at 9/17/09 5:41 PM:
--------------------------------------------------------------

At the worst, we can just clone the delegate and not be reusable (the javadoc says you don't have to be reusable)

Not ideal, but it will fix, and cease to be a (possible performance) problem in 3.0 :)

      was (Author: [hidden email]):
    At the worst, we can just clone the delegate and not be reusable (the javadoc says you don't have to be reusable)

Not ideal, but it will fix, and cease to be a problem in 3.0 :)
 

> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>             Fix For: 2.9
>
>         Attachments: LUCENE-1919.patch
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Robert Muir updated LUCENE-1919:
--------------------------------

    Attachment: LUCENE-1919.patch

alternative patch, should not change performance.

> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>             Fix For: 2.9
>
>         Attachments: LUCENE-1919.patch, LUCENE-1919.patch
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Robert Muir updated LUCENE-1919:
--------------------------------

    Attachment: LUCENE-1919.patch

better patch with testcase for the issue.

really, its just that in next() tokenwrapper must be cloned before calling incrementToken, instead of after.

> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>             Fix For: 2.9
>
>         Attachments: LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Yonik Seeley commented on LUCENE-1919:
--------------------------------------

Robert, you would need to handle the incrementToken() case too in next() - that's actually where the bug occured in the Solr test.

{code}
    if (supportedMethods.hasIncrementToken) {
      tokenWrapper.delegate = new Token();
      return incrementToken() ? ((Token) tokenWrapper.delegate.clone()) : null;
{code}

Could we remove the clone()?  not sure...

> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>             Fix For: 2.9
>
>         Attachments: LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Issue Comment Edited: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Yonik Seeley edited comment on LUCENE-1919 at 9/17/09 6:38 PM:
---------------------------------------------------------------

edit: collision w/ robert.
Still wonder if it's safe to get rid of that second clone()... the combinations are mind-bending.

      was (Author: [hidden email]):
    Robert, you would need to handle the incrementToken() case too in next() - that's actually where the bug occured in the Solr test.

{code}
    if (supportedMethods.hasIncrementToken) {
      tokenWrapper.delegate = new Token();
      return incrementToken() ? ((Token) tokenWrapper.delegate.clone()) : null;
{code}

Could we remove the clone()?  not sure...
 

> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>             Fix For: 2.9
>
>         Attachments: LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Mark Miller commented on LUCENE-1919:
-------------------------------------

Nice - thanks Robert!

> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>             Fix For: 2.9
>
>         Attachments: LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Jason Rutherglen commented on LUCENE-1919:
------------------------------------------

With SOLR-908 CommonGramsQueryFilter which uses the old API,
we've been seeing since we upgraded to Solr 1.4/Lucene 2.9,
random negations to query clauses. It almost looks like there's
some sort of shared state or multithreading issue, however I've
also thought somehow it's related to mixing the old and new
APIs. Unfortunately it's so inconsistent I don't have a test
case that reproduces it (happens in production only).

Is there any sort of shared state in the analyzing, possibly
between instances that is fixed in this patch?

> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>             Fix For: 2.9
>
>         Attachments: LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Robert Muir commented on LUCENE-1919:
-------------------------------------

{quote}
edit: collision w/ robert.
Still wonder if it's safe to get rid of that second clone()... the combinations are mind-bending.
{quote}

yonik, hmm i think the second clone() is a hint there remains another problem
if you look at my patch, it only fixes the case where you have a tokenstream supporting incrementToken(), and you use both next() and next(Token) apis.

what if the tokenstream only supports next(reusableTS) ?
if you call next(token) then next(), i think in that case you will have the same problem.
this still won't introduce any extra cloning, just fix the logic so it doesnt overwrite the tokenWrapper, and returns a "full private copy" like the javadocs say.

 (i'll add another test and upload a new patch)

> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>             Fix For: 2.9
>
>         Attachments: LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Robert Muir commented on LUCENE-1919:
-------------------------------------

bq. what if the tokenstream only supports next(reusableTS) ?

ok i tested the 2nd scenario and it is ok.
if you want my additional tests, i can add them, but the existing patch is fine... i confused myself worrying about this 2nd case.
in this case when the consumer calls next(reusableTS), no delegate is involved since its overridden... duh :)



> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>             Fix For: 2.9
>
>         Attachments: LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Robert Muir commented on LUCENE-1919:
-------------------------------------

bq. Still wonder if it's safe to get rid of that second clone()... the combinations are mind-bending.

its not safe to do this for the case of tokenstream that only supports next(reusableTS) but not incrementToken.
otherwise, next() does not return a full copy, but a reference to the delegate, which will be overwritten by future calls to next().
if you want to get rid of it, then you need to clone the delegate before deferring to next(Token).


> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>             Fix For: 2.9
>
>         Attachments: LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Robert Muir commented on LUCENE-1919:
-------------------------------------

{quote}
Is there any sort of shared state in the analyzing, possibly
between instances that is fixed in this patch?
{quote}

Yes. if for instance you call foo = ts.next(reusableToken), then call bar = ts.next()
foo will be overwritten by bar in the second call.
this is because it is incorrectly "reused" in next()... see the testcase i attached for an example.


> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>             Fix For: 2.9
>
>         Attachments: LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Uwe Schindler updated LUCENE-1919:
----------------------------------

    Attachment: LUCENE-1919.patch

Here is my solution for the problem. Michael and me never thought about mixing the old and very old API, which seems to be used. The problem is now, that the behaviour changed for calling next(). The original 2.4.1 code looks the following:

{code}
public Token next() throws IOException {
  final Token reusableToken = new Token();
  Token nextToken = next(reusableToken);

  if (nextToken != null) {
    Payload p = nextToken.getPayload();
    if (p != null) {
      nextToken.setPayload((Payload) p.clone());
    }
  }

  return nextToken;
}
{code}

The difference is, that a new token is created *before* the call to the reusable next() methods (incrementToken() is somehow reuseable, too).

The attached patch, restores exactly this functionality, but also for incrementToken(). It also removes an unneeded assignment to the deleget in the case of next(Token) delegating to next() (which is also a bug, because it makes the token no longer private for the caller - it can be overridden by a later call to incrementToken()).

Tokens generated by next() must be always private and not shared or reused as reusableToken for later next/incrementToken calls by the API. This is the problem of Robert's patch. The full private token (which was cloned before) is then also available as delegate for later calls to incrementToken() (for next(Token) the delegate is replaced, as Robert noticed, so no problem here).

The wrapper around incrementToken() does the following:
- save the current delegate
- replace delegate by a new Token (just like the old next() in 2.4.1)
- call incrementToken and assign to nextToken variable
- restore the reusable delegate
- after that all goes like for next(Token)

Simply said: the next() wrapper is completely decoupled and always uses a completely private (new) Token instance.

I am not sure, why this payload cloning code is in 2.4.1, but I moved it here, too. I think it is because of some old bug, where a payload was assigned in next(Token), that was also shared by the TokenStream itsself between more than one tokens. Using this code, the Token is for sure full private (and even not reused later as before).

Using this patch, you should now even be able to mix all three APIs in one filter/consumer - but I still would'nt do this :-)

> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>             Fix For: 2.9
>
>         Attachments: LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Assigned: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Uwe Schindler reassigned LUCENE-1919:
-------------------------------------

    Assignee: Uwe Schindler

> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>            Assignee: Uwe Schindler
>             Fix For: 2.9
>
>         Attachments: LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Issue Comment Edited: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Uwe Schindler edited comment on LUCENE-1919 at 9/18/09 12:01 AM:
-----------------------------------------------------------------

Good morning all together! What a nice day and then this problem :-)

Here is my solution for the problem: Michael and me never thought about mixing the old and very old API as consumer, which seems to be used. The problem is now, that the behaviour changed for calling next(). The original 2.4.1 code looks the following:

{code}
public Token next() throws IOException {
  final Token reusableToken = new Token();
  Token nextToken = next(reusableToken);

  if (nextToken != null) {
    Payload p = nextToken.getPayload();
    if (p != null) {
      nextToken.setPayload((Payload) p.clone());
    }
  }

  return nextToken;
}
{code}

The difference is, that a new token is created *before* the call to the reusable next() methods (incrementToken() is somehow reuseable, too).

The attached patch, restores exactly this functionality, but also for incrementToken(). It also removes an unneeded assignment to the delegate in the case of next(Token) delegating to next() (which is also a bug, because it makes the token no longer private for the caller - it can be overridden by a later call to incrementToken()) - sorry for that.

Tokens generated by next() must be always private and not shared or reused as reusableToken for later next/incrementToken calls by the API. This is the problem of Robert's patch. The full private token (which was cloned before) is then also available as delegate for later calls to incrementToken() (for next(Token) the delegate is replaced, as Robert noticed, so no problem here).

The wrapper around incrementToken() does the following:
- save the current delegate
- replace delegate by a new Token (just like the old next() in 2.4.1)
- call incrementToken and assign to nextToken variable
- restore the reusable delegate
- after that all goes like for next(Token)

Simply said: the next() wrapper is completely decoupled and always uses a completely private (new) Token instance.

I am not sure, why this payload cloning code is in 2.4.1, but I moved it here, too. I think it is because of some old bug, where a payload was assigned in next(Token), that was also shared by the TokenStream itsself between more than one tokens. Using this code, the Token is for sure full private (and even not reused later as before).

Using this patch, you should now even be able to mix all three APIs in one filter/consumer - but I still would'nt do this :-)

      was (Author: thetaphi):
    Here is my solution for the problem. Michael and me never thought about mixing the old and very old API, which seems to be used. The problem is now, that the behaviour changed for calling next(). The original 2.4.1 code looks the following:

{code}
public Token next() throws IOException {
  final Token reusableToken = new Token();
  Token nextToken = next(reusableToken);

  if (nextToken != null) {
    Payload p = nextToken.getPayload();
    if (p != null) {
      nextToken.setPayload((Payload) p.clone());
    }
  }

  return nextToken;
}
{code}

The difference is, that a new token is created *before* the call to the reusable next() methods (incrementToken() is somehow reuseable, too).

The attached patch, restores exactly this functionality, but also for incrementToken(). It also removes an unneeded assignment to the deleget in the case of next(Token) delegating to next() (which is also a bug, because it makes the token no longer private for the caller - it can be overridden by a later call to incrementToken()).

Tokens generated by next() must be always private and not shared or reused as reusableToken for later next/incrementToken calls by the API. This is the problem of Robert's patch. The full private token (which was cloned before) is then also available as delegate for later calls to incrementToken() (for next(Token) the delegate is replaced, as Robert noticed, so no problem here).

The wrapper around incrementToken() does the following:
- save the current delegate
- replace delegate by a new Token (just like the old next() in 2.4.1)
- call incrementToken and assign to nextToken variable
- restore the reusable delegate
- after that all goes like for next(Token)

Simply said: the next() wrapper is completely decoupled and always uses a completely private (new) Token instance.

I am not sure, why this payload cloning code is in 2.4.1, but I moved it here, too. I think it is because of some old bug, where a payload was assigned in next(Token), that was also shared by the TokenStream itsself between more than one tokens. Using this code, the Token is for sure full private (and even not reused later as before).

Using this patch, you should now even be able to mix all three APIs in one filter/consumer - but I still would'nt do this :-)
 

> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>            Assignee: Uwe Schindler
>             Fix For: 2.9
>
>         Attachments: LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Uwe Schindler updated LUCENE-1919:
----------------------------------

    Attachment: LUCENE-1919.patch

This is just a patch that enhances/rectifies the BW testcase. It corrects the messages in assertTrue, because they should refelct that something is going wrong (just cosmetics). But it adds a test for correctness of other token's contents to these POSToken tests, not only if the one is a proper noun.

It also mixes consuming the new API into Robert's test and the call to next(Token), to check if the full-private Token returned from next() is still valid.

Nothing special, no other changes in core code.

> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>            Assignee: Uwe Schindler
>             Fix For: 2.9
>
>         Attachments: LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (LUCENE-1919) Analysis back compat break

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

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

Uwe Schindler commented on LUCENE-1919:
---------------------------------------

I would suggest to create a new RC because of this issue. The TokenStream BW stuff is very tricky and is not limited to only one TokenStream. When fixing thigs here, you must always also look into issues that could arise by mixing old/new API. The current issue is a typical example for that. Your brain is always "fuming", when you think about what happens if TF1 calls TF2 using old API, but TF2 is new API and calls TF3 using new API. TF3 itsself is again very old API without reuse and so on.

But this one is new, a reusable TF is calling another TS mixing the APIs, but the changes also affect the other variants. So testing, testing, testing and take a cold shower when your brain starts getting hot :-)

I will commit the current patch later in the afternoon, when you are awake at the west coast.

> Analysis back compat break
> --------------------------
>
>                 Key: LUCENE-1919
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1919
>             Project: Lucene - Java
>          Issue Type: Bug
>            Reporter: Yonik Seeley
>            Assignee: Uwe Schindler
>             Fix For: 2.9
>
>         Attachments: LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch, LUCENE-1919.patch
>
>
> Old and new style token streams don't mix well.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

12