Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL: clear the cursor if nested inner hits are enough to fulfill the query required limits #35398

Merged
merged 13 commits into from
Nov 15, 2018
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,18 @@
import org.elasticsearch.xpack.sql.qa.jdbc.CsvSpecTestCase;
import org.elasticsearch.xpack.sql.qa.jdbc.CsvTestUtils.CsvTestCase;

import java.sql.Connection;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;

public class JdbcCsvSpecIT extends CsvSpecTestCase {
public JdbcCsvSpecIT(String fileName, String groupName, String testName, Integer lineNumber, CsvTestCase testCase) {
super(fileName, groupName, testName, lineNumber, testCase);
}

@Override
protected ResultSet executeJdbcQuery(Connection con, String query) throws SQLException {
protected int fetchSize() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpick - to remove the super.fetchSize redundancy, you can make the method a one liner using the ternary conditional:

return fileName.startsWith("nested") && randomBoolean() ? randomIntBetween(1,5) : super.fetchSize();

// using a smaller fetchSize for nested documents' tests to uncover bugs
// similar with https://github.com/elastic/elasticsearch/issues/35176 quicker
if (fileName.startsWith("nested")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of overriding executeJdbcQuery, override fetchSize():

protected int fetchSize() {
        if (fileName.startsWith("nested")) {
           return randomBoolean() ? super.fetchSize() : randomIntBetween(1,5));
        }
  }

Statement statement = con.createStatement();
statement.setFetchSize(randomBoolean() ? fetchSize() : randomIntBetween(1, 5));
return statement.executeQuery(query);
return randomBoolean() ? super.fetchSize() : randomIntBetween(1, 5);
}
return super.executeJdbcQuery(con, query);
return super.fetchSize();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class SearchHitRowSet extends AbstractRowSet {
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method can have default visibility as it's not used anywhere else.

public boolean isLimitReached() {
protected boolean isLimitReached() {
return cursor == Cursor.EMPTY;
}

Expand Down