You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/06/24 18:18:48 UTC
[GitHub] [solr] thelabdude opened a new pull request #191: SOLR-15489: Implement OFFSET & FETCH for LIMIT queries
thelabdude opened a new pull request #191:
URL: https://github.com/apache/solr/pull/191
https://issues.apache.org/jira/browse/SOLR-15489
# Description
Add support for OFFSET & FETCH for Parallel SQL queries; only supports limit queries!
```
SELECT id FROM $ALIAS ORDER BY id DESC OFFSET 5 FETCH NEXT 5 ROWS ONLY
```
# Solution
Enhanced the LimitStream to support an offset. If offset is provided, then we wrap the CloudSolrStream with a SortStream and then use the LimitStream with offset.
# Tests
Added unit test to verify the correct rows returned based on offset / limit used.
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] thelabdude commented on a change in pull request #191: SOLR-15489: Implement OFFSET & FETCH for LIMIT queries
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #191:
URL: https://github.com/apache/solr/pull/191#discussion_r660562145
##########
File path: solr/core/src/test/org/apache/solr/handler/TestSQLHandler.java
##########
@@ -1853,21 +1853,18 @@ public void testParallelTimeSeriesGrouping() throws Exception {
}
protected List<Tuple> getTuples(final SolrParams params, String baseUrl) throws IOException {
- //log.info("Tuples from params: {}", params);
- TupleStream tupleStream = new SolrStream(baseUrl, params);
-
- tupleStream.open();
- List<Tuple> tuples = new ArrayList<>();
- for (; ; ) {
- Tuple t = tupleStream.read();
- //log.info(" ... {}", t.fields);
- if (t.EOF) {
- break;
- } else {
- tuples.add(t);
+ List<Tuple> tuples = new LinkedList<>();
+ try (TupleStream tupleStream = new SolrStream(baseUrl, params)) {
Review comment:
Maybe? Here I'm fixing a bug where the stream didn't get closed if an exception gets raised, causing tests to hang.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] thelabdude merged pull request #191: SOLR-15489: Implement OFFSET & FETCH for LIMIT queries
Posted by GitBox <gi...@apache.org>.
thelabdude merged pull request #191:
URL: https://github.com/apache/solr/pull/191
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] thelabdude commented on a change in pull request #191: SOLR-15489: Implement OFFSET & FETCH for LIMIT queries
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #191:
URL: https://github.com/apache/solr/pull/191#discussion_r658265207
##########
File path: solr/core/src/java/org/apache/solr/handler/sql/SolrTable.java
##########
@@ -275,25 +277,43 @@ private TupleStream handleSelect(String zk,
String fl = getFields(fields);
- if(orders.size() > 0) {
+ if (!orders.isEmpty()) {
params.add(SORT, getSort(orders));
} else {
- if(limit == null) {
+ if (limit == null) {
params.add(SORT, "_version_ desc");
fl = fl+",_version_";
} else {
params.add(SORT, "score desc");
- if(fl.indexOf("score") == -1) {
+ if (!fl.contains("score")) {
fl = fl + ",score";
}
}
}
params.add(CommonParams.FL, fl);
-
+
+ if (offset != null && limit == null) {
+ throw new IOException("OFFSET without LIMIT not supported by Solr! Specify desired limit using 'FETCH NEXT <LIMIT> ROWS ONLY'");
+ }
+
if (limit != null) {
- params.add(CommonParams.ROWS, limit);
- return new LimitStream(new CloudSolrStream(zk, collection, params), Integer.parseInt(limit));
+ int limitInt = Integer.parseInt(limit);
+ // if there's an offset, then we need to fetch offset + limit rows from each shard and then sort accordingly
+ LimitStream limitStream;
+ if (offset != null) {
+ int offsetInt = Integer.parseInt(offset);
+ int rows = limitInt + offsetInt;
+ params.add(CommonParams.START, "0"); // tricky ... we need all rows up to limit + offset
Review comment:
How would you map a cursor mark to SQL? Not sure why this makes you nervous as it's not exposing any more risk than people doing:
```
q=*:*&start=1000000000000&rows=10
```
Solr's query interface allows all kinds of dangerous input if people want to mis-use the parameters.
If you have an idea on how to map cursorMark to SQL I'd appreciate hearing the details.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #191: SOLR-15489: Implement OFFSET & FETCH for LIMIT queries
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #191:
URL: https://github.com/apache/solr/pull/191#discussion_r660288462
##########
File path: solr/core/src/test/org/apache/solr/handler/TestSQLHandler.java
##########
@@ -1853,21 +1853,18 @@ public void testParallelTimeSeriesGrouping() throws Exception {
}
protected List<Tuple> getTuples(final SolrParams params, String baseUrl) throws IOException {
- //log.info("Tuples from params: {}", params);
- TupleStream tupleStream = new SolrStream(baseUrl, params);
-
- tupleStream.open();
- List<Tuple> tuples = new ArrayList<>();
- for (; ; ) {
- Tuple t = tupleStream.read();
- //log.info(" ... {}", t.fields);
- if (t.EOF) {
- break;
- } else {
- tuples.add(t);
+ List<Tuple> tuples = new LinkedList<>();
+ try (TupleStream tupleStream = new SolrStream(baseUrl, params)) {
Review comment:
This pattern makes me wish we had an `asStream` method on TupleStream that returned a java Stream. Maybe a future issue.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #191: SOLR-15489: Implement OFFSET & FETCH for LIMIT queries
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #191:
URL: https://github.com/apache/solr/pull/191#discussion_r658259169
##########
File path: solr/core/src/java/org/apache/solr/handler/sql/SolrTable.java
##########
@@ -275,25 +277,43 @@ private TupleStream handleSelect(String zk,
String fl = getFields(fields);
- if(orders.size() > 0) {
+ if (!orders.isEmpty()) {
params.add(SORT, getSort(orders));
} else {
- if(limit == null) {
+ if (limit == null) {
params.add(SORT, "_version_ desc");
fl = fl+",_version_";
} else {
params.add(SORT, "score desc");
- if(fl.indexOf("score") == -1) {
+ if (!fl.contains("score")) {
fl = fl + ",score";
}
}
}
params.add(CommonParams.FL, fl);
-
+
+ if (offset != null && limit == null) {
+ throw new IOException("OFFSET without LIMIT not supported by Solr! Specify desired limit using 'FETCH NEXT <LIMIT> ROWS ONLY'");
+ }
+
if (limit != null) {
- params.add(CommonParams.ROWS, limit);
- return new LimitStream(new CloudSolrStream(zk, collection, params), Integer.parseInt(limit));
+ int limitInt = Integer.parseInt(limit);
+ // if there's an offset, then we need to fetch offset + limit rows from each shard and then sort accordingly
+ LimitStream limitStream;
+ if (offset != null) {
+ int offsetInt = Integer.parseInt(offset);
+ int rows = limitInt + offsetInt;
+ params.add(CommonParams.START, "0"); // tricky ... we need all rows up to limit + offset
Review comment:
This makes me nervous... at what point do we need to get back into cursor mark world?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] thelabdude commented on pull request #191: SOLR-15489: Implement OFFSET & FETCH for LIMIT queries
Posted by GitBox <gi...@apache.org>.
thelabdude commented on pull request #191:
URL: https://github.com/apache/solr/pull/191#issuecomment-870961665
> I think it is possible to use CursorMark in some form for this.
>
> Given that pagination is semi accurate (and generally not very valuable :)), I think that leaves some attack path opens.
>
> The CursorMark is really just last values for the sort to be used to indicate the starting offset.
>
> So you could come up with some caching, loose effort type approaches I think.
>
> For deep paging, it seems likely worth some effort. I started to look into it, but the actually value and likely practical is so low.
>
> I'm +1 on starting here.
Agreed ... the reason I didn't think caching would work is requests to the `/sql` handler get load balanced across potentially many nodes. Also given the rows / cols nature of SQL result sets, it's hard to pass back the next `cursorMark` to the client to include in the next paging request. So I guess you could cache the next cursorMark with the SQL + params as the key in a global distributed cache (znode) but that feels like a lot of complexity for a questionable SQL use case? It's not lost on me that deep paging is dangerous when exposed to web crawlers but that doesn't seem like it's as much of a risk with SQL.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #191: SOLR-15489: Implement OFFSET & FETCH for LIMIT queries
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #191:
URL: https://github.com/apache/solr/pull/191#discussion_r660288462
##########
File path: solr/core/src/test/org/apache/solr/handler/TestSQLHandler.java
##########
@@ -1853,21 +1853,18 @@ public void testParallelTimeSeriesGrouping() throws Exception {
}
protected List<Tuple> getTuples(final SolrParams params, String baseUrl) throws IOException {
- //log.info("Tuples from params: {}", params);
- TupleStream tupleStream = new SolrStream(baseUrl, params);
-
- tupleStream.open();
- List<Tuple> tuples = new ArrayList<>();
- for (; ; ) {
- Tuple t = tupleStream.read();
- //log.info(" ... {}", t.fields);
- if (t.EOF) {
- break;
- } else {
- tuples.add(t);
+ List<Tuple> tuples = new LinkedList<>();
+ try (TupleStream tupleStream = new SolrStream(baseUrl, params)) {
Review comment:
This pattern makes me wish we had an `asStream` method on TupleStream that returned a java Stream. Maybe a future issue.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] thelabdude commented on a change in pull request #191: SOLR-15489: Implement OFFSET & FETCH for LIMIT queries
Posted by GitBox <gi...@apache.org>.
thelabdude commented on a change in pull request #191:
URL: https://github.com/apache/solr/pull/191#discussion_r660562145
##########
File path: solr/core/src/test/org/apache/solr/handler/TestSQLHandler.java
##########
@@ -1853,21 +1853,18 @@ public void testParallelTimeSeriesGrouping() throws Exception {
}
protected List<Tuple> getTuples(final SolrParams params, String baseUrl) throws IOException {
- //log.info("Tuples from params: {}", params);
- TupleStream tupleStream = new SolrStream(baseUrl, params);
-
- tupleStream.open();
- List<Tuple> tuples = new ArrayList<>();
- for (; ; ) {
- Tuple t = tupleStream.read();
- //log.info(" ... {}", t.fields);
- if (t.EOF) {
- break;
- } else {
- tuples.add(t);
+ List<Tuple> tuples = new LinkedList<>();
+ try (TupleStream tupleStream = new SolrStream(baseUrl, params)) {
Review comment:
Maybe? Here I'm fixing a bug where the stream didn't get closed if an exception gets raised, causing tests to hang.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org