thomasmueller commented on code in PR #535:
URL: https://github.com/apache/jackrabbit-oak/pull/535#discussion_r866922645
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryImpl.java:
##########
@@ -1102,13 +1113,16 @@ private SelectorExecutionPlan
getBestSelectorExecutionPlan(
IndexPlan indexPlan = null;
if (index instanceof AdvancedQueryIndex) {
AdvancedQueryIndex advIndex = (AdvancedQueryIndex) index;
- long maxEntryCount = limit;
- if (offset > 0) {
- if (offset + limit < 0) {
+
+ long localLimit = limit.orElse(Long.MAX_VALUE);
+ long localOffset = offset.orElse(0L);
+ long maxEntryCount = localLimit;
+ if (localOffset > 0) {
+ if (localOffset + localLimit < 0) {
Review Comment:
This is not really related to your patch...
Yes it works, Long.MAX_VALUE + Long.MAX_VALUE == -2.
We could also use QueryImpl.saturatedAdd, which is slower, as it's using
BigInteger.
I wonder, should we move the the logic outside of the loop?
##########
oak-api/src/main/java/org/apache/jackrabbit/oak/api/QueryEngine.java:
##########
@@ -91,6 +92,24 @@ Result executeQuery(
String statement, String language, long limit, long offset,
Map<String, ? extends PropertyValue> bindings,
Map<String, String> mappings) throws ParseException;
+
+ /**
+ * Execute a query and get the result.
+ *
+ * @param statement the query statement
+ * @param language the language
+ * @param limit the maximum result set size (may not be negative but may
be empty)
+ * @param offset the number of rows to skip (may not be negative but may
be empty)
+ * @param bindings the bind variable value bindings
+ * @param mappings namespace prefix mappings
+ * @return the result
+ * @throws ParseException if the statement could not be parsed
+ * @throws IllegalArgumentException if there was an error executing the
query
+ */
+ Result executeQuery(
+ String statement, String language, Optional<Long> limit,
Optional<Long> offset,
Review Comment:
I agree with you @adamcin , but I think this case here doesn't warrant this
yet. Another solution would be to have a class "LimitOffset" with a builder
(methods "limit" and "offset"), but I think the number of callers of this
method, and the number of parameters so far, is so low that it's not needed yet.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
