-
Couldn't load subscription status.
- Fork 12
Introduce batch support. #136
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
base: main
Are you sure you want to change the base?
Conversation
|
There is an existing patch(es) for this commit SHA: Please note that the status that is posted is not in the context of this PR but rather the (latest) existing patch and that may affect some tests that may depend on the particular PR. If your tests do not rely on any PR-specific values (like base or head branch name) then your tests will report the same status. If you would like a patch to run in the context of this PR and abort the other(s), comment 'evergreen retry'. |
| @Override | ||
| public void addBatch(String mql) throws SQLException { | ||
| checkClosed(); | ||
| throw new SQLFeatureNotSupportedException("TODO-HIBERNATE-35 https://jira.mongodb.org/browse/HIBERNATE-35"); | ||
| } | ||
|
|
||
| @Override | ||
| public void clearBatch() throws SQLException { | ||
| checkClosed(); | ||
| throw new SQLFeatureNotSupportedException("TODO-HIBERNATE-35 https://jira.mongodb.org/browse/HIBERNATE-35"); | ||
| } | ||
|
|
||
| @Override | ||
| public int[] executeBatch() throws SQLException { | ||
| checkClosed(); | ||
| closeLastOpenResultSet(); | ||
| throw new SQLFeatureNotSupportedException("TODO-HIBERNATE-35 https://jira.mongodb.org/browse/HIBERNATE-35"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not implemented because Hibernate relies on PreparedStatement methods for batching.
|
Let's remove Update: we decided to leave it, as removing it causes code restructuring (otherwise the code looks weird), and we don't want to do that. Continue having this method seems to be a lesser burden than removing it. |
src/integrationTest/java/com/mongodb/hibernate/jdbc/MongoPreparedStatementIntegrationTests.java
Outdated
Show resolved
Hide resolved
src/integrationTest/java/com/mongodb/hibernate/jdbc/MongoPreparedStatementIntegrationTests.java
Outdated
Show resolved
Hide resolved
src/integrationTest/java/com/mongodb/hibernate/jdbc/MongoPreparedStatementIntegrationTests.java
Outdated
Show resolved
Hide resolved
|
|
||
| @ParameterizedTest(name = "test not supported update elements. Parameters: option={0}") | ||
| @ValueSource(strings = {"hint", "collation", "arrayFilters", "sort", "upsert", "c"}) | ||
| void testNotSupportedUpdateElements(String unsupportedElement) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not called "element", they are called "option". See https://www.mongodb.com/docs/manual/reference/command/update/. Let's make all the changes needed to reflect that.
The same applies to testNotSupportedDeleteElements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed during the walkthrough - renamed to "field".
| } | ||
|
|
||
| @ParameterizedTest(name = "test not supported update elements. Parameters: option={0}") | ||
| @ValueSource(strings = {"hint", "collation", "arrayFilters", "sort", "upsert", "c"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect, we should use both keys and values, like "hint: 'abc'", here, as otherwise the { _id: 1 } value currently used as the value of each option is not always valid. If we use a valid value for each option, then having an exception makes it more clear the the option name itself caused it.
The same applies to testNotSupportedDeleteElements.
…redStatementIntegrationTests.java Co-authored-by: Valentin Kovalenko <[email protected]>
…redStatementIntegrationTests.java Co-authored-by: Valentin Kovalenko <[email protected]>
…redStatementIntegrationTests.java Co-authored-by: Valentin Kovalenko <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished reviewing MongoPreparedStatementIntegrationTests. So far I have the following reviewed:
AbstractQueryIntegrationTestsLimitOffsetFetchClauseIntegrationTestsBatchUpdateIntegrationTestsMongoPreparedStatementIntegrationTests
We worked thoroughly on MongoPreparedStatement, MongoStatement during the code walkthrough. But I will still need to look at them once the changes are pushed.
The last reviewed commit is 11a2b07.
| .returns(0, BatchUpdateException::getErrorCode); | ||
| } catch (SQLException e) { | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this catch (SQLException e) here and in any other test in ExecuteBatchTests.
Pre-existing test methods need it because they implement Function, while the new tests implement Work, and its Work.execute method throws SQLException.
src/integrationTest/java/com/mongodb/hibernate/jdbc/MongoPreparedStatementIntegrationTests.java
Show resolved
Hide resolved
|
|
||
| @Test | ||
| void testBatchInsert() { | ||
| var batchCount = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe let's call it batchSize here and in other places? Feels more natural, and is closer to hibernate.jdbc.batch_size, which has the same meaning.
Neither JDBC API documentation, nor the JDBC specification seem to use "count" to refer to the number of commands in a batch.
| for (int i = 0; i < batchCount; i++) { | ||
| expectedDocs.add(BsonDocument.parse(format( | ||
| """ | ||
| { | ||
| "_id": %d, | ||
| "title": "Book %d" | ||
| }""", | ||
| i + 1, i + 1))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use for (int i = 1; i <= batchCount; i++), especially given that above we used these exact boundaries, so that we could use i, i instead of i + 1, i + 1. The same suggestion applies to testBatchUpdate.
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Test statement’s batch queue is reset once executeBatch returns") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API documentation of PreparedStatement.addBatch calls it "batch of commands". I found "qeue" neither in the API documentation nor in the JDBC specification. Unless I missed it, and "queue" is the term that is actually used, let's say either "statement’s batch" or "statement's batch of commands".
…redStatementIntegrationTests.java Co-authored-by: Valentin Kovalenko <[email protected]>
Remove redundant tests.
# Conflicts: # src/main/java/com/mongodb/hibernate/jdbc/MongoPreparedStatement.java # src/test/java/com/mongodb/hibernate/jdbc/MongoPreparedStatementTests.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
| } | ||
| if (!commandDescription.isUpdate()) { | ||
| throw new SQLFeatureNotSupportedException( | ||
| format("Unsupported command for batch operation: %s", commandDescription.getCommandName())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"operation" here, and also in "Unsupported command for query operation"/"Unsupported command for update operation" does not refer to the operations executable via MongoClient. Rather, it refers to the executeBatch, executeQuery, executeUpdate methods. In EXCEPTION_MESSAGE_OPERATION_FAILED, EXCEPTION_MESSAGE_TIMEOUT it, on the other hand, refers to the operation executable via MongoClient, as far as I understand.
I fail to think of a term that seems both suitable here and not conflating with other meanings (operation is specific to MongoClient, and command is specific to the DBMS, which is MongoDB in our case). So I propose us to just say "Unsupported command for executeUpdate"/"Unsupported command for executeQuery"/"Unsupported command for executeUpdate".
| checkClosed(); | ||
| throw new SQLFeatureNotSupportedException("TODO-HIBERNATE-35 https://jira.mongodb.org/browse/HIBERNATE-35"); | ||
| checkAllParametersSet(); | ||
| commandBatch.add(command.clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[optional]
This check may not help us at all, as batching parameterless commands is hardly practical, but neither will it hurt, and it makes it clear that we have to clone specifically because of the parameter value setters.
| commandBatch.add(command.clone()); | |
| commandBatch.add(parameterValueSetters.isEmpty() ? command : command.clone()); |
| return returnsResultSet; | ||
| } | ||
|
|
||
| static CommandDescription fromString(String commandName) throws SQLFeatureNotSupportedException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of/fromName?
|
|
||
| private void checkSupportedQueryCommand(BsonDocument command) throws SQLException { | ||
| var commandDescription = getCommandDescription(command); | ||
| if (commandDescription.isUpdate()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A command used as a query also must produce a result set, mustn't it?
if (commandDescription.isUpdate() || !commandDescription.returnsResultSet()) {However, if we do #136 (comment), then we don't need any changes here.
| * Indicates whether the command is used in {@code executeUpdate(...)} or {@code executeBatch()} methods. | ||
| * | ||
| * @return true if the command is used in update operations, false if it is used in query operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method indicates only whether a command may be used in executeUpdate/executeBatch
| * Indicates whether the command is used in {@code executeUpdate(...)} or {@code executeBatch()} methods. | |
| * | |
| * @return true if the command is used in update operations, false if it is used in query operations. | |
| * Indicates whether the command may be used in {@code executeUpdate(...)} or {@code executeBatch()} methods. | |
| * | |
| * @return true if the command may be used in update operations, false if it is used in query operations. |
| } | ||
|
|
||
| /** @throws BatchUpdateException if any of the commands in the batch attempts to return a result set. */ | ||
| private void checkSupportedBatchCommand(BsonDocument command) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private void checkSupportedBatchCommand(BsonDocument command) | |
| private static void checkSupportedBatchCommand(BsonDocument command) |
| closeLastOpenResultSet(); | ||
| checkAllParametersSet(); | ||
| return executeQueryCommand(command); | ||
| return executeQuery(command); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checkSupportedQueryCommand is missing here, and it should be after checkAllParametersSet, similarly to how it is in executeUpdate.
| return executeQuery(command); | |
| checkSupportedQueryCommand(command); | |
| return executeQuery(command); |
| static final int NO_ERROR_CODE = 0; | ||
| static final int[] EMPTY_BATCH_RESULT = new int[0]; | ||
|
|
||
| @Nullable static final String NULL_SQL_STATE = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This annotation applies to ElementType.TYPE_USE:
| @Nullable static final String NULL_SQL_STATE = null; | |
| static final String @Nullable NULL_SQL_STATE = null; |
Co-authored-by: Valentin Kovalenko <[email protected]>
Co-authored-by: Valentin Kovalenko <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost done, but some more review work (or, rather, trying and maybe proposing) is needed in the area of exception handling and transaction handling. I continue reviewing.
| .formatted(commandDescription.getCommandName()), | ||
| NULL_SQL_STATE, | ||
| NO_ERROR_CODE, | ||
| NULL_UPDATE_COUNTS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we use null here and new int[0] in the other place? The API/specification does not seem to specify any difference between them. I propose us to use the same value.
| private static final String EXCEPTION_MESSAGE_OPERATION_FAILED = "Failed to execute operation"; | ||
| private static final String EXCEPTION_MESSAGE_TIMEOUT = "Timeout while waiting for operation to complete"; | ||
| static final int NO_ERROR_CODE = 0; | ||
| static final int[] EMPTY_BATCH_RESULT = new int[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We name it EMPTY_BATCH_RESULT here, but we name NULL_UPDATE_COUNTS conceptually the same thing in the other place. Also, the method we use EMPTY_BATCH_RESULT in is called calculateBatchUpdateCounts.
Let's rename this to EMPTY_UPDATE_COUNTS.
|
|
||
| class MongoStatement implements StatementAdapter { | ||
| private static final String EXCEPTION_MESSAGE_OPERATION_FAILED = "Failed to execute operation"; | ||
| private static final String EXCEPTION_MESSAGE_TIMEOUT = "Timeout while waiting for operation to complete"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be named EXCEPTION_MESSAGE_OPERATION_TIMED_OUT similarly to how EXCEPTION_MESSAGE_OPERATION_FAILED is named?
|
|
||
| private WriteModelConverter() {} | ||
|
|
||
| private static void convertToWriteModels( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[optional]
- This is the only program element in
WriteModelConverterthat is intended to be used outside of this class. Let's make it package-access to express that? Note that sinceWriteModelConverteritself isprivate, the making its method package-access does not really make it more visible. - If you agree with such an approach, then the same applies to the constructor,
add,findCommandIndexinWriteModelsToCommandMapper.
| INSERT("insert", false, true), | ||
| UPDATE("update", false, true), | ||
| DELETE("delete", false, true), | ||
| AGGREGATE("aggregate", true, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[optional]
Let's link to the command docs for convenience.
| INSERT("insert", false, true), | |
| UPDATE("update", false, true), | |
| DELETE("delete", false, true), | |
| AGGREGATE("aggregate", true, false); | |
| /** See <a href="https://www.mongodb.com/docs/manual/reference/command/insert/">{@code insert}</a>. */ | |
| INSERT("insert", false, true), | |
| /** See <a href="https://www.mongodb.com/docs/manual/reference/command/update/">{@code update}</a>. */ | |
| UPDATE("update", false, true), | |
| /** See <a href="https://www.mongodb.com/docs/manual/reference/command/delete/">{@code delete}</a>. */ | |
| DELETE("delete", false, true), | |
| /** See <a href="https://www.mongodb.com/docs/manual/reference/command/aggregate/">{@code aggregate}</a>. */ | |
| AGGREGATE("aggregate", true, false); |
| var writeErrors = mongoBulkWriteException.getWriteErrors(); | ||
| var writeConcernError = mongoBulkWriteException.getWriteConcernError(); | ||
| if (writeConcernError == null) { | ||
| if (!writeErrors.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API documentation of MongoBulkWriteException.getWriteConcernError says
The write concern error, which may be null (in which case the list of errors will not be empty).
That is, if writeConcernError == null, we know that writeErrors is not empty, and we should assert than rather than check.
Furthermore, since our bulk write is ordered, which is the default, we know that the size of writeErrors is 1. Thus, we should have assertTrue(writeErrors.size() == 1) instead of if (!writeErrors.isEmpty()).
I seem to remember us discussing this.
| if (writeConcernError == null) { | ||
| if (!writeErrors.isEmpty()) { | ||
| var failedModelIndex = writeErrors.get(0).getIndex(); | ||
| var commandIndex = writeModelsToCommandMapper.findCommandIndex(failedModelIndex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commandIndex -> failedCommandIndexInBatch?
| private static final String UNSUPPORTED_MESSAGE_STATEMENT_FIELD = "Unsupported field in %s statement: [%s]"; | ||
| private static final String UNSUPPORTED_MESSAGE_COMMAND_FIELD = "Unsupported field in %s command: [%s]"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- These are message templates, not messages.
- Shouldn't the first
%sbe also wrapped in[,]?
| private static final String UNSUPPORTED_MESSAGE_STATEMENT_FIELD = "Unsupported field in %s statement: [%s]"; | |
| private static final String UNSUPPORTED_MESSAGE_COMMAND_FIELD = "Unsupported field in %s command: [%s]"; | |
| private static final String UNSUPPORTED_MESSAGE_TEMPLATE_STATEMENT_FIELD = | |
| "Unsupported field in [%s] statement: [%s]"; | |
| private static final String UNSUPPORTED_MESSAGE_TEMPLATE_COMMAND_FIELD = | |
| "Unsupported field in [%s] command: [%s]"; |
| private static final Set<String> SUPPORTED_INSERT_COMMAND_FIELDS = Set.of("documents"); | ||
|
|
||
| private static final Set<String> SUPPORTED_UPDATE_COMMAND_FIELDS = Set.of("updates"); | ||
| private static final Set<String> SUPPORTED_UPDATE_STATEMENT_FIELDS = Set.of("q", "u", "multi"); | ||
|
|
||
| private static final Set<String> SUPPORTED_DELETE_COMMAND_FIELDS = Set.of("deletes"); | ||
| private static final Set<String> SUPPORTED_DELETE_STATEMENT_FIELDS = Set.of("q", "limit"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[optional]
What do you think about moving these to CommandDescription? That way the information tightly related to commands stays mostly there.
P.S. At first I thought about moving them there as instance methods, potentially with the checks, but then I realized that different commands have different structure (for example, aggregate does not have statements), which makes such endeavour non-trivial.
|
|
||
| private static void checkFields( | ||
| CommandDescription commandDescription, | ||
| String exceptionMessage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a template, not a message: exceptionMessage -> exceptionMessageTemplate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| var collection = getCollection(commandDescription, command); | ||
| var pipeline = command.getArray("pipeline").stream() | ||
| .map(BsonValue::asDocument) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
command.getArray("pipeline")may throwBsonInvalidOperationException, which before we started supporting native queries wasn't possible.BsonValue::asDocumentmay also result inBsonInvalidOperationExceptionfor the same reason.- We start the transaction before obtaining
collection, dealing with the pipeline, really for no good reason.
Previously this wasn't needed because there was no native queries support. We should handle these exceptions here similarly to getCollection.
| int executeUpdateCommand(BsonDocument command) throws SQLException { | ||
| int[] executeBatch(List<? extends BsonDocument> commandBatch) throws SQLException { | ||
| var firstCommandInBatch = commandBatch.get(0); | ||
| int commandBatchSize = commandBatch.size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| int commandBatchSize = commandBatch.size(); | |
| var commandBatchSize = commandBatch.size(); |
| throw new SQLException("Failed to execute update command", e); | ||
| var writeModels = new ArrayList<WriteModel<BsonDocument>>(commandBatchSize); | ||
| writeModelsToCommandMapper = new WriteModelsToCommandMapper(commandBatchSize); | ||
| for (BsonDocument command : commandBatch) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| for (BsonDocument command : commandBatch) { | |
| for (var command : commandBatch) { |
| ResultSet executeQuery(BsonDocument command) throws SQLException { | ||
| var commandDescription = getCommandDescription(command); | ||
| try { | ||
| startTransactionIfNeeded(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Let's shift everything above
trydown so that the lines are at the top of thetryblock. - Let's move the
startTransactionIfNeeded()call right beforecollection.aggregate. We should not start a transaction before it is actually needed.
This also applies to executeUpdate, executeBatch. Here is what I mean: 2d09493 (the commit is only for expressing what I mean, whether to cherry-pick it or do the changes otherwise is up to you).
https://jira.mongodb.org/browse/HIBERNATE-40
https://jira.mongodb.org/browse/HIBERNATE-35