You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "jainankitk (via GitHub)" <gi...@apache.org> on 2025/04/10 21:30:19 UTC
[PR] Minor refactoring in ComponentTree [lucene]
jainankitk opened a new pull request, #14474:
URL: https://github.com/apache/lucene/pull/14474
### Description
Minor refactoring of `Comparator` in `ComponentTree`
<!--
If this is your first contribution to Lucene, please make sure you have reviewed the contribution guide.
https://github.com/apache/lucene/blob/main/CONTRIBUTING.md
-->
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Minor refactoring in ComponentTree [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita merged PR #14474:
URL: https://github.com/apache/lucene/pull/14474
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Minor refactoring in ComponentTree [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on code in PR #14474:
URL: https://github.com/apache/lucene/pull/14474#discussion_r2038545565
##########
lucene/core/src/java/org/apache/lucene/geo/ComponentTree.java:
##########
@@ -26,6 +26,13 @@
* <p>Construction takes {@code O(n log n)} time for sorting and tree construction.
*/
final class ComponentTree implements Component2D {
+
+ private static final Comparator<Component2D> XComparator =
Review Comment:
Makes sense!
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Minor refactoring in ComponentTree [lucene]
Posted by "jainankitk (via GitHub)" <gi...@apache.org>.
jainankitk commented on code in PR #14474:
URL: https://github.com/apache/lucene/pull/14474#discussion_r2038470766
##########
lucene/core/src/java/org/apache/lucene/geo/ComponentTree.java:
##########
@@ -322,27 +317,7 @@ private static ComponentTree createTree(
}
final int mid = (low + high) >>> 1;
if (low < high) {
- Comparator<Component2D> comparator;
- if (splitX) {
- comparator =
- (left, right) -> {
- int ret = Double.compare(left.getMinX(), right.getMinX());
- if (ret == 0) {
- ret = Double.compare(left.getMaxX(), right.getMaxX());
- }
- return ret;
- };
- } else {
- comparator =
- (left, right) -> {
- int ret = Double.compare(left.getMinY(), right.getMinY());
- if (ret == 0) {
- ret = Double.compare(left.getMaxY(), right.getMaxY());
- }
- return ret;
- };
- }
- ArrayUtil.select(components, low, high + 1, mid, comparator);
+ ArrayUtil.select(components, low, high + 1, mid, splitX ? XComparator : YComparator);
Review Comment:
I noticed 1/2 places where we have used Ternary operators. But, not something I am too particular about. Added `if-else` back
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Minor refactoring in ComponentTree [lucene]
Posted by "jainankitk (via GitHub)" <gi...@apache.org>.
jainankitk commented on code in PR #14474:
URL: https://github.com/apache/lucene/pull/14474#discussion_r2038461725
##########
lucene/core/src/java/org/apache/lucene/geo/ComponentTree.java:
##########
@@ -92,11 +99,9 @@ public boolean contains(double x, double y) {
}
}
if (right != null
- && ((splitX == false && y >= this.component.getMinY())
+ && ((!splitX && y >= this.component.getMinY())
Review Comment:
Happy to stick with the existing convention!
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Minor refactoring in ComponentTree [lucene]
Posted by "jainankitk (via GitHub)" <gi...@apache.org>.
jainankitk commented on PR #14474:
URL: https://github.com/apache/lucene/pull/14474#issuecomment-2795420056
> Thank you for the prompt response! Could you also add an entry in CHANGES.txt?
Thank you for a prompt review. Added the entry to CHANGES.txt
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Minor refactoring in ComponentTree [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on code in PR #14474:
URL: https://github.com/apache/lucene/pull/14474#discussion_r2038390465
##########
lucene/core/src/java/org/apache/lucene/geo/ComponentTree.java:
##########
@@ -92,11 +99,9 @@ public boolean contains(double x, double y) {
}
}
if (right != null
- && ((splitX == false && y >= this.component.getMinY())
+ && ((!splitX && y >= this.component.getMinY())
Review Comment:
Comparing with `== false` is a Lucene convention that we should maintain.
##########
lucene/core/src/java/org/apache/lucene/geo/ComponentTree.java:
##########
@@ -322,27 +317,7 @@ private static ComponentTree createTree(
}
final int mid = (low + high) >>> 1;
if (low < high) {
- Comparator<Component2D> comparator;
- if (splitX) {
- comparator =
- (left, right) -> {
- int ret = Double.compare(left.getMinX(), right.getMinX());
- if (ret == 0) {
- ret = Double.compare(left.getMaxX(), right.getMaxX());
- }
- return ret;
- };
- } else {
- comparator =
- (left, right) -> {
- int ret = Double.compare(left.getMinY(), right.getMinY());
- if (ret == 0) {
- ret = Double.compare(left.getMaxY(), right.getMaxY());
- }
- return ret;
- };
- }
- ArrayUtil.select(components, low, high + 1, mid, comparator);
+ ArrayUtil.select(components, low, high + 1, mid, splitX ? XComparator : YComparator);
Review Comment:
I think we avoid ternary operators?
##########
lucene/core/src/java/org/apache/lucene/geo/ComponentTree.java:
##########
@@ -26,6 +26,13 @@
* <p>Construction takes {@code O(n log n)} time for sorting and tree construction.
*/
final class ComponentTree implements Component2D {
+
+ private static final Comparator<Component2D> XComparator =
Review Comment:
Is it worth defining these separately if we only use them once?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Minor refactoring in ComponentTree [lucene]
Posted by "jainankitk (via GitHub)" <gi...@apache.org>.
jainankitk commented on code in PR #14474:
URL: https://github.com/apache/lucene/pull/14474#discussion_r2038491526
##########
lucene/core/src/java/org/apache/lucene/geo/ComponentTree.java:
##########
@@ -26,6 +26,13 @@
* <p>Construction takes {@code O(n log n)} time for sorting and tree construction.
*/
final class ComponentTree implements Component2D {
+
+ private static final Comparator<Component2D> XComparator =
Review Comment:
IMO, it is better to reuse the `Comparator` instance for every invocation of `createTree`. Although I feel compiler is probably doing that optimization already, don't see any harm in being more explicit about it.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org