Skip to content

simple DML translation #102

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

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

NathanQingyangXu
Copy link
Contributor

@NathanQingyangXu NathanQingyangXu commented May 27, 2025

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

The scope of the ticket is to unblock the following simple DML HQL mutation queries:

  • insert into Book (id, title) values (1, "War and Peace"), (2, "Crime and Punishmnet")
  • update Book set title = "War and Peace" where title = "War & Peace"
  • delete from Book where title = "War and Peace"

Translation is straightforward. The following new visitor methods would be implemented in this PR:

  • void visitDeleteStatement(DeleteStatement deleteStatement);
  • void visitUpdateStatement(UpdateStatement updateStatement);
  • void visitInsertStatement(InsertSelectStatement insertSelectStatement);

Both of the first two methods share a lot in common in the sense that they are both based on where clause, with various AST filters as the backbones, which has been finished in previous PR.

The scope of this PR is modest and only focuses on the following basic stuff:

  • no new AST filter will be introduced
  • when updating, only $set updating operator is supported (with the constraint that $set focuses on field path and value pair)

One caveat is previously our insertion command mongo ast only allows one document, to fully support HQL's feature, the mongo ast model was refactored to allow multiple documents, consistent with MQL's syntax as well. There is a separate ticket for this requirment: https://jira.mongodb.org/browse/HIBERNATE-44, so we can finish it in this PR additionally.

@NathanQingyangXu NathanQingyangXu marked this pull request as draft May 27, 2025 18:17
});
}

public static final class MutateTranslatorAwareDialect extends Dialect {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An important feature to test is we need to include the table names involved in DML queries into the result of SqlAstTranslator#getAffectedTableNames() which acts as auto-flush purpose in the upper level caller of the translator.

We might need to finish other features (like table joining) to test the auto-flush features in integration test; currently we could only test this important feature by some expedient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is. related to auto-flushing criteria No..2 (see https://docs.jboss.org/hibernate/orm/6.6/userguide/html_single/Hibernate_User_Guide.html#flushing-auto):

  • prior to executing a JPQL/HQL query that overlaps with the queued entity actions

@@ -36,17 +38,17 @@

@SessionFactory(exportSchema = false)
@ExtendWith(MongoExtension.class)
abstract class AbstractSelectionQueryIntegrationTests implements SessionFactoryScopeAware, ServiceRegistryScopeAware {
public abstract class AbstractQueryIntegrationTests implements SessionFactoryScopeAware, ServiceRegistryScopeAware {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this class is reusable not only for select query but also for mutate query, so it is renamed and moved to more general location. The Book class was changed accordingly to go together with it hand in hand.

throw new FeatureNotSupportedException("Unsupported mutation statement type: "
+ mutationStatement.getClass().getName());
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently there is no fourth class of MutationStatement; the above last else clause is for future-proof purpose.

…t/command/AstInsertCommand.java

Co-authored-by: Jeff Yemin <[email protected]>
checkMutationStatement(insertStatement);
if (insertStatement.getConflictClause() != null) {
throw new FeatureNotSupportedException();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

HQL has an interesting ON CONFLICT during insertion feature (see https://www.baeldung.com/hibernate-insert-query-on-conflict-clause). It is sort of similar to MongoDB's upsert feature but it seems there is a big difference between them:

  • HQL mainly focuses on conflict during insertion (w.r.t. unique constraint or fields combination); MongoDB's upsert focuses on conflict during updating (w.r.t. some query or filter,, e.g. comparison between a field and a value)

So I guess we can't support this HQL feature. HQL's update statement is simpler and provides no possibility to emulate MongoDB's upsert feature.

Copy link
Member

@vbabanin vbabanin Jun 24, 2025

Choose a reason for hiding this comment

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

I think we could support the same semantics in MQL.

1. "on conflict do update:

What HQL does:
insert … on conflict do update - checks a unique index; if the key is taken it runs the UPDATE clause.

What MongoDB does:
update … { upsert:true } matches a filter; if no doc matches it inserts instead.

Despite starting from opposite directions (insert-first vs. update-first) they end up with the same two-way branch:

Case HQL MongoDB
No existing row/doc run regular INSERT insert via upsert
Conflict / match found run DO UPDATE … run UPDATE …

We could put the “excluded” columns in $set, and use $setOnInsert for values present only on insert.
Example command:

  {
    updateOne: {
      filter: { _id: 1 },                
      update: {
      
      // similar to DO UPDATE … SET title = excluded.title
        $set: { title: "Pride & Prejudice" },   
        
        // fields present only on first insert
        
        $setOnInsert: { 
          title: "Pride & Prejudice"                   
          outOfStock: false,
          publishYear: 1813,
          isbn13: NumberLong("9780141439518"),
          discount: 0.2,
          price: Decimal128("23.55")
        }
      },
      upsert: true
    }
  },

2. "on conflict do nothing":

We could use ordered: false for insert operations to process all inserts and ignore any duplication errors reported via writeErrors. For this, though, we’d need to be aware of the HQL context, if possible.

In any case, I suggest we handle this as a separate ticket, so as not to block this PR and to keep the review focused. Let me know your thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitively it deserves a ticket. I think all of the following HQL criteria needs to be satisfied to allow for MQL translation:

  • ON CONFLICT is specified
  • ON CONSTRAINT uniqueIndexName is not used as the criteria (MQL's filter can't be based on unique index name, correct?)
  • CONFLICT ACTION is not NOTHING

so it seems the ticket is an interesting tech one but could be used very rarely in reality; native query seems more flixible and powerful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a ticket here: https://jira.mongodb.org/browse/HIBERNATE-94. Let me know when you have any further suggestion.

}
if (insertStatement.getSourceSelectStatement() != null) {
throw new FeatureNotSupportedException();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

HQL has two flavours of insertion statement:

  • insert into Book (id, title) values (1, "War and Peace"), (2, "Crime and Punishment")
  • insert into Book (id, title) select c.identifier, c.name from Catalog as c

This PR will focus on the first variant and throw exception for the latter.

documents.add(new AstDocument(astElements));
}

astVisitorValueHolder.yield(COLLECTION_MUTATION, new AstInsertCommand(collection, documents));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there are 3 assertion usages above as follows:

  • insertion field list is not empty
  • insertion values list is not empty
  • field list length is the same as values length

The above constraints were either enforced from HQL grammar level or semantic checking phase in Hibernate (a prior step when the final SQL AST translation step is reached), so we can rest assured these constraints are enforced already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

similarly, various assertions are used in updating case as well. Common constraints have been carefully centralized into checkMutationStatementSupportability() method. Only one checking is required to ensure the insertion field value or udpating field value expressions should be either valueExpression (literal value or parameter expressions).

@NathanQingyangXu NathanQingyangXu marked this pull request as ready for review May 29, 2025 13:10
@NathanQingyangXu NathanQingyangXu marked this pull request as draft May 29, 2025 14:21
@NathanQingyangXu NathanQingyangXu force-pushed the HIBERNATE-46 branch 2 times, most recently from 0cc0a35 to 4d24c09 Compare June 11, 2025 18:26
class Book {
@Table(name = Book.COLLECTION)
public class Book {
public static final String COLLECTION = "books";
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this constant? Currently, tests fail if we change it because the collection name is hardcoded in the test assertions (which is okay, as it keeps the test self-sufficient and clear). I think we could stick to one approach - even if that means some duplication of string literals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not necessary but still good to have. Either way is fine. But given we have renamed it, we need to change its references in the other testing cases.
I commited to fix the compiling issues.

if (fromClause.getRoots().size() != 1) {
throw new FeatureNotSupportedException("Only single root from clause is supported");
}
var root = fromClause.getRoots().get(0);
Copy link
Member

Choose a reason for hiding this comment

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

Currently, HQL joins are ignored without an exception being thrown. The root has a hasRealJoins method that detects when a join is present. Should we throw a FeatureNotSupportedException in this case, with a reference to HIBERNATE-65?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Adopted.

checkMutationStatement(insertStatement);
if (insertStatement.getConflictClause() != null) {
throw new FeatureNotSupportedException();
}
Copy link
Member

@vbabanin vbabanin Jun 24, 2025

Choose a reason for hiding this comment

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

I think we could support the same semantics in MQL.

1. "on conflict do update:

What HQL does:
insert … on conflict do update - checks a unique index; if the key is taken it runs the UPDATE clause.

What MongoDB does:
update … { upsert:true } matches a filter; if no doc matches it inserts instead.

Despite starting from opposite directions (insert-first vs. update-first) they end up with the same two-way branch:

Case HQL MongoDB
No existing row/doc run regular INSERT insert via upsert
Conflict / match found run DO UPDATE … run UPDATE …

We could put the “excluded” columns in $set, and use $setOnInsert for values present only on insert.
Example command:

  {
    updateOne: {
      filter: { _id: 1 },                
      update: {
      
      // similar to DO UPDATE … SET title = excluded.title
        $set: { title: "Pride & Prejudice" },   
        
        // fields present only on first insert
        
        $setOnInsert: { 
          title: "Pride & Prejudice"                   
          outOfStock: false,
          publishYear: 1813,
          isbn13: NumberLong("9780141439518"),
          discount: 0.2,
          price: Decimal128("23.55")
        }
      },
      upsert: true
    }
  },

2. "on conflict do nothing":

We could use ordered: false for insert operations to process all inserts and ignore any duplication errors reported via writeErrors. For this, though, we’d need to be aware of the HQL context, if possible.

In any case, I suggest we handle this as a separate ticket, so as not to block this PR and to keep the review focused. Let me know your thoughts.

throw new FeatureNotSupportedException();
}
if (insertStatement.getSourceSelectStatement() != null) {
throw new FeatureNotSupportedException();
Copy link
Member

Choose a reason for hiding this comment

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

Let’s return a descriptive message here; otherwise, it won’t be clear to a user which feature isn’t supported.

import org.junit.jupiter.api.Test;

@DomainModel(annotatedClasses = Book.class)
class InsertionIntegrationTests extends AbstractMutationQueryIntegrationTests {
Copy link
Member

@vbabanin vbabanin Jun 24, 2025

Choose a reason for hiding this comment

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

Do we currently have a ticket to implement writeErrors handling as part of bulkWrite in executeUpdate?

At the moment, duplicate key errors on insert are not handled in executeUpdate, so it’s possible to insert two entities with the same ID without a specific error on the Hibernate side, even though the server returns a duplicate key error.

I think, before we switch executeUpdate to use bulkWrite instead of runCommand, we should add tests for duplicate inserts (annotated as @Disabled and linked to the relevant ticket) to ensure this logic isn’t missed during refactoring.

What do you think?

P.S Later, we may need to override buildSQLExceptionConversionDelegate in MongoDialect to handle exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do have ticket to switch from runCommand to better alternative at https://jira.mongodb.org/browse/HIBERNATE-40, but there is no separate ticket to implement writeErrors.

I did experiment reproducing duplicated insertion as below:

   @Test
    void testDuplicatedInsertion() {
        getSessionFactoryScope().inTransaction(session -> {
            var query = session.createMutationQuery("""
                   insert into Book (id, title, outOfStock, publishYear, isbn13, discount, price)
                        values
                            (1, 'Pride & Prejudice', false, 1813, 9780141439518L, 0.2D, 23.55BD)
                   """);
            query.executeUpdate();
        });
        getSessionFactoryScope().inTransaction(session -> {
            var query = session.createMutationQuery("""
                   insert into Book (id, title, outOfStock, publishYear, isbn13, discount, price)
                        values
                            (1, 'Pride & Prejudice', false, 1813, 9780141439518L, 0.2D, 23.55BD)
                   """);
            query.executeUpdate();
        });
    }

Below is the current stack trace:

Failed to commit transaction
java.sql.SQLException: Failed to commit transaction
	at com.mongodb.hibernate.jdbc.MongoConnection.doCommitIfNeeded(MongoConnection.java:88)
	at com.mongodb.hibernate.jdbc.MongoConnection.commit(MongoConnection.java:78)
	at org.hibernate.resource.jdbc.internal.AbstractLogicalConnectionImplementor.commit(AbstractLogicalConnectionImplementor.java:87)
	at org.hibernate.resource.transaction.backend.jdbc.internal.JdbcResourceLocalTransactionCoordinatorImpl$TransactionDriverControlImpl.commit(JdbcResourceLocalTransactionCoordinatorImpl.java:268)
	at org.hibernate.engine.transaction.internal.TransactionImpl.commit(TransactionImpl.java:101)
	at org.hibernate.testing.orm.transaction.TransactionUtil.wrapInTransaction(TransactionUtil.java:54)
	at org.hibernate.testing.orm.transaction.TransactionUtil.inTransaction(TransactionUtil.java:24)
	at org.hibernate.testing.orm.junit.SessionFactoryExtension$SessionFactoryScopeImpl.inTransaction(SessionFactoryExtension.java:376)
	at org.hibernate.testing.orm.junit.SessionFactoryExtension$SessionFactoryScopeImpl.inTransaction(SessionFactoryExtension.java:353)
	at com.mongodb.hibernate.query.mutation.InsertionIntegrationTests.testDuplicatedInsertion(InsertionIntegrationTests.java:155)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: com.mongodb.MongoCommandException: Command failed with error 251 (NoSuchTransaction): 'Transaction with { txnNumber: 2 } has been aborted.' on server localhost:27017. The full response is {"errorLabels": ["TransientTransactionError"], "ok": 0.0, "errmsg": "Transaction with { txnNumber: 2 } has been aborted.", "code": 251, "codeName": "NoSuchTransaction", "$clusterTime": {"clusterTime": {"$timestamp": {"t": 1750774932, "i": 2}}, "signature": {"hash": {"$binary": {"base64": "AAAAAAAAAAAAAAAAAAAAAAAAAAA=", "subType": "00"}}, "keyId": 0}}, "operationTime": {"$timestamp": {"t": 1750774932, "i": 2}}}
	at app//com.mongodb.internal.connection.ProtocolHelper.getCommandFailureException(ProtocolHelper.java:210)
	at app//com.mongodb.internal.connection.InternalStreamConnection.receiveCommandMessageResponse(InternalStreamConnection.java:520)
	at app//com.mongodb.internal.connection.InternalStreamConnection.sendAndReceiveInternal(InternalStreamConnection.java:448)
	at app//com.mongodb.internal.connection.InternalStreamConnection.lambda$sendAndReceive$0(InternalStreamConnection.java:375)
	at app//com.mongodb.internal.connection.InternalStreamConnection.sendAndReceive(InternalStreamConnection.java:378)
	at app//com.mongodb.internal.connection.UsageTrackingInternalConnection.sendAndReceive(UsageTrackingInternalConnection.java:111)
	at app//com.mongodb.internal.connection.DefaultConnectionPool$PooledConnection.sendAndReceive(DefaultConnectionPool.java:747)
	at app//com.mongodb.internal.connection.CommandProtocolImpl.execute(CommandProtocolImpl.java:61)
	at app//com.mongodb.internal.connection.DefaultServer$DefaultServerProtocolExecutor.execute(DefaultServer.java:208)
	at app//com.mongodb.internal.connection.DefaultServerConnection.executeProtocol(DefaultServerConnection.java:112)
	at app//com.mongodb.internal.connection.DefaultServerConnection.command(DefaultServerConnection.java:82)
	at app//com.mongodb.internal.connection.DefaultServerConnection.command(DefaultServerConnection.java:74)
	at app//com.mongodb.internal.connection.DefaultServer$OperationCountTrackingConnection.command(DefaultServer.java:298)
	at app//com.mongodb.internal.operation.SyncOperationHelper.lambda$executeRetryableWrite$10(SyncOperationHelper.java:267)
	at app//com.mongodb.internal.operation.SyncOperationHelper.lambda$withSourceAndConnection$0(SyncOperationHelper.java:131)
	at app//com.mongodb.internal.operation.SyncOperationHelper.withSuppliedResource(SyncOperationHelper.java:156)
	at app//com.mongodb.internal.operation.SyncOperationHelper.lambda$withSourceAndConnection$1(SyncOperationHelper.java:130)
	at app//com.mongodb.internal.operation.SyncOperationHelper.withSuppliedResource(SyncOperationHelper.java:156)
	at app//com.mongodb.internal.operation.SyncOperationHelper.withSourceAndConnection(SyncOperationHelper.java:129)
	at app//com.mongodb.internal.operation.SyncOperationHelper.lambda$executeRetryableWrite$11(SyncOperationHelper.java:252)
	at app//com.mongodb.internal.operation.SyncOperationHelper.lambda$decorateWriteWithRetries$12(SyncOperationHelper.java:308)
	at app//com.mongodb.internal.async.function.RetryingSyncSupplier.get(RetryingSyncSupplier.java:67)
	at app//com.mongodb.internal.operation.SyncOperationHelper.executeRetryableWrite(SyncOperationHelper.java:279)
	at app//com.mongodb.internal.operation.TransactionOperation.execute(TransactionOperation.java:60)
	at app//com.mongodb.internal.operation.CommitTransactionOperation.execute(CommitTransactionOperation.java:69)
	at app//com.mongodb.internal.operation.CommitTransactionOperation.execute(CommitTransactionOperation.java:48)
	at app//com.mongodb.client.internal.MongoClusterImpl$OperationExecutorImpl.execute(MongoClusterImpl.java:446)
	at app//com.mongodb.client.internal.ClientSessionImpl.commitTransaction(ClientSessionImpl.java:204)
	at app//com.mongodb.client.internal.ClientSessionImpl.commitTransaction(ClientSessionImpl.java:112)
	at app//com.mongodb.hibernate.jdbc.MongoConnection.doCommitIfNeeded(MongoConnection.java:86)
	... 12 more

So it seems we can get clue from the root cause returned from Mongo Java driver?
What is your suggestion to improve on the current behaviour?

Copy link
Member

@vbabanin vbabanin Jun 24, 2025

Choose a reason for hiding this comment

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

Yes, we can get more details from the root cause returned by the driver. The BulkWriteResult contains writeErrors, which includes error codes. Currently, when a transaction is aborted, the message doesn’t clarify what went wrong.

Hibernate ORM throws ConstraintViolationException for unique constraint violations - we should align with that behavior.

This can be achieved by handling writeErrors and throw an exception with the appropriate code. This can then be mapped in SQLExceptionConversionDelegate to the relevant Hibernate exception for user visibility.

When an exception is thrown from executeUpdate, Hibernate calls rollback on MongoConnection as expected, rather than attempting to commit a transaction that no longer exists.

Here’s a draft commit showing the relevant test and changes for illustration: 057bc0e

For this PR, I propose:

  1. Add a test to cover ConstraintViolationException (to be disabled until writeError handling is implemented).

  2. Throw SQLException with an error message and MongoDB error code, Hibernate should convert it to JdbcGenericException. Specific handling of write error codes can be addressed in a separate ticket during implementation of HIBERNATE-40.

I have created a separate ticket for proper implementation: HIBERNATE-95 later.

Let me know your thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great!! That makes sense. I commit at 826df65. Further details like the exception message is better to be decided when we really start working on https://jira.mongodb.org/browse/HIBERNATE-95

@NathanQingyangXu NathanQingyangXu marked this pull request as ready for review July 2, 2025 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants