Hibernate batch processing #35

NathanQingyangXu · 2025-02-04T14:00:22Z

https://jira.mongodb.org/browse/HIBERNATE-35

Hibernate uses JDBC batch feature only for PreparedStatement, so the code logic changes are centralized in MongoPreparedStatement, not in MongoStatement. On top of JDBC's batch feature, Hibernate builds some higher abstraction (see BatchImpl class: https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/java/org/hibernate/engine/jdbc/batch/internal/BatchImpl.java), so we need to code our logic in the context of Hibernate's usage.

Internally bulk writing API methods are used as implementation. ReplaceOne write model is not added for it is out of scope of milestone1.

It is a pity there is no possibility to showcase the batch processing using high-level Hibernate's CRUD processing context.
I tested it on top of our first CRUD PR and confirmed that it works as expected (by checking logs and explore in debugging mode).

The pattern of batch processing in PreparedStatement is as follows:

for (var i = 0;  i < batch_size; i++) {
    pstmt.setXXX(1, ...);
    pstmt.setXXX(2, ...);
    ... ...
    pstmt.addBatch();
    pstmt.executeBatch();
}

Only when PreparedStatement is to be released (but not closed), should the clearBatch() method be invoked. As the pattern above shows, the internal statement batch should have been emptied after executeBatch() invocation (Hibernate would guarantee there is no dangling last batch without executing), so in theory there is not much to be done in the clearBatch() method (its name might be misleading into thinking that it is in charge of emptying the batch storage internally).

NathanQingyangXu · 2025-02-04T14:01:09Z

src/main/java/com/mongodb/hibernate/internal/ConfigurationHelper.java

+        throw new ConfigurationException("Could not determine how to handle configuration value [name=" + name
+                + ", value=" + value + "(" + value.getClass().getName() + ")] as int");
+    }
+}


the above util class was copied from Hibernate's internal class.

jyemin

Initial comments

src/main/java/com/mongodb/hibernate/jdbc/MongoPreparedStatement.java

stIncMale · 2025-02-04T15:42:33Z

src/main/java/com/mongodb/hibernate/jdbc/MongoConnectionProvider.java

-        var jdbcUrl = configValues.get(JAKARTA_JDBC_URL);
+    public void configure(Map<String, Object> configurationValues) {
+
+        batchSize = ConfigurationHelper.getInt(AvailableSettings.STATEMENT_BATCH_SIZE, configurationValues, 0);


This approach conflicts with the one proposed in #31 (there, we would have a new setting in MongoDialectSettings). Let's deal with the configuration approach first.

So we want to centralize all Hibernate related configs in one single class? That is ambitous. This PR would be merged after yours for sure.

the latest commit has removed the usage of batchable and addBatch() method alone will decide. For that reason, no configuration is needed at all in this PR. I removed the copied ConfigurationHelper as well.

stIncMale · 2025-02-04T15:48:19Z

src/main/java/com/mongodb/hibernate/jdbc/MongoPreparedStatement.java

+        this(mongoClient, clientSession, mongoConnection, mql, false);
+    }
+
+    public MongoPreparedStatement(


MongoPreparedStatement is package access, there is no point in introducing a public constructor.

I have just noticed, the same applies to:

the existing MongoPreparedStatement constructor

the existing MongoStatement constructor

good catch! changed.

stIncMale · 2025-02-04T15:55:01Z

src/main/java/com/mongodb/hibernate/jdbc/MongoPreparedStatement.java


 /**
 * MongoDB Dialect's JDBC {@link java.sql.PreparedStatement} implementation class.
 *
 * <p>It only focuses on API methods MongoDB Dialect will support. All the other methods are implemented by throwing
 * exceptions in its parent {@link PreparedStatementAdapter adapter interface}.
 */
-final class MongoPreparedStatement extends MongoStatement implements PreparedStatementAdapter {
+class MongoPreparedStatement extends MongoStatement implements PreparedStatementAdapter {


What is the reason behind making this class extensible in this PR?

My first iteration is to separate batch related logic into a child class called MongoBatchablePreparedStatement, later on I referred to other JDBC driver implementation and gave up the idea; then I forgot to add back final. Changed.

…dStatement

src/main/java/com/mongodb/hibernate/jdbc/MongoPreparedStatement.java

src/main/java/com/mongodb/hibernate/jdbc/MongoStatement.java

src/main/java/com/mongodb/hibernate/jdbc/MongoPreparedStatement.java

This reverts commit 691777f.

src/main/java/com/mongodb/hibernate/jdbc/MongoPreparedStatement.java

NathanQingyangXu · 2025-02-18T14:22:57Z

@jyemin given the timeout concern will be dealt with in #40 (which will centralize the updating in one place, regardless of bulk updating or not), could we continue reviewing this PR? I think the two PRs are tightly related (which one is merged first requires updating of the second one), but I think the necessary refactoring should be straightforward.

jyemin · 2025-02-18T14:29:55Z

Yes, we can continue. Did you mean to re-request reviews?

NathanQingyangXu · 2025-02-18T17:45:46Z

Yes, we can continue. Did you mean to re-request reviews?

Yeah, I clicked the re-request button then.

src/main/java/com/mongodb/hibernate/jdbc/MongoPreparedStatement.java

# Conflicts: # src/main/java/com/mongodb/hibernate/jdbc/MongoConnectionProvider.java # src/main/java/com/mongodb/hibernate/jdbc/MongoPreparedStatement.java # src/test/java/com/mongodb/hibernate/jdbc/MongoConnectionTests.java # src/test/java/com/mongodb/hibernate/jdbc/MongoPreparedStatementTests.java # src/test/resources/hibernate.properties

# Conflicts: # src/integrationTest/java/com/mongodb/hibernate/jdbc/MongoPreparedStatementIntegrationTests.java

…n and deletion features

stIncMale

[I have not reviewed this PR]

We still have TODO-HIBERNATE-35 comments in the code, but all of them are supposed to be addressed as part of https://jira.mongodb.org/browse/HIBERNATE-35, which this PR implements. What is the plan for those?

I overlooked. Those references should have been deleted for Hibernate only supports batch processing in PreparedStatement, not Statement.

stIncMale · 2025-03-02T21:02:10Z

src/integrationTest/java/com/mongodb/hibernate/jdbc/BatchUpdateIntegrationTests.java

+                        command4 -> assertThat(command4.getFirstKey()).isEqualTo("commitTransaction"));
+    }
+
+    // TODO-HIBERNATE-19 https://jira.mongodb.org/browse/HIBERNATE-19


This PR adds a TODO-HIBERNATE-19 note, but #59 for https://jira.mongodb.org/browse/HIBERNATE-19 has already been created. Now #59 (HIBERNATE-19) depends on the current PR (#35 (HIBERNATE-35)), yet they are being worked on concurrently.

If we proceed as is, it is not impossible that we'll merge #59 (HIBERNATE-19) first, then the currernt PR (#35 (HIBERNATE-35)), and the TODO-HIBERNATE-19 added by the current PR will stay in main despite HIBERNATE-19 having been resolved. This scenario is actually very likely, given this comment.

What is our plan on making sure the above is not going to happen?

seems we can avoid such deadlock by disabling one PR for the time being until the other one got merged?

I disabled this PR for the time being so we could focus on #59.

I worry that when we create another PR for HIBERNATE-35, it will be based on this one (I don't think you'll just throw away these changes and start from scratch in the future), and the TODO-HIBERNATE-19 will slip there, despite HIBERNATE-19 having been closed. Can we replace this TODO-HIBERNATE-19 comment with a TODO-HIBERNATE-35 comment explaining what needs to be done, and that it should be done after implementing HIBERNATE-19.

did it. good point.

NathanQingyangXu · 2025-03-03T15:39:20Z

closed to avoid conflict with #59. Will open it when the dependency PR has been merged.

# Conflicts: # src/test/java/com/mongodb/hibernate/jdbc/MongoPreparedStatementTests.java

NathanQingyangXu · 2025-03-11T19:58:52Z

This PR is re-opened for the "basic update translation" is merged so a complete integration testing suite is possible.
@jyemin , the only new thing is the new integration testing case (including 'insertion', 'deletion' and 'update') at https://github.com/mongodb/mongo-hibernate/pull/35/files#diff-208727884b6fd50b4fce3475acee3115f5a5d880432232cf412e3a37ee48a235

NathanQingyangXu · 2025-03-11T20:56:19Z

close this PR for it might be out of scope of M1

implement batch processing for non-native query scenario.

19803e1

NathanQingyangXu requested review from jyemin and stIncMale February 4, 2025 14:00

NathanQingyangXu commented Feb 4, 2025

View reviewed changes

implement batch processing for non-native query scenario.

f643b35

NathanQingyangXu force-pushed the HIBERNATE-35-batch-processing branch from b0b4bc5 to f643b35 Compare February 4, 2025 14:02

jyemin removed request for jyemin and stIncMale February 4, 2025 14:06

NathanQingyangXu marked this pull request as ready for review February 4, 2025 14:06

jyemin reviewed Feb 4, 2025

View reviewed changes

stIncMale reviewed Feb 4, 2025

View reviewed changes

stIncMale requested changes Feb 4, 2025

View reviewed changes

NathanQingyangXu and others added 7 commits February 5, 2025 09:09

remove logic of redundant batchable testing in addBatch()

f360ce5

fix constructor visibility issues for MongoStatement and MongoPrepare…

23476a7

…dStatement

use final for MongoPreparedStatement

e1dee99

Merge branch 'main' into HIBERNATE-35-batch-processing

87a86e9

remove further unnecessary 'batchable' checking

3d30c26

remove unnecessary collection name checking

5e7f1ab

add more comments to explain some confusing logics for batch processing

64505a4

jyemin requested changes Feb 5, 2025

View reviewed changes

src/main/java/com/mongodb/hibernate/jdbc/MongoPreparedStatement.java Outdated Show resolved Hide resolved

remove batchable flag usage

93b2be7

jyemin requested changes Feb 5, 2025

View reviewed changes

NathanQingyangXu added 2 commits February 5, 2025 17:12

make clearBatch() empty commandBatch; avoid trivial private method

88f8759

simplify the implementation of MongoPreparedStatement#addBatch

6510723

jyemin requested changes Feb 6, 2025

View reviewed changes

src/main/java/com/mongodb/hibernate/jdbc/MongoStatement.java Outdated Show resolved Hide resolved

src/main/java/com/mongodb/hibernate/jdbc/MongoPreparedStatement.java Outdated Show resolved Hide resolved

refactor to reuse bulk writing usage for any update scenarios

691777f

NathanQingyangXu force-pushed the HIBERNATE-35-batch-processing branch from 1cc8940 to 691777f Compare February 7, 2025 14:51

NathanQingyangXu added 2 commits February 7, 2025 10:00

Revert "refactor to reuse bulk writing usage for any update scenarios"

ec7bf27

This reverts commit 691777f.

fix 'deleted' typo in delete command

7659a21

jyemin requested changes Feb 7, 2025

View reviewed changes

src/main/java/com/mongodb/hibernate/jdbc/MongoPreparedStatement.java Outdated Show resolved Hide resolved

src/main/java/com/mongodb/hibernate/jdbc/MongoPreparedStatement.java Show resolved Hide resolved

resolve conflict with latest main branch

225aaf6

NathanQingyangXu requested a review from jyemin February 18, 2025 17:45

jyemin previously approved these changes Feb 18, 2025

View reviewed changes

src/main/java/com/mongodb/hibernate/jdbc/MongoPreparedStatement.java Outdated Show resolved Hide resolved

jyemin mentioned this pull request Feb 18, 2025

improve MongoStatement#executeUpdate #40

Closed

NathanQingyangXu requested a review from stIncMale February 19, 2025 13:35

NathanQingyangXu added 2 commits February 25, 2025 09:33

resolve conflicts with main branch

b84c682

NathanQingyangXu dismissed jyemin’s stale review via b84c682 February 25, 2025 14:44

NathanQingyangXu added 7 commits February 26, 2025 13:19

Merge branch 'main' into HIBERNATE-35-batch-processing

44873e8

# Conflicts: # src/integrationTest/java/com/mongodb/hibernate/jdbc/MongoPreparedStatementIntegrationTests.java

resolve conflict with latest main branch

d1bbfa4

Merge branch 'main' into HIBERNATE-35-batch-processing

91e4f44

# Conflicts: # src/integrationTest/java/com/mongodb/hibernate/jdbc/MongoPreparedStatementIntegrationTests.java

merge in the latest main

9d07b33

improve MongoPreparedStatement in minor ways

e25246d

Merge branch 'main' into HIBERNATE-35-batch-processing

fcc0579

add batch integration testing cases based on finished entity insertio…

4d31fe5

…n and deletion features

stIncMale reviewed Mar 2, 2025

View reviewed changes

NathanQingyangXu closed this Mar 3, 2025

NathanQingyangXu added 4 commits March 3, 2025 10:51

remove unused batch methods in MongoStatement

e72248a

change TODO placeholder from 19 to 35

16d8848

Merge branch 'main' into HIBERNATE-35-batch-processing

74ca667

# Conflicts: # src/test/java/com/mongodb/hibernate/jdbc/MongoPreparedStatementTests.java

add batch update integration testing case

b23bfc5

NathanQingyangXu reopened this Mar 11, 2025

NathanQingyangXu closed this Mar 11, 2025

Hibernate batch processing #35

Hibernate batch processing #35

Uh oh!

Conversation

NathanQingyangXu commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyemin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NathanQingyangXu commented Feb 18, 2025

Uh oh!

jyemin commented Feb 18, 2025

Uh oh!

NathanQingyangXu commented Feb 18, 2025

Uh oh!

Uh oh!

stIncMale left a comment • edited by NathanQingyangXu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stIncMale Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stIncMale Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NathanQingyangXu commented Mar 3, 2025

Uh oh!

NathanQingyangXu commented Mar 11, 2025

Uh oh!

NathanQingyangXu commented Mar 11, 2025

Uh oh!

Uh oh!

NathanQingyangXu commented Feb 4, 2025 •

edited

Loading

stIncMale left a comment •

edited by NathanQingyangXu

Loading

stIncMale Mar 2, 2025 •

edited

Loading

stIncMale Mar 3, 2025 •

edited

Loading