Repository navigation
Documentation update: ResultSet.countEntries() consumes ResultSet iterator #728
Description
ArcadeDB Version: v22.11.1
JDK Version: openjdk 11.0.12 2021-07-20 LTS
OpenJDK Runtime Environment Corretto-11.0.12.7.1 (build 11.0.12+7-LTS)
OpenJDK 64-Bit Server VM Corretto-11.0.12.7.1 (build 11.0.12+7-LTS, mixed mode)
OS: CentOS release 6.9 (Final)
Expected behavior
The documentation should include a big, bold, red WARNING: ResultSet.countEntries() consumes the ResultSet iterator.
If you call countEntries() after the query is executed, but before iterating over the Resultset, ResultSet.hasNext() will be false and you won't be able to retrieve any records.
The ResultSet Interface contains the following note: "Note, that the default implementation will consume the iterator", but it is not clear if some of the many classes that implement ResultSet override this method and provide an implementation that allows calling countEntries before iterating over the result set.
I have tried to find out myself, but IntelliJ Idea reports 474 usages of ResultSet in the code, so it would be too time-consuming to reverse-engineer the code.
Are there any circumstances in which countEntries() does not consume the iterator?
Actual behavior
The documentation does not explain this constraint.
Steps to reproduce
use the following class
public class Test {
Database database;
public static void main(String[] args) {
Test test = new Test();
test.start();
}
void start() {
// create database
try (DatabaseFactory databaseFactory = new DatabaseFactory("./databases/test");
Database db = databaseFactory.create()) {
db.transaction(() -> {
DocumentType dtProducts = db.getSchema().createDocumentType("Product");
dtProducts.createProperty("name", Type.STRING);
dtProducts.createProperty("type", Type.STRING);
dtProducts.createProperty("start", Type.STRING);
dtProducts.createProperty("stop", Type.STRING);
});
} catch (Exception e) {
System.out.println(e.getMessage());
System.exit(1);
}
// start server
ArcadeDBServer arcadeDBServer = new ArcadeDBServer();
arcadeDBServer.start();
database = arcadeDBServer.getDatabase("test");
// insert 1 record
String sqlString, name, start, stop;
String type = "SIR1LRM_0_";
sqlString = "INSERT INTO Product SET name = ?, type = ?, start = ?, stop = ?";
try {
database.begin();
name = "CS_OPER_SIR1LRM_0__20220319T002624_20220319T002827_0001.DBL";
start = "20220319_002624.404379";
stop = "20220319_002826.525650";
try (ResultSet resultSet = database.command("sql", sqlString, name, type, start, stop)) {
System.out.println("n. of records inserted = " + resultSet.countEntries());
}
database.commit();
} catch (Exception e) {
System.out.println(e.getMessage());
database.rollback();
}
// execute query
sqlString = "SELECT FROM Product";
try {
System.out.println("test 1: send query, count entries, check next");
database.begin();
try (ResultSet resultSet = database.query("sql", sqlString)) {
System.out.println("query " + sqlString + " returned " + resultSet.countEntries() + " records");
System.out.println("resultSet.hasNext() = " + resultSet.hasNext());
} catch (Exception e) {
System.out.println(e.getMessage());
}
database.commit();
System.out.println("test 2: send query, check next, count entries");
database.begin();
try (ResultSet resultSet = database.query("sql", sqlString)) {
System.out.println("resultSet.hasNext() = " + resultSet.hasNext());
System.out.println("query " + sqlString + " returned " + resultSet.countEntries() + " records");
} catch (Exception e) {
System.out.println(e.getMessage());
}
database.commit();
} catch (Exception e) {
System.out.println(e.getMessage());
database.rollback();
}
}
}
After running it, inspect the System.out messages:
n. of records inserted = 1
test 1: send query, count entries, check next
query SELECT FROM Product returned 1 records
resultSet.hasNext() = false
test 2: send query, check next, count entries
resultSet.hasNext() = true
query SELECT FROM Product returned 1 records
Activity
lvca commentedon Dec 31, 2022
I just checked the usage of
ResultSet.countEntries()and I see only 6 spots, all test cases. We have this interface in the core:That mark the possibility to reset the iterator back to the start. So counting the entries is harmful for such implementation. Anyway I agree, there should be better docs and semantic around this.
lvca commentedon Jan 1, 2023
The SQL engine tries, as much as it's possible, to avoid leverage onto the RAM. A lot of operations are lazy evaluated while you're iterating the resultset. This means it's not possible to know the resultset size at priori unless the whole result set is iterated, that means the whole query is executed.
There is no clean way to fix this. Since that API is only used in tests and it's very misleading (even with a bold javadoc), I'd prefer to remove it from the code base and move it as
CollectionUtils.countEntries(iterator).Removed unused `Resultset.countEntries()` to avoid confusion
vic0824 commentedon Jan 2, 2023
Totally agree.
The next step is to provide a consistent way to obtain the number of records affected by an UPDATE/INSERT/DELETE command.
This is the current situation:
This means that the client code needs to implement two different ways of obtaining the number of affected records:
ResultSet.hasNext()for INSERTResultSet.next().toJSON()for DELETE/UPDATEI think the best approach would be to update the INSERT so that it returns the count as default, like DELETE and UPDATE.
It is true that in this case the count can only be 1 or 0, but it would provide the client code with a method of obtaining the number of affected records that is consistent across the various DML commands.
As a "nice to have", the Result.toString() could just return the count, it would make it easier to parse it. The toJSON() method already provides a way to obtains a JSON object with the count field, so it seems redundant to provide a similar format in the toString method.
I can open a dedicated enhancement request under "Ideas".
lvca commentedon Jan 3, 2023
The INSERT can only return the inserted record. Can't return zero records (as a count) because it would throw an exception instead. So having a count in the insert could be pointless, unless I'm forgetting a way to insert multiple records with one statement.
About the JSON format, I think it's just the toString() of the object. You should access the "count" as a property of the Result. Like:
I'd like to have in the ResultSet utility methods to simplify the most common use cases, like when the returned element is only 1, like in this case. I was thinking of something like this also to avoid null checks:
So the code above would be more readable as:
Where the
Result.getProperty()allows a default value.WDYT?
feat: added new utility method in ResultSet, example: `int updated = …
lvca commentedon Jan 3, 2023
I've just committed such a helper method and used it in a test class. Before documenting it, I'd like to have your feedback.
vic0824 commentedon Jan 4, 2023
If I use it like this:
int updateCount = resultSet.nextIfAvailable().getProperty("count", 0);I get a
java.lang.ClassCastException: class java.lang.Long cannot be cast to class java.lang.IntegerI need to change it to:
long updateCount = resultSet.nextIfAvailable().getProperty("count", 0L)or
int updateCount = resultSet.nextIfAvailable().getProperty("count", 0L).intValue()to make it work.
lvca commentedon Jan 4, 2023
I can work this out by accepting a generic Object as the default parameter and then casting the
defaultValueto the returned. This would allow automatic casting among numbers.fix: Result.getProperty() automatic cast of default value
lvca commentedon Jan 4, 2023
Ok tested with:
And it works properly.
vic0824 commentedon Jan 5, 2023
Tested it with v23.1.1-SNAPSHOT (build 0ba5399/1672916179785/main) (pulled from git this morning).
My arcadedb-engine-23.1.1-SNAPSHOT.jar has the following MD5 checksum: 93faef2f53b8a44f16988380ed583be1
if updateCount is defined as an int, I get the same class cast exception as before.
If updateCount is defined as a long (like I see you do in your test cases uploaded in commit 5d21de2), it works.
It's no big deal, the important thing was the compact syntax, having the count as a long is perfectly fine.
test: created additional tests for result conversion
build(deps): bump actions/setup-java from 4.6.0 to 4.7.0 [skip ci]
chore(deps): bump the github-actions group with 5 updates [skip ci]
chore(deps): bump the github-actions group with 2 updates [skip ci]
Removed unused `Resultset.countEntries()` to avoid confusion
feat: added new utility method in ResultSet, example: `int updated = …
fix: Result.getProperty() automatic cast of default value
test: created additional tests for result conversion
build(deps): bump actions/setup-java from 4.6.0 to 4.7.0 [skip ci]
chore(deps): bump the github-actions group with 5 updates [skip ci]
chore(deps): bump the github-actions group with 2 updates [skip ci]