[jira] [Issue Comment Edited] (LUCENE-3807) Cleanup suggester API

["Dawid Weiss (Issue Comment Edited) (JIRA)" <ji...@apache.org>]

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

Dawid Weiss edited comment on LUCENE-3807 at 2/22/12 9:16 AM:
--------------------------------------------------------------

Sorry for being late, work. I like the patch. Comments:

{noformat}
+    public Comparator<BytesRef> getComparator() {
+      return null;
+    }
{noformat}

This shows up in a number of places. I have mixed feelings about certain types having a comparator and others not having it, but it's minor.

BufferingTermFreqIteratorWrapper is a nuisance (buffers in memory). It would be nicer to have a sort on disk if something doesn't support sorted iteration order.

I also wonder float -> long = 4 -> 8 bytes... would this count as an incompatible API change (because what used to work for a given amount of RAM won't work anymore -- BufferingTermFreqIteratorWrapper again)?

{noformat}
+      if (l1 < l2) {
+        aStop = l1;
+      } else {
+        aStop = l2;
+      }
{noformat}

if I remember correctly Math.min/max are intrinsics, so you can afford to be explicit ;)

Why not a specific covariant here?

{noformat}
-  public Float get(String key) {
+  public Object get(CharSequence key) {
{noformat}

This doesn't seem necessary (lookup accepts a CharSequence?).
{noformat}
@@ -199,7 +199,7 @@ public class LookupBenchmarkTest extends LuceneTestCase {
         public Integer call() throws Exception {
           int v = 0;
           for (String term : input) {
-            v += lookup.lookup(term, onlyMorePopular, num).size();
+            v += lookup.lookup(new CharsRef(term), onlyMorePopular, num).size();
{noformat}

I like the rest, including the CharSequenceish evilness of bytesToCharSequence :)

                
      was (Author: dweiss):
    Sorry for being late, work. I like the patch. Comments:

+    public Comparator<BytesRef> getComparator() {
+      return null;
+    }

This shows up in a number of places. I have mixed feelings about certain types having a comparator and others not having it, but it's minor.

BufferingTermFreqIteratorWrapper is a nuisance (buffers in memory). It would be nicer to have a sort on disk if something doesn't support sorted iteration order.

I also wonder float -> long = 4 -> 8 bytes... would this count as an incompatible API change (because what used to work for a given amount of RAM won't work anymore -- BufferingTermFreqIteratorWrapper again)?

+      if (l1 < l2) {
+        aStop = l1;
+      } else {
+        aStop = l2;
+      }

if I remember correctly Math.min/max are intrinsics, so you can afford to be explicit ;)

Why not a specific covariant here?

-  public Float get(String key) {
+  public Object get(CharSequence key) {

@@ -199,7 +199,7 @@ public class LookupBenchmarkTest extends LuceneTestCase {
         public Integer call() throws Exception {
           int v = 0;
           for (String term : input) {
-            v += lookup.lookup(term, onlyMorePopular, num).size();
+            v += lookup.lookup(new CharsRef(term), onlyMorePopular, num).size();

This doesn't seem necessary (lookup accepts a CharSequence?).

I like the rest, including the CharSequenceish evilness of bytesToCharSequence :)


                  
> Cleanup suggester API
> ---------------------
>
>                 Key: LUCENE-3807
>                 URL: https://issues.apache.org/jira/browse/LUCENE-3807
>             Project: Lucene - Java
>          Issue Type: Improvement
>          Components: modules/other
>    Affects Versions: 3.6, 4.0
>            Reporter: Simon Willnauer
>             Fix For: 4.0
>
>         Attachments: LUCENE-3807.patch, LUCENE-3807.patch, LUCENE-3807.patch
>
>
> Currently the suggester api and especially TermFreqIterator don't play that nice with BytesRef and other paradigms we use in lucene, further the java iterator pattern isn't that useful when it gets to work with TermsEnum, BytesRef etc. We should try to clean up this api step by step moving over to BytesRef including the Lookup class and its interface...

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org