implement loading by primary key #69

NathanQingyangXu · 2025-03-17T01:19:51Z

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

The first "select statement" translation ticket, though the simplest one (where clause only contains primary key equalness comparison). Very business logic intensitve PR.

I added comments to explain the various edge cases of business logic and hope that would help.

src/integrationTest/java/com/mongodb/hibernate/BasicCrudIntegrationTests.java

src/integrationTest/resources/logback-test.xml

NathanQingyangXu · 2025-03-17T13:30:31Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

        var astElements = new ArrayList<AstElement>(tableInsert.getNumberOfValueBindings());
        for (var columnValueBinding : tableInsert.getValueBindings()) {
            var columnExpression = columnValueBinding.getColumnReference().getColumnExpression();

            var valueExpression = columnValueBinding.getValueExpression();
-            if (valueExpression == null) {
-                throw new FeatureNotSupportedException();
-            }


after further investigation, it seems we can rule out the above possibility (only table updating has such concern for some reason), so I deleted it to avoid long-term potential confusion.

only table updating has such concern

What is that "table updating", and why it can never happen to us? Also, if we can see why it can never happen, but it's a thing nevertheless generally speaking, we should leave an explicit assertion, informing a reader that we did not miss the case when columnValueBinding.getValueExpression() is null, but thought about it and claim that it will never happen to us.

Below is the scenario that columnValueBinding.getValueExpression() could be null (https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/java/org/hibernate/sql/ast/spi/AbstractSqlAstTranslator.java#L8745C6-L8745C58):

if ( tableUpdate.getNumberOfOptimisticLockBindings() > 0 ) { tableUpdate.forEachOptimisticLockBinding( (position, columnValueBinding) -> { sqlBuffer.append( " and " ); sqlBuffer.append( columnValueBinding.getColumnReference().getColumnExpression() ); if ( columnValueBinding.getValueExpression() == null || columnValueBinding.getValueExpression().getFragment() == null ) { sqlBuffer.append( " is null" ); } else { sqlBuffer.append( "=" ); columnValueBinding.getValueExpression().accept( this ); } } ); }

The reason is the requirement of SQL's special nullness syntax (i.e. xxx is null), so if columnValueBinding.getValueExpression() is a parameter placeholder, there is no accommodation of the nullness SQL requirement or not easy; for insertion case, there seems no such possibility, so that is why I claimed that we might be able to remove the unnecessary nullness checking.

So it seems it doesn't hurt to retain the nullness checking in insertion case. I reverted it back.

NathanQingyangXu · 2025-03-17T14:16:47Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

+                || selectStatement.getCteObjects() != null
+                        && !selectStatement.getCteObjects().isEmpty()) {
+            throw new FeatureNotSupportedException("CTE feature not supported");
+        }


CTE (Common Table Expression; See https://hightouch.com/sql-dictionary/sql-common-table-expression-cte for a gentle introduction to CTE) is about creating some temporary table. Mongo aggregate command seems to support it, but for this PR let us throw exception for now.

NathanQingyangXu · 2025-03-17T14:20:53Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

+                        && !selectStatement.getCteObjects().isEmpty()) {
+            throw new FeatureNotSupportedException("CTE feature not supported");
+        }
+        selectStatement.getQueryPart().accept(this);


QueryPart has two child classes:

QueryGroup

QuerySpec

The latter will be covered in the visitor method below; QueryGroup is used for a set of QuerySpec and how to combine them together. A typical example is as follows:

SELECT column_name(s) FROM table1 UNION SELECT column_name(s) FROM table2;

NathanQingyangXu · 2025-03-17T15:05:53Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

-        throw new FeatureNotSupportedException("TODO-HIBERNATE-22 https://jira.mongodb.org/browse/HIBERNATE-22");
+        logSqlAst(selectStatement);
+        if (!selectStatement.getQueryPart().isRoot()) {
+            throw new FeatureNotSupportedException("Subquery not supported");


a subquery example:

select d from Department as d where d in (select s.department from Salesperson s where s.name = ?1)"

…ommandTests; improve edge case triage logic

NathanQingyangXu · 2025-03-17T18:49:25Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

+    public void visitFromClause(FromClause fromClause) {
+        if (fromClause.getRoots().size() != 1) {
+            throw new FeatureNotSupportedException();
+        }


An example HQL in which there are multipe roots:

select li.id from LineItem li, Product as p where p.id = li.product.id and li.quantity >= ?1 and p.name = ?2

NathanQingyangXu · 2025-03-17T19:07:27Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

+
+        if (!(tableGroup.getModelPart() instanceof EntityPersister entityPersister)
+                || entityPersister.getQuerySpaces().length != 1) {
+            throw new FeatureNotSupportedException();


tableGroup's modelPart doesn't necessarily corresponds to an entity table (EntityPersister class corresponds to it), for instance, for @OneToMany association, the collection field will have its SQL translated and its model part will be a special PluralAttributeMappingImpl class object containing foreign key specific logic.

When model part is an EntityPersister corresponding to an entity, not entity collection, there could be the possibility that the entity class hierarchy ends up with multiple tables (getQuerySpaces() will return all the relevant table names during query) corresponding to the "joinied table" mapping option (see https://docs.jboss.org/hibernate/orm/6.6/userguide/html_single/Hibernate_User_Guide.html#entity-inheritance for details)

NathanQingyangXu · 2025-03-17T19:09:03Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

+
+        for (SqlSelection sqlSelection : selectClause.getSqlSelections()) {
+            if (sqlSelection.isVirtual()) {
+                continue;


There is some edge case that some SqlSelection entry exists for some reason but are not meant to be used in SQL. For such virtual entries, we simply ignore during SQL translation (a pattern also in AbstractSqlTranslator).

jyemin

Just did an initial pass of the core functionality, and had a few comments.

src/integrationTest/java/com/mongodb/hibernate/BasicCrudIntegrationTests.java

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

stIncMale

Partial review results. I am submitting them only because it's the only way to leave a comment in an existing discussion.

src/integrationTest/java/com/mongodb/hibernate/BasicCrudIntegrationTests.java

src/main/java/com/mongodb/hibernate/internal/translate/TableMutationMqlTranslator.java

stIncMale · 2025-03-19T19:18:46Z

...ernate/internal/translate/mongoast/command/aggregate/stage/AstProjectStageSpecification.java

+
+public abstract class AstProjectStageSpecification implements AstNode {
+
+    public static AstProjectStageSpecification include(String path) {


What are the possible situations when we would have to use anything but a field name here? If we actually specify a path like "contact.phone.number", that does not result in having a field named "contact.phone.number", but simply explicitly includes in the result the number field of the phone document embedded in the contact document.

I fail to imagine a situation where we would need this behavior. Also MongoStatement.getFieldNamesFromProjectStage does not correctly handle this, and incorrectly includes the "contact.phone.number" field name, instead of the contact field name.

We need this in the future table joining scenario. There are intermediate steps ($lookup and $unwind) ending up with deep nested documents and we rely on the path to locate relevant field. Our tech design doc provides such field path example.

Our tech design doc provides such field path example.

There is a single example in the tech design doc that uses $lookup and $unwind, but it does not use field path in the $project specifications the way AstProjectStageSpecification does.

It is in the $project section as below (f2, f3, f4, f5 are relying on a path):

{ "$project":{ "f0":"$_id", "f1":"$name", "f2":"$p1_0._id", "f3":"$p1_0.c2_0._id", "f4":"$p1_0.c2_0.name", "f5":"$p1_0.name", "_id":0 } }

btw, In POC, random field names are used and expression is used for setting values to them. That is why i was motivated to relax the limit of our ResultSet PR logic; but the choice is whimscal and I fail to see why not switch to inclusion list.

I, of course, saw that $project stage example, as I indicated above. But, also, as I said, it does not use field path the way AstProjectStageSpecification does:

AstProjectStageSpecification uses field paths as keys, with value being always true;

the example uses field paths in field path expressions, i.e., as values in the $project specifications, with keys being f0, ....

These are not equivalent, I mentioned the difference in the first comment above #69 (comment). And AstProjectStageSpecification not only does not support what we have in the example in the design doc, but also allows us to do something that I am not sure we need, which is why my first comment was

I fail to imagine a situation where we would need this behavior. Also MongoStatement.getFieldNamesFromProjectStage does not correctly handle this, and incorrectly includes the "contact.phone.number" field name, instead of the contact field name.

P.S. I struggle to understand why situations like this keep happening quite often. Was the first comment I left actually so indecipherable, or was it not read carefully?

I meant the above $project stage could be rewritten using inclusion list as below:

"$project":{ "_id": true, "name": true, "p1_0._id": true, "p1_0.c2_0._id": true, "p1_0.c2_0.name": true, "p1_0.name": true, }

I didn't explain fully, but I did hint that it could explain why a field path is required.

Sorry for the misunerstanding. I understand your comment but I might not convey my meaning completely. I think in scenario when people have dramatically different knowledge background, misunderstanding is common. I only hope as time goes on it will become less and less.

I think we do rely on contact.phone.number field name in table joining scenario. I thought I could prove that by running POC testing case at https://github.com/NathanQingyangXu/jpa-mongodb-mapping/blob/main/chameleon-core/src/test/java/org/hibernate/omm/join/SimpleJoinTests.java

I experimented using contact.phone.number pattern in POC project and found it won't work as I had expected. it won't end up with a single field but an embedded doc. So you are right. contact.phone.number won't work in the table joining case and the new field by setting an expression is the only way to go.

Sorry about that. I apologize for my lack of domain knowledge. I think field path won't apply in our table joining case. But is it possible that it could apply in other scenario (e.g. STRUCT or embedded document in m2)? Should we still retain the fieldPath naming then?

...ernate/internal/translate/mongoast/command/aggregate/stage/AstProjectStageSpecification.java

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

stIncMale

The review hasn't been completed. Leaving partial results again to comment on an existing discussion.

...java/com/mongodb/hibernate/internal/translate/mongoast/command/AstAggregateCommandTests.java

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

…ationTests.java Co-authored-by: Valentin Kovalenko <[email protected]>

build.gradle.kts

src/integrationTest/java/com/mongodb/hibernate/BasicCrudIntegrationTests.java

# Conflicts: # build.gradle.kts # gradle/libs.versions.toml

…tMqlTranslator.java Co-authored-by: Valentin Kovalenko <[email protected]>

…tSelectClause()

…eterBindings is non-null in SelectStatementMqlTranslator#translate() method

stIncMale

The last reviewed commit is 6d0985a, but I haven't reviewed src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java.

For now, I will be reviewing the new changes.

src/main/java/com/mongodb/hibernate/internal/translate/SelectStatementMqlTranslator.java

…meters

…e similar to real usage (some fields were totally wrong)

# Conflicts: # src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java # src/main/java/com/mongodb/hibernate/internal/translate/mongoast/command/AstUpdateCommand.java

…w' into HIBERNATE-22-load-translation-new

stIncMale · 2025-03-22T00:56:37Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

@@ -143,6 +159,8 @@ abstract class AbstractMqlTranslator<T extends JdbcOperation> implements SqlAstT

    private final List<JdbcParameterBinder> parameterBinders = new ArrayList<>();

+    private final Set<String> affectedTableNames = new HashSet<>();


I started thinking on which of the classes we plug in Hibernate ORM have to be thread-safe, and which are used multiple times - it's not obvious, and is, of course, not documented by Hibernate ORM.

MongoDialect - only one instance is created. Does this mean it has to be thread-safe, or does Hibernate ORM guarantee not to use it concurrently?

MongoTranslatorFactory - essentially, Hibernate ORM uses a single instance, but creates two due to this sloppy code. As far as I understand, this instance should be thread-safe.

SqlAstTranslators created by MongoTranslatorFactory
3.1. It looks like a new instance of SelectStatementMqlTranslator is created for each query that is different in any way from those previously translated, even if the difference is in QueryOptions. Furthermore, SelectStatementMqlTranslator.translate is called only once per instance of SelectStatementMqlTranslator. That is, it looks like SelectStatementMqlTranslator does not need to be thread-safe, nor does it need to be reusable, i.e., we are fine with making it stateful and not resetting the state before returning from the SelectStatementMqlTranslator.translate method.
3.2. Is the same true about all translators created by MongoTranslatorFactory?

StandardServiceRegistryScopedState.ServiceContributor.contribute - may be called multiple times if multiple instances of StandardServiceRegistry are built.

MongoAdditionalMappingContributor - TODO for @stIncMale.

We should document (for ourselves) the above, provided that the above is correct. @NathanQingyangXu, do you think the above assessment is correct?

A similar thing is about the SPI we expose:

MongoConfigurationContributor - its method configure is called once per building an instance of StandardServiceRegistry. We should document this, because this affects both reusability requirements applied to implementations, and only an application author may figure out based on the application whether his MongoConfigurationContributor has to be reusable.

I created PR 76: Add MongoConfigurationContributor tests and document it better to address this part.

With our JDBC implementations things are simple, because we are familiar with the JDBC API, and are not familiar with the Hibernate ORM extension API, which is not documented extensively - none of them are used concurrently, i.e., they don't need to be thread-safe.

I think what you think is correct, except for the point to create multiple StandardServiceRegistry instances (I explained about it below).

As a general rule, all those class related to global configuration will end up with singleton instance (similar to Spring bean). Technically these classes are not thread-safe and it is developer's responsibility to ensure thread-safety (e.g. Dialect singleton instance could be accessed from different concurrent sessions at the same time), but usually these classes don't maintain their own state and is read-only in essence, so it is very rare to see concurrency related stuff is there.

Only for the global SessionFactory scope, concurrency is a relevant concern (like QueryPlan cache, which is based on double lock checking to ensure thread-safety).

Yeah, JDBC APIs will be used in Session which is thread-safe, so we don't need bother with concurrency issue in JDBC layer.

The various translators would be created dynamically as the following javadoc implies (by single-use verbiage):

/** * Factory for obtaining single-use SQL AST translators * * @author Steve Ebersole */ public interface SqlAstTranslatorFactory { /** * Builds a single-use select translator */ SqlAstTranslator<JdbcOperationQuerySelect> buildSelectTranslator(SessionFactoryImplementor sessionFactory, SelectStatement statement); /** * Builds a single-use mutation translator */ SqlAstTranslator<? extends JdbcOperationQueryMutation> buildMutationTranslator(SessionFactoryImplementor sessionFactory, MutationStatement statement); /** * Builds a single-use delete translator * * @deprecated Use {@link #buildMutationTranslator(SessionFactoryImplementor, MutationStatement)} instead */ @Deprecated(forRemoval = true) default SqlAstTranslator<JdbcOperationQueryDelete> buildDeleteTranslator(SessionFactoryImplementor sessionFactory, DeleteStatement statement) { //noinspection unchecked return (SqlAstTranslator<JdbcOperationQueryDelete>) buildMutationTranslator( sessionFactory, statement ); } /** * Builds a single-use insert-select translator * * @deprecated Use {@link #buildMutationTranslator(SessionFactoryImplementor, MutationStatement)} instead */ @Deprecated(forRemoval = true) default SqlAstTranslator<JdbcOperationQueryInsert> buildInsertTranslator(SessionFactoryImplementor sessionFactory, InsertStatement statement) { //noinspection unchecked return (SqlAstTranslator<JdbcOperationQueryInsert>) buildMutationTranslator( sessionFactory, statement ); } /** * Builds a single-use update translator * * @deprecated Use {@link #buildMutationTranslator(SessionFactoryImplementor, MutationStatement)} instead */ @Deprecated(forRemoval = true) default SqlAstTranslator<JdbcOperationQueryUpdate> buildUpdateTranslator(SessionFactoryImplementor sessionFactory, UpdateStatement statement) { //noinspection unchecked return (SqlAstTranslator<JdbcOperationQueryUpdate>) buildMutationTranslator( sessionFactory, statement ); } /** * Builds a single-use translator for dealing with model mutations */ <O extends JdbcMutationOperation> SqlAstTranslator<O> buildModelMutationTranslator(TableMutation<O> mutation, SessionFactoryImplementor sessionFactory); }

I failed to see the necessity of multiple instances of StandardServiceRegistry. It should be created as singleton for one SessionFactory as well.

If our user is tech-savy, I think such doc might only end up with confusion.

sorry, if the doc is for our own side, go ahead

…nd SelectMqlTranslator

...internal/translate/mongoast/command/aggregate/AstProjectStageIncludeSpeicificationTests.java

stIncMale · 2025-03-24T18:18:21Z

...internal/translate/mongoast/command/aggregate/AstProjectStageIncludeSpeicificationTests.java

+        var expectedJson = """
+                           {"$project": {"name": true}}\
+                           """;
+        assertRender(expectedJson, astProjectStage);


This tests AstProjectStage, not AstProjectStageIncludeSpecification. AstProjectStageIncludeSpeicificationTests.testRendering does not need to involve AstProjectStage.

It was because assertRender() won't work for name: true by throwing exception with messsage:

A Name value cannot be written to the root level of a BSON document.

how to verify the rendering in such case?

I introduced a assertNameValueRender() method in AstNodeAssertions and it will handle the case.

stIncMale · 2025-03-24T19:37:17Z

...ernate/internal/translate/mongoast/command/aggregate/stage/AstProjectStageSpecification.java

+
+public abstract class AstProjectStageSpecification implements AstNode {
+
+    public static AstProjectStageSpecification include(String path) {


I, of course, saw that $project stage example, as I indicated above. But, also, as I said, it does not use field path the way AstProjectStageSpecification does:

AstProjectStageSpecification uses field paths as keys, with value being always true;

the example uses field paths in field path expressions, i.e., as values in the $project specifications, with keys being f0, ....

These are not equivalent, I mentioned the difference in the first comment above #69 (comment). And AstProjectStageSpecification not only does not support what we have in the example in the design doc, but also allows us to do something that I am not sure we need, which is why my first comment was

I fail to imagine a situation where we would need this behavior. Also MongoStatement.getFieldNamesFromProjectStage does not correctly handle this, and incorrectly includes the "contact.phone.number" field name, instead of the contact field name.

P.S. I struggle to understand why situations like this keep happening quite often. Was the first comment I left actually so indecipherable, or was it not read carefully?

stIncMale · 2025-03-24T20:04:36Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

            astElements.add(new AstElement(fieldName, fieldValue));
        }
        astVisitorValueHolder.yield(
-                COLLECTION_MUTATION, new AstInsertCommand(tableInsert.getTableName(), new AstDocument(astElements)));
+                COLLECTION_MUTATION,
+                new AstInsertCommand(tableInsert.getMutatingTable().getTableName(), new AstDocument(astElements)));


Previosely we had getTableName() instead of getMutatingTable().getTableName(). These seem to be functionally equivalent. Was this change a matter of taste, or is it functional?

Internally they are equivalent in function. I changed it to maintain consistency between table insertion and updating.

…ableInsert()

stIncMale

The last reviewed commit is 4fca70d.

stIncMale · 2025-03-24T22:54:03Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

@@ -301,7 +316,106 @@ public void visitParameter(JdbcParameter jdbcParameter) {

    @Override
    public void visitSelectStatement(SelectStatement selectStatement) {
-        throw new FeatureNotSupportedException("TODO-HIBERNATE-22 https://jira.mongodb.org/browse/HIBERNATE-22");
+        logSqlAst(selectStatement);


If we do it in visitSelectStatement, we should similarly do it in

visitInsertStatement, visitDeleteStatement, visitUpdateStatement

visitStandardTableInsert, visitStandardTableDelete, visitStandardTableUpdate

But maybe, even better, we should do this in just three places:

SelectMqlTranslator.translate, ModelMutationMqlTranslator.translate, MutationMqlTranslator.translate (this one does not yet exist).

Let's do the second option?

logSqlAst has limit in that it only supports query statement (using it on ModelMutationMqlTranslate#translate() will end up Hibernate launching failure.

Currently we only have a dummy NoopSqlAstTranslator for MutationMqlTranslator so currently only SelectMqlTranslator can use it. I moved it there.

stIncMale · 2025-03-25T00:50:24Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

+        }
+        if (querySpec.hasSortSpecifications()) {
+            throw new FeatureNotSupportedException("Sorting not supported");
+        }


Why are we not checking querySpec.hasOffsetOrFetchClause()?

Good catch. I overlooked. Added.

stIncMale · 2025-03-25T01:53:32Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

+    }
+
+    @Override
+    public void visitSelectClause(SelectClause selectClause) {


Should we check selectClause.isDistinct()?

we should. Added.

stIncMale · 2025-03-25T04:38:59Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

+            throw new FeatureNotSupportedException();
+        }
+
+        affectedTableNames.add(((String[]) entityPersister.getQuerySpaces())[0]);


Should we check that the size of entityPersister.getQuerySpaces() is 1?

It has been checked already in the preceding code block:

if (!(tableGroup.getModelPart() instanceof EntityPersister entityPersister) || entityPersister.getQuerySpaces().length != 1) { throw new FeatureNotSupportedException(); }

stIncMale · 2025-03-25T04:40:28Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

+        }
+
+        affectedTableNames.add(((String[]) entityPersister.getQuerySpaces())[0]);
+        tableGroup.getPrimaryTableReference().accept(this);


TableGroup has many more parts than just getModelPart and getPrimaryTableReference. Do we need to check then and fail if they are present?

Yeah, TableGroup has many other fields, but they have been ruled out by the following validation logic:

if (!(tableGroup.getModelPart() instanceof EntityPersister entityPersister) || entityPersister.getQuerySpaces().length != 1) { throw new FeatureNotSupportedException(); }

because the first checking ensures that the table group is not collection table group (e.g. @OneToMany, @ManyToMany), and the second checking ensures that no table joining for otherwise the QuerySpace wont only have one table. Also, if only one table is involved, I think we can ignore the alias fields as well.

I struggled a lot on how to best limit the scope to our scenario, and I think the above two seem effective.

stIncMale · 2025-03-25T06:13:16Z

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java

+        if (queryOptions.getFetchSize() != null) {
+            throw new FeatureNotSupportedException("TO-DO-HIBERNATE-54 https://jira.mongodb.org/browse/HIBERNATE-54");
+        }
+        if (queryOptions.getLimit() != null && !queryOptions.getLimit().isCompatible(Limit.NONE)) {


Why not check queryOptions.isEmpty instead?

yeah, queryOptions.getLimit().isEmpty() suffices. Changed.

…t/command/aggregate/AstProjectStageIncludeSpeicificationTests.java Co-authored-by: Valentin Kovalenko <[email protected]>

…anslator#visitQuerySpec

…visitSelectClause()

…ng 'queryOptions.getLimit().isEmpty()'

NathanQingyangXu · 2025-03-25T14:43:04Z

P.S. I struggle to understand why situations like this keep happening quite often. Was the first comment I left actually so indecipherable, or was it not read carefully?

It is mainly due to the knowledge gap from my side. I didn't know of the subtlety of contact.phone.number until I experiemented locally and afterwards read the relevant mongo doc.

…deAssertions#assertNameValueRender() method

NathanQingyangXu requested a review from jyemin March 17, 2025 01:19

NathanQingyangXu commented Mar 17, 2025

View reviewed changes

src/integrationTest/java/com/mongodb/hibernate/BasicCrudIntegrationTests.java Show resolved Hide resolved

NathanQingyangXu commented Mar 17, 2025

View reviewed changes

src/integrationTest/java/com/mongodb/hibernate/BasicCrudIntegrationTests.java Outdated Show resolved Hide resolved

NathanQingyangXu commented Mar 17, 2025

View reviewed changes

src/integrationTest/resources/logback-test.xml Show resolved Hide resolved

implement loading by primary key

9cb4599

NathanQingyangXu force-pushed the HIBERNATE-22-load-translation-new branch from 182d8fc to 9cb4599 Compare March 17, 2025 01:28

stIncMale self-requested a review March 17, 2025 02:13

NathanQingyangXu commented Mar 17, 2025

View reviewed changes

add testing case to cover AstMatchesEverythingFilter in AstAggregateC…

e646199

…ommandTests; improve edge case triage logic

NathanQingyangXu commented Mar 17, 2025

View reviewed changes

jyemin reviewed Mar 18, 2025

View reviewed changes

NathanQingyangXu requested a review from jyemin March 19, 2025 14:48

jyemin requested changes Mar 19, 2025

View reviewed changes

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java Show resolved Hide resolved

stIncMale requested changes Mar 19, 2025

View reviewed changes

add unit testing case to verify affectedTableNames content

fcf5e76

NathanQingyangXu commented Mar 19, 2025

View reviewed changes

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java Outdated Show resolved Hide resolved

jyemin self-requested a review March 19, 2025 22:53

jyemin requested changes Mar 19, 2025

View reviewed changes

src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java Outdated Show resolved Hide resolved

stIncMale requested changes Mar 19, 2025

View reviewed changes

NathanQingyangXu and others added 2 commits March 19, 2025 19:33

Update src/integrationTest/java/com/mongodb/hibernate/BasicCrudIntegr…

e4445b3

…ationTests.java Co-authored-by: Valentin Kovalenko <[email protected]>

use get() instead of load() in loading by primary key testing case

6d0985a

stIncMale reviewed Mar 20, 2025

View reviewed changes

build.gradle.kts Outdated Show resolved Hide resolved

stIncMale reviewed Mar 20, 2025

View reviewed changes

src/integrationTest/java/com/mongodb/hibernate/BasicCrudIntegrationTests.java Show resolved Hide resolved

stIncMale reviewed Mar 20, 2025

View reviewed changes

src/integrationTest/java/com/mongodb/hibernate/BasicCrudIntegrationTests.java Outdated Show resolved Hide resolved

improve as per code review comments

6837cdc

NathanQingyangXu and others added 6 commits March 20, 2025 11:52

Merge branch 'main' into HIBERNATE-22-load-translation-new

e26b1a4

# Conflicts: # build.gradle.kts # gradle/libs.versions.toml

resolve conflict with main branch

9a15a74

Update src/main/java/com/mongodb/hibernate/internal/translate/Abstrac…

d266dd7

…tMqlTranslator.java Co-authored-by: Valentin Kovalenko <[email protected]>

rename columnName variable to fieldPath in AbstractMqlTranslator#visi…

1040ff3

…tSelectClause()

remove unnecessary AstMatchesEverythingFilter

5724937

add logic to throw FeatureNotSupportedException in event of jdbcParam…

6e14c1b

…eterBindings is non-null in SelectStatementMqlTranslator#translate() method

stIncMale requested changes Mar 20, 2025

View reviewed changes

src/main/java/com/mongodb/hibernate/internal/translate/SelectStatementMqlTranslator.java Outdated Show resolved Hide resolved

src/main/java/com/mongodb/hibernate/internal/translate/SelectStatementMqlTranslator.java Outdated Show resolved Hide resolved

NathanQingyangXu added 4 commits March 20, 2025 16:16

add validation to SelectStatementMqlTranslator#translate()'s two para…

2a3ccf2

…meters

get rid of usage of internal fastSessionServices; keep from exposing mql

21b832d

improve SelectStatementMqlTranslatorTests to make the table group mor…

31268bf

…e similar to real usage (some fields were totally wrong)

remove stage package

8381eab

NathanQingyangXu mentioned this pull request Mar 21, 2025

Minor improvements to AST stuff (generics usage and naming) #73

Merged

NathanQingyangXu and others added 3 commits March 21, 2025 13:55

Merge branch 'main' into HIBERNATE-22-load-translation-new

9b15ee8

Merge branch 'main' into HIBERNATE-22-load-translation-new

1fb7049

# Conflicts: # src/main/java/com/mongodb/hibernate/internal/translate/AbstractMqlTranslator.java # src/main/java/com/mongodb/hibernate/internal/translate/mongoast/command/AstUpdateCommand.java

Merge remote-tracking branch 'origin/HIBERNATE-22-load-translation-ne…

d4bf619

…w' into HIBERNATE-22-load-translation-new

stIncMale reviewed Mar 22, 2025

View reviewed changes

NathanQingyangXu added 3 commits March 24, 2025 09:18

rename translators as per factory method names

cd59077

refactor AstProjectStageSpecification to align with existing pattern

a5f72d3

add QueryOptions assertion logic to both ModelMutationMqlTranslator a…

15f56bf

…nd SelectMqlTranslator

stIncMale requested changes Mar 24, 2025

View reviewed changes

stIncMale reviewed Mar 24, 2025

View reviewed changes

revert back nullness checking in AbstractMqlTranslator#visitStandardT…

4fca70d

…ableInsert()

stIncMale requested changes Mar 25, 2025

View reviewed changes

NathanQingyangXu and others added 5 commits March 25, 2025 09:42

Update src/test/java/com/mongodb/hibernate/internal/translate/mongoas…

bf02e2d

…t/command/aggregate/AstProjectStageIncludeSpeicificationTests.java Co-authored-by: Valentin Kovalenko <[email protected]>

move logSqlAst to SelectMqlTranslator#translate() method

8f834b2

add validation of QuerySpec#hasOffsetOrFetchClause() in AbstractMqlTr…

6fa73e1

…anslator#visitQuerySpec

add validation of SelectClause#isDistinct() in AbstractMqlTranslator#…

2c26854

…visitSelectClause()

simplify AbstractMqlTranslator#checkQueryOptionsSupportability by usi…

d1e68a1

…ng 'queryOptions.getLimit().isEmpty()'

improve AstProjectStageIncludeSpecificationTests by introducing AstNo…

e7c6c80

…deAssertions#assertNameValueRender() method


		public abstract class AstProjectStageSpecification implements AstNode {

		public static AstProjectStageSpecification include(String path) {

		@@ -143,6 +159,8 @@ abstract class AbstractMqlTranslator<T extends JdbcOperation> implements SqlAstT

		private final List<JdbcParameterBinder> parameterBinders = new ArrayList<>();

		private final Set<String> affectedTableNames = new HashSet<>();

implement loading by primary key #69

Are you sure you want to change the base?

implement loading by primary key #69

Conversation

NathanQingyangXu commented Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyemin left a comment

Choose a reason for hiding this comment

stIncMale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NathanQingyangXu Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

stIncMale Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

NathanQingyangXu Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stIncMale left a comment

Choose a reason for hiding this comment

stIncMale left a comment

Choose a reason for hiding this comment

stIncMale Mar 22, 2025 • edited Loading

Choose a reason for hiding this comment

NathanQingyangXu Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stIncMale Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stIncMale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NathanQingyangXu commented Mar 25, 2025

NathanQingyangXu commented Mar 17, 2025 •

edited

Loading

NathanQingyangXu Mar 24, 2025 •

edited

Loading

stIncMale Mar 24, 2025 •

edited

Loading

NathanQingyangXu Mar 25, 2025 •

edited

Loading

stIncMale Mar 22, 2025 •

edited

Loading

NathanQingyangXu Mar 24, 2025 •

edited

Loading

stIncMale Mar 24, 2025 •

edited

Loading