[GitHub] [lucene] fcofdez commented on pull request #11997: Add IntField, LongField, FloatField and DoubleField
fcofdez commented on PR #11997: URL: https://github.com/apache/lucene/pull/11997#issuecomment-1345542724 Thanks all for the reviews! > This sounds good to me. The main issue I see with this is that our default SortFields fail if the field is indexed with multi-valued doc values. But maybe we could make it more lenient and sort on the min value for ascending sorts and max values for descending sorts. I noticed this issue while I was running the sorting tests, maybe we can tackle this in a follow up PR? This one is getting big already, but I'm open to suggestions. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] jpountz commented on pull request #11997: Add IntField, LongField, FloatField and DoubleField
jpountz commented on PR #11997: URL: https://github.com/apache/lucene/pull/11997#issuecomment-1345543447 Absolutely, adding support for multivalued fields to default sort fields would be a big change too. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] stefanvodita commented on pull request #11780: GH#11601: Add ability to compute reader states after refresh
stefanvodita commented on PR #11780: URL: https://github.com/apache/lucene/pull/11780#issuecomment-1345547314 @mikemccand’s feedback made me shift focus from rebuilding ordinal maps on refresh to just making it convenient for the user to do so. `SsdvReaderStatesManager` shows how that might be done. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] gsmiller commented on pull request #11997: Add IntField, LongField, FloatField and DoubleField
gsmiller commented on PR #11997: URL: https://github.com/apache/lucene/pull/11997#issuecomment-1345583546 > maybe we can tackle this in a follow up PR? +1 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] gsmiller opened a new issue, #12009: Find (and fix) places where we treat a long as a double without an explicit cast
gsmiller opened a new issue, #12009: URL: https://github.com/apache/lucene/issues/12009 ### Description I fixed one instance of this I happened to run across (see GH#12008), which appears to have just been a copy/paste issue, but as @rmuir points out (in the same PR), we can use `errorprone` to find all occurrences of this issue by running `gradlew check -Dvalidation.errorprone=true` with this line commented out: https://github.com/apache/lucene/blob/main/gradle/validation/error-prone.gradle#L381 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] gsmiller commented on pull request #12008: Remove unnecessary NaN checks from LongRange#verifyAndEncode
gsmiller commented on PR #12008: URL: https://github.com/apache/lucene/pull/12008#issuecomment-1345585253 @rmuir done: GH#12009. Thanks for the suggestion! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #11997: Add IntField, LongField, FloatField and DoubleField
rmuir commented on PR #11997: URL: https://github.com/apache/lucene/pull/11997#issuecomment-1345588464 There is already `SortedSetSortField` and `SortedNumericSortField` that do this. So I don't understand why it would be a big change? Maybe just add some sugar (e.g. `newSortField` or similar) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] gsmiller merged pull request #12008: Remove unnecessary NaN checks from LongRange#verifyAndEncode
gsmiller merged PR #12008: URL: https://github.com/apache/lucene/pull/12008 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] jpountz commented on pull request #11997: Add IntField, LongField, FloatField and DoubleField
jpountz commented on PR #11997: URL: https://github.com/apache/lucene/pull/11997#issuecomment-1345663912 Good point. I was thinking of making changes to `SortField` but adding factory methods to these Field classes feels like a better approach to me. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #11998: Migrate away from per-segment-per-threadlocals on SegmentReader
rmuir commented on PR #11998: URL: https://github.com/apache/lucene/pull/11998#issuecomment-1345665387 +1 to improve docs -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #11997: Add IntField, LongField, FloatField and DoubleField
rmuir commented on PR #11997: URL: https://github.com/apache/lucene/pull/11997#issuecomment-1345666166 OK, well maybe defer it if we have options. I haven't thought about the `SortField` approach, maybe it could be more natural that way. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] jpountz commented on pull request #11997: Add IntField, LongField, FloatField and DoubleField
jpountz commented on PR #11997: URL: https://github.com/apache/lucene/pull/11997#issuecomment-1345673357 I just found an issue with this PR which is that multi-valued floats and doubles are encoded with `Double#doubleToLongBits` while they should use `NumericUtil#doubleToSortableLong` to present ordering within a document. One way to fix it would be to change the encoding depending on whether the field is multi-valued, but this isn't great as we then also need to pass this flag to factor methods for queries (or sort fields in the future). So maybe we should go with @rmuir's earlier suggestion of only supporting multi-valued fields, so that there is a single encoding in doc values? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] LuXugang commented on pull request #12003: Some minor code cleanup in IndexSortSortedNumericDocValuesRangeQuery
LuXugang commented on PR #12003: URL: https://github.com/apache/lucene/pull/12003#issuecomment-1345785775 Thanks @gsmiller , a new syntactic sugar `record` to me and first time appeard in lucene code. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene] rmuir opened a new pull request, #12010: Enable LongDoubleConversion error-prone check
rmuir opened a new pull request, #12010: URL: https://github.com/apache/lucene/pull/12010 See https://errorprone.info/bugpattern/LongDoubleConversion After seeing #12008 (Calling isNaN on long arguments), looked into this check and it finds a few other things. In general, we don't have many long-double conversions so the check is not particularly annoying or noisy, and as a library it is better not to play it fast and loose with floating point, even though java allows implicit cast of `long` to a `double` It finds two bugs: * BooleanPerceptron classifier was missing a call to `signum`, it passed the result of `Boolean.compareTo` directly to a scoring function, but the exact return values here are not guaranteed (only 0, negative, positive) * TestBytesWritesTrackingDirectoryWrapper was inadvertently using `assertEquals(double, double, double)` to compare two long values: no need to bring floating point into this test. If it fails it will be a lot less confusing. Otherwise I added explicit casts, with the exception of that pesky TestLucenePointsFormat.testEstimatePointCount which needs help around its math in general (I added TODOs, since it ends out getting duplicated about 6 times in the codebase right now due to backwards codecs) Closes #12009 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
