[jira] Created: (LUCENE-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

classic Classic list List threaded Threaded
44 messages Options
123
Reply | Threaded
Open this post in threaded view
|

[jira] Created: (LUCENE-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

JIRA jira@apache.org
Problem with CharStream and Tokenizers with custom reset(Reader) method
-----------------------------------------------------------------------

                 Key: LUCENE-1906
                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
             Project: Lucene - Java
          Issue Type: Bug
          Components: Analysis
    Affects Versions: 2.9
            Reporter: Uwe Schindler
            Assignee: Uwe Schindler
            Priority: Blocker
             Fix For: 2.9


When reviewing the new CharStream code added to Tokenizers, I found a
serious problem with backwards compatibility and other Tokenizers, that do
not override reset(CharStream).

The problem is, that e.g. CharTokenizer only overrides reset(Reader):

  public void reset(Reader input) throws IOException {
    super.reset(input);
    bufferIndex = 0;
    offset = 0;
    dataLen = 0;
  }

If you reset such a Tokenizer with another CharStream (not a Reader), this
method will never be called and breaking the whole Tokenizer.

As CharStream extends Reader, I propose to remove this reset(CharStream
method) and simply do an instanceof check to detect if the supplied Reader
is no CharStream and wrap it. We could also remove the extra ctor (because
most Tokenizers have no support for passing CharStreams). If the ctor also
checks with instanceof and warps as needed the code is backwards compatible
and we do not need to add additional ctors in subclasses.

As this instanceof check is always done in CharReader.get() why not remove
ctor(CharStream) and reset(CharStream) completely?

Any thoughts?

I would like to fix this somehow before RC4, I'm, sorry :(


--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

JIRA jira@apache.org

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

Uwe Schindler updated LUCENE-1906:
----------------------------------

    Description:
When reviewing the new CharStream code added to Tokenizers, I found a
serious problem with backwards compatibility and other Tokenizers, that do
not override reset(CharStream).

The problem is, that e.g. CharTokenizer only overrides reset(Reader):

{code}
  public void reset(Reader input) throws IOException {
    super.reset(input);
    bufferIndex = 0;
    offset = 0;
    dataLen = 0;
  }
{code}

If you reset such a Tokenizer with another CharStream (not a Reader), this
method will never be called and breaking the whole Tokenizer.

As CharStream extends Reader, I propose to remove this reset(CharStream
method) and simply do an instanceof check to detect if the supplied Reader
is no CharStream and wrap it. We could also remove the extra ctor (because
most Tokenizers have no support for passing CharStreams). If the ctor also
checks with instanceof and warps as needed the code is backwards compatible
and we do not need to add additional ctors in subclasses.

As this instanceof check is always done in CharReader.get() why not remove
ctor(CharStream) and reset(CharStream) completely?

Any thoughts?

I would like to fix this somehow before RC4, I'm, sorry :(


  was:
When reviewing the new CharStream code added to Tokenizers, I found a
serious problem with backwards compatibility and other Tokenizers, that do
not override reset(CharStream).

The problem is, that e.g. CharTokenizer only overrides reset(Reader):

  public void reset(Reader input) throws IOException {
    super.reset(input);
    bufferIndex = 0;
    offset = 0;
    dataLen = 0;
  }

If you reset such a Tokenizer with another CharStream (not a Reader), this
method will never be called and breaking the whole Tokenizer.

As CharStream extends Reader, I propose to remove this reset(CharStream
method) and simply do an instanceof check to detect if the supplied Reader
is no CharStream and wrap it. We could also remove the extra ctor (because
most Tokenizers have no support for passing CharStreams). If the ctor also
checks with instanceof and warps as needed the code is backwards compatible
and we do not need to add additional ctors in subclasses.

As this instanceof check is always done in CharReader.get() why not remove
ctor(CharStream) and reset(CharStream) completely?

Any thoughts?

I would like to fix this somehow before RC4, I'm, sorry :(



> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Uwe Schindler updated LUCENE-1906:
----------------------------------

    Attachment: LUCENE-1906.patch

> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Uwe Schindler commented on LUCENE-1906:
---------------------------------------

I will now check, if the change of the "input" member variable leads to backwards breaks (it was changed from Reader to CharStream)...

> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Yonik Seeley commented on LUCENE-1906:
--------------------------------------

+1, this looks like the best fix.

> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Michael McCandless commented on LUCENE-1906:
--------------------------------------------

+1, good catch Uwe!

> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Mark Miller commented on LUCENE-1906:
-------------------------------------

Ready Uwe?

> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Uwe Schindler commented on LUCENE-1906:
---------------------------------------

bq. I will now check, if the change of the "input" member variable leads to backwards breaks (it was changed from Reader to CharStream)...

We also have a not-in-CHANGES.txt backwards break. We changed Reader to CharStream. When committing this change, also the backwards-branch was modified to use CharStream. I reverted this change (to get the state of a legacy Tokenizer) and wrote a Test, that called input.read() inside the Tokenizer. The test compiled correct in backwards and also run correct.

In trunk, the test-tag failed:
{code}
    [junit] Testcase: testChangeToCharStream29(org.apache.lucene.analysis.TestTokenizer):       Caused an ERROR
    [junit] input
    [junit] java.lang.NoSuchFieldError: input
    [junit]     at org.apache.lucene.analysis.TestTokenizer$1.next(TestTokenizer.java:43)
    [junit]     at org.apache.lucene.analysis.TestTokenizer.testChangeToCharStream29(TestTokenizer.java:58)
    [junit]
    [junit]
    [junit] Test org.apache.lucene.analysis.TestTokenizer FAILED
{code}

So somebody cannot use his old Tokenizers without recompiling (because Java is not able to respect the type change). After recompiling his classes it works.

If we want to do this, we should clearly state this in CHANGES at the backwards-breaks.

> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Uwe Schindler updated LUCENE-1906:
----------------------------------

    Attachment: backwards-break.patch

Here is the patch for backwards-branch, that fails. It reverts the change from Reader to CharStream and adds a dummy Tokenizer in the test that compiles, but does not run in trunk.

> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: backwards-break.patch, LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Uwe Schindler updated LUCENE-1906:
----------------------------------

    Attachment:     (was: backwards-break.patch)

> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Uwe Schindler updated LUCENE-1906:
----------------------------------

    Attachment: backwards-break.patch

Sorry, wrong patch, this one is correct. Other one was a even harder test by assigning a value.

> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: backwards-break.patch, LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Uwe Schindler commented on LUCENE-1906:
---------------------------------------

One possibility to prevent this break would be the following:
- revert the CharStream to a Reader in Tokenizer (restore the old class).
- add a method correctOffset(int) to Tokenizer that does:
{code}
protected final int correctOffset(int offset) {
  if (input instanceof CharStream) return ((CharStream) input).correctOffset(offset);
  return offset;
}
{code}
- Change the usage of correctOffset in all Tokenizers in core/contrib (only a few are affected)
- revert the backwards-branch changes

> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: backwards-break.patch, LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Uwe Schindler edited comment on LUCENE-1906 at 9/10/09 10:15 AM:
-----------------------------------------------------------------

One possibility to prevent this break would be the following:
- revert the CharStream to a Reader in Tokenizer (restore the old class).
- add a method correctOffset(int) to Tokenizer that does:
{code}
protected final int correctOffset(int offset) {
  if (input instanceof CharStream) return ((CharStream) input).correctOffset(offset);
  return offset;
}
{code}
- Change the usage of correctOffset in all Tokenizers in core/contrib (only a few are affected)
- revert the backwards-branch changes

In this case, the input of a Tokenizer stays always a j.io.Reader and the offset correction defaults to just nothing. If a custom Reader extends CharStream, the offset correction is automatically used. This is 100% backwards compatible (only some old Tokenizers not calling correctOffset() before passing to Token/OffsetAttribute would produce invalid offsets if used with CharStreams instead of Readers)

      was (Author: thetaphi):
    One possibility to prevent this break would be the following:
- revert the CharStream to a Reader in Tokenizer (restore the old class).
- add a method correctOffset(int) to Tokenizer that does:
{code}
protected final int correctOffset(int offset) {
  if (input instanceof CharStream) return ((CharStream) input).correctOffset(offset);
  return offset;
}
{code}
- Change the usage of correctOffset in all Tokenizers in core/contrib (only a few are affected)
- revert the backwards-branch changes
 

> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: backwards-break.patch, LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Mark Miller commented on LUCENE-1906:
-------------------------------------

What about using an introspection cache again? Kind of nasty depending on how soon we could get rid of it.

Personally, I think I'd prefer a break to a solution that doesnt solve every case.

> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: backwards-break.patch, LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Robert Muir commented on LUCENE-1906:
-------------------------------------

bq. (only some old Tokenizers not calling correctOffset() before passing to Token/OffsetAttribute would produce invalid offsets if used with CharStreams instead of Readers)

but don't you have this problem already? I like Uwe's approach.

> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: backwards-break.patch, LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Uwe Schindler commented on LUCENE-1906:
---------------------------------------

bq. What about using an introspection cache again? Kind of nasty depending on how soon we could get rid of it.

A cache for what? I do not understand :-) The problem is the change of the member field.

bq. but don't you have this problem already? I like Uwe's approach.

Correct, this is always the problem with this change. The offsets are wrong if one legacy Tokenizer does not correct the offset (either through Tokenizer.correctOffset() or CharStream.correctOffset()).

> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: backwards-break.patch, LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Michael McCandless commented on LUCENE-1906:
--------------------------------------------

bq. We also have a not-in-CHANGES.txt backwards break.

Sigh, I forgot to also add an entry to "Changes in back compat policy"
when we made this change (in LUCENE-1466).

We had decided at the time to make an exception to back-compat, ie
allow reader to become CharStream, but we can definitely revisit
that...

bq. One possibility to prevent this break would be the following

Adding an instanceof check in every correctOffset call makes me
nervous -- what's the performance cost?


> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: backwards-break.patch, LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Robert Muir commented on LUCENE-1906:
-------------------------------------

bq. Correct, this is always the problem with this change. The offsets are wrong if one legacy Tokenizer does not correct the offset (either through Tokenizer.correctOffset() or CharStream.correctOffset()).

right, so your proposed fix would correct this issue (the back compat break), but it doesn't introduce any new issue.

if you want to use CharFilter, then your tokenizer must call correctOffset() so the offsets will be correct.


> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: backwards-break.patch, LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Michael McCandless commented on LUCENE-1906:
--------------------------------------------

bq. Correct, this is always the problem with this change.

Right, we should separately fix this (so that all of our core/contrib tokenizers invoke correctOffset).

> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: backwards-break.patch, LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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-1906) Problem with CharStream and Tokenizers with custom reset(Reader) method

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

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

Mark Miller commented on LUCENE-1906:
-------------------------------------

bq. A cache for what? I do not understand  The problem is the change of the member field.

Sorry - I meant to say reflection. I mean check if the instance just overrides reset(Reader), and if so, call it with the CharReader from reset(Reader) (and cache the reflection results) - perhaps it gets more complicated though - I havn't thought on it - just throwing it out. I've glossed over the code involved but I'm still at a pretty high level with it, as I trust there is enough people that already know what they are doing in this area. I was never the kid in a group racing to have the math answer first ;)

bq. Correct, this is always the problem with this change. The offsets are wrong if one legacy Tokenizer does not correct the offset (either through Tokenizer.correctOffset() or CharStream.correctOffset()).

Okay - I took it as - this is 100% backwards compatible, except for this exception where it is not. But if thats always been the case, it is back compat and it sounds fine to me - just got caught up in the wording.

So if its good, lets ship it! It will take me a bit of time to ensure all the tests and package up, and transfer over again (man does all that maven stuff make transfer take forever - too many little files)

> Problem with CharStream and Tokenizers with custom reset(Reader) method
> -----------------------------------------------------------------------
>
>                 Key: LUCENE-1906
>                 URL: https://issues.apache.org/jira/browse/LUCENE-1906
>             Project: Lucene - Java
>          Issue Type: Bug
>          Components: Analysis
>    Affects Versions: 2.9
>            Reporter: Uwe Schindler
>            Assignee: Uwe Schindler
>            Priority: Blocker
>             Fix For: 2.9
>
>         Attachments: backwards-break.patch, LUCENE-1906.patch
>
>
> When reviewing the new CharStream code added to Tokenizers, I found a
> serious problem with backwards compatibility and other Tokenizers, that do
> not override reset(CharStream).
> The problem is, that e.g. CharTokenizer only overrides reset(Reader):
> {code}
>   public void reset(Reader input) throws IOException {
>     super.reset(input);
>     bufferIndex = 0;
>     offset = 0;
>     dataLen = 0;
>   }
> {code}
> If you reset such a Tokenizer with another CharStream (not a Reader), this
> method will never be called and breaking the whole Tokenizer.
> As CharStream extends Reader, I propose to remove this reset(CharStream
> method) and simply do an instanceof check to detect if the supplied Reader
> is no CharStream and wrap it. We could also remove the extra ctor (because
> most Tokenizers have no support for passing CharStreams). If the ctor also
> checks with instanceof and warps as needed the code is backwards compatible
> and we do not need to add additional ctors in subclasses.
> As this instanceof check is always done in CharReader.get() why not remove
> ctor(CharStream) and reset(CharStream) completely?
> Any thoughts?
> I would like to fix this somehow before RC4, I'm, sorry :(

--
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]

123