fabriziofortino commented on code in PR #535:
URL: https://github.com/apache/jackrabbit-oak/pull/535#discussion_r861095425
##########
oak-doc/src/site/markdown/query/query-engine.md:
##########
@@ -153,6 +154,26 @@ This is supported for both XPath and SQL-2, as follows:
order by name()
option(traversal ok)
+
+#### Query Option Offset / Limit
+
+`@since Oak 1.44.0 (OAK-9740)`
+
+By setting the offset / limit of a query you can set the limits and offsets
set on the query object.
+Note, this setting will be overriden by any settings made via the
`Query#setOffset` or `Query#setLimit` methods.
Review Comment:
typo
```suggestion
Note, this setting will be overridden by any settings made via the
`Query#setOffset` or `Query#setLimit` methods.
```
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/xpath/Statement.java:
##########
@@ -353,29 +355,23 @@ private static void appendQueryOptions(StringBuilder
buff, QueryOptions queryOpt
return;
}
buff.append(" option(");
- int optionCount = 0;
+ List<String> optionValues = new ArrayList<>();
if (queryOptions.traversal != Traversal.DEFAULT) {
- buff.append("traversal " + queryOptions.traversal);
- optionCount++;
+ optionValues.add("traversal " + queryOptions.traversal);
}
if (queryOptions.indexName != null) {
- if (optionCount > 0) {
- buff.append(", ");
- }
- buff.append("index name [");
- buff.append(queryOptions.indexName);
- buff.append("]");
- optionCount++;
+ optionValues.add("index name [" + queryOptions.indexName + "]");
}
if (queryOptions.indexTag != null) {
- if (optionCount > 0) {
- buff.append(", ");
- }
- buff.append("index tag [");
- buff.append(queryOptions.indexTag);
- buff.append("]");
- optionCount++;
+ optionValues.add("index tag [" + queryOptions.indexTag + "]");
+ }
+ if (queryOptions.offset != -1) {
+ optionValues.add("offset " + queryOptions.offset);
+ }
+ if (queryOptions.limit != -1) {
+ optionValues.add("limit " + queryOptions.limit);
}
+ buff.append(optionValues.stream().collect(Collectors.joining(", ")));
Review Comment:
this can be simplified with
```suggestion
buff.append(String.join(", ", optionValues));
```
##########
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:
Some people suggest NOT using `Optional` as type parameters but only as
return types. There are a lot of threads around this topic (see
https://medium.com/@yassinhajaj/optionals-are-bad-practices-still-bad-practices-if-everyone-practices-them-6ec5a66c30aa).
IMHO it depends on the case. In this case, though, I would be in favor of
removing it. The main reason is that `Optional` and `Long` are two wrappers.
This double wrapping is hard to read and suboptimal for the compiler.
@thomasmueller I would like to hear your opinion on this
##########
oak-core/src/main/java/org/apache/jackrabbit/oak/query/QueryEngineImpl.java:
##########
@@ -292,7 +290,39 @@ public Result executeQuery(
}
}
}
-
+
+ @Override
+ public Result executeQuery(
+ String statement, String language, long limit, long offset,
+ Map<String, ? extends PropertyValue> bindings,
+ Map<String, String> mappings) throws ParseException {
+ return executeQuery(statement, language, Optional.of(limit),
Optional.of(offset), bindings, mappings);
+
+ }
+
+ private static long getOffset(List<Query> queries, Optional<Long>
passedOffset) {
+ if (!passedOffset.isPresent()) {
+ return
queries.stream().map(Query::getOffset).filter(Optional::isPresent).map(Optional::get).findFirst()
+ .orElse(0L);
+ }
+ if (passedOffset.get() < 0) {
+ throw new IllegalArgumentException("Offset may not be negative,
is: " + passedOffset.get());
+ }
+ return passedOffset.get();
+ }
+
+ private static long getLimit(List<Query> queries, Optional<Long>
passedLimit) {
+ if (!passedLimit.isPresent()) {
+ return
queries.stream().map(Query::getLimit).filter(Optional::isPresent).map(Optional::get).findFirst()
+ .orElse(Long.MAX_VALUE);
+ }
+ long limit = passedLimit.get();
+ if (limit < 0) {
+ throw new IllegalArgumentException("Limit may not be negative, is:
" + passedLimit.get());
+ }
+ return limit;
+ }
+
Review Comment:
these two functions do pretty much the same. They can be replaced by:
```java
private static long getValue(List<Query> queries, Optional<Long> value,
Function<Query, Optional<Long>> getter, Long defaultValue) {
if (!value.isPresent()) {
return
queries.stream().map(getter::apply).filter(Optional::isPresent).map(Optional::get).findFirst()
.orElse(defaultValue);
}
if (value.get() < 0) {
throw new IllegalArgumentException("Value may not be negative,
is: " + value.get());
}
return value.get();
}
```
--
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]
