lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dawid Weiss (Issue Comment Edited) (JIRA)" <j...@apache.org>
Subject [jira] [Issue Comment Edited] (LUCENE-3807) Cleanup suggester API
Date Wed, 22 Feb 2012 09:16:49 GMT

    [ 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


Mime
View raw message