-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[HHH-19365] [WIP] GaussDB Dialect Support #10048
New issue
Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “No 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? No Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ests - Modified the database connection information of GaussDB - Adjusted the version compatibility of the GaussDBDialect class - Updated multiple test cases to adapt to the characteristics of GaussDB -Optimized some SQL statements to comply with the syntax requirements of GaussDB
…ay_position function - Skip GaussDB tests in multiple unit tests- Update the registration logic of GaussDB related functions in CommonFunctionFactory- Add a GaussDB-specific array operation function class - Adjust gradle configuration and update GaussDB connection parameters- Add checks for GaussDB in DialectFeatureChecks
…e related configurations - Removed unsupported GaussDB function implementations - Updated GaussDB database connection configurations - Optimized Gradle build configurations - Adjusted JVM parameter settings
…ts of GaussDB array functions - Modify the comments of the GaussDBArrayRemoveIndexFunction class to clarify its function - Add comments to the GaussDBArraySetFunction class to explain its function - Update the comments of the GaussDBArrayTrimFunction class to clarify its function and add code source description
…d @SkipForDialect annotation in multiple test classes to skip the test of GaussDBDialect dialect - Mainly involves test cases such as array types, JSON functions, mathematical functions, date and time processing - Also includes some dialect-specific functional tests, such as upsert and merge operations
… database- Add a GaussDB-specific JSON function implementation class - Update GaussDBDialect to register new JSON functions - Modify existing JSON function tests to support GaussDB- Optimize JSON path parsing logic
…functions - Removed commented out code blocks in GaussDBArrayPositionFunction - Removed unused SQL statement snippets in GaussdbJsonExistsFunction These code snippets have been officially removed because they are no longer needed in the current code logic.
- Updated SkipForDialect annotation reasons in multiple test classes - Refactored GaussDBArrayTrimFunction, GaussDBJsonInsertFunction, GaussDBJsonObjectAggFunction, GaussDBJsonRemoveFunction, GaussDBJsonReplaceFunction, and GaussDBJsonSetFunction - Adjusted the function registration logic in the GaussDBDialect class - Removed unnecessary type checking and conversion logic - Simplified the implementation of JSON functions to make it closer to PostgreSQL native syntax
- Added or updated SkipForDialect annotations for GaussDBDialect in several test cases - Removed unused generateSeries function in GaussDBDialect - Updated SkipForDialect annotations for some test cases and added specific error messages
- Add multiple improvements to GaussDB dialect, including: - Add support for the less than operator (<) - Optimize the registration of JSON-related functions - Adjust the date format function to adapt to the characteristics of GaussDB - Fix some GaussDB-specific bugs - Add special processing logic for GaussDB in multiple tests - Add support for GaussDB-specific SQL statement structure
- Upgrade GaussDB JDBC driver version from 1.0.0 to 506.0.0.b058 - Change the groupId of GaussDB JDBC driver from com.huaweicloud to com.huaweicloud.gaussdb
- Added @SkipForDialect annotation in multiple test classes to skip the OpenGauss database test - Fixed code formatting issues in some test classes - Removed unnecessary import statements and optimized code structure
- Add OpenGauss database configuration to CI build script - Update GitHub Actions workflow to include OpenGauss testing - Modify the database startup script to support OpenGauss container - Add special handling for OpenGauss in unit tests - Update Docker database scripts to add OpenGauss image and configuration
…ion script - Change "gauss" to "gaussdb" in CI configuration file to correctly display database type - Update database initialization script and add steps to create test schema
…or GaussDB database in build scripts - Skip hibernate-agroal tests when RDBMS environment variable is gaussdb
…or GaussDB database in build scripts - Skip hibernate-agroal tests when RDBMS environment variable is gaussdb
… GaussDB database does not support - In the BaseTransactionIsolationConfigTest class, add the SkipForDialect annotation to the following test methods: - testSettingIsolationAsNumeric - testSettingIsolationAsNumericString - testSettingIsolationAsName - testSettingIsolationAsNameAlt - The reason for skipping these tests is that GaussDBDialect does not support serialization isolation level
- Removed code blocks that handled GaussDB specially based on RDBMS environment variables - Removed the EXTRA_GRADLE_ARGS variable set during the build process - Now execute the build.sh script directly without passing extra parameters
- Updated multiple GaussDB-related JDBC types and function implementations - Adjusted some code structures to improve maintainability - Optimized the handling of PostgreSQL compatibility
gradle.properties
Outdated
@@ -3,7 +3,7 @@ db=h2 | |||
# Keep all these properties in sync unless you know what you are doing! | |||
# We set '-Dlog4j2.disableJmx=true' to prevent classloader leaks triggered by the logger. | |||
# (Some of these settings need to be repeated in the test.jvmArgs blocks of each module) | |||
org.gradle.jvmargs=-Dlog4j2.disableJmx -Xmx2g -XX:MaxMetaspaceSize=256m -XX:+HeapDumpOnOutOfMemoryError -Duser.language=en -Duser.country=US -Duser.timezone=UTC -Dfile.encoding=UTF-8 | |||
org.gradle.jvmargs=-Dlog4j2.disableJmx -Xmx4g -XX:MaxMetaspaceSize=256m -XX:+HeapDumpOnOutOfMemoryError -Duser.language=en -Duser.country=US -Duser.timezone=UTC -Dfile.encoding=UTF-8 |
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.
Do not change this setting if possible.
import java.util.Map; | ||
|
||
/** | ||
* PostgreSQL json_query function. |
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 be GaussDB
import org.hibernate.sql.ast.tree.expression.Expression; | ||
|
||
/** | ||
* @author Steve Ebersole |
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.
Seems code not included in this PR.
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.
You did add this file in this PR: a93ff39#diff-2880ff9a18fe32ce9c5f8b10ac94da9a9d8613a55cfa8c52adee02cd59fa9e1b
I'm not sure why, though. Maybe a rebase gone wrong?
If you have a specific reason to add this, you will probably want to explain it.
import org.junit.jupiter.api.AfterEach; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.Test; | ||
|
||
import jakarta.persistence.Column; | ||
import jakarta.persistence.Embeddable; | ||
import jakarta.persistence.EmbeddedId; |
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.
set you code formaters to meet hibernate.
import jakarta.persistence.ManyToOne; | ||
import jakarta.persistence.OrderColumn; | ||
import jakarta.persistence.Table; | ||
import org.assertj.core.api.Assertions; | ||
import org.hibernate.Hibernate; |
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.
set you code formaters to meet hibernate.
assertNull( loadedBook.getEditor() ); | ||
} ); | ||
doInHibernate( | ||
this::sessionFactory, session -> { |
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.
set you code formaters to meet hibernate.
header[0] = new Byte( ( byte ) 3 ); | ||
header[1] = new Byte( ( byte ) 0 ); | ||
header[0] = new Byte( (byte) 3 ); | ||
header[1] = new Byte( (byte) 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.
set you code formaters to meet hibernate.
@@ -48,6 +49,7 @@ public void testTypeSelection() { | |||
} | |||
|
|||
@Test | |||
@org.hibernate.testing.orm.junit.SkipForDialect(dialectClass = GaussDBDialect.class, reason = "opengauss don't support") |
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.
Do not need fully qualified name.
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.
There seem to be two annotations named @SkipForDialect, which have the same name but are different.
public void testVersionUnchangedByteArray() throws Exception { | ||
Session s; | ||
Transaction tx; | ||
s = openSession(); | ||
tx = s.beginTransaction(); | ||
VersionedCompiledCode cc = createCompiledCode(); | ||
Byte[] header = new Byte[2]; | ||
header[0] = new Byte( ( byte ) 3 ); | ||
header[1] = new Byte( ( byte ) 0 ); | ||
header[0] = new Byte( (byte) 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.
Code format
This should all be moved to the Also, is this something you plan on testing continuously? |
I noticed that there are still some dialect-related functions in the core module. |
|| dialect instanceof CockroachDialect | ||
|| dialect instanceof MySQLDialect | ||
|| dialect instanceof PostgreSQLDialect; | ||
|| dialect instanceof CockroachDialect |
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.
Please remove all such formatting changes. There are a lot throughout your PR...
We have read https://github.com/hibernate/hibernate-orm/blob/main/dialects.adoc and plan to contribute this dialect into core. Do you have any suggestions for contributing to core ? Do we need to contribute to |
Correct. All of the GaussDBDialect-related code you added here should be moved there. |
We have very strict standards for when Dialects get moved into hibernate-core. For now (at least, though no promises at all), this is a community contributed and maintained Dialect, by definition. We (the Hibernate team) do not test it and won't support it per-se beyond keeping it up-to-date with changes we make to Dialect contracts. |
Thanks for your pull request! This pull request appears to follow the contribution rules. › This message was automatically generated. |
This reverts commit b2f27c1.
832658b
to
c975ed5
Compare
we analyzed the differences between hibernate-core and hibernate-community. There is a relatively big gap in the quality assurance measures of the two. The code in hibernate-community will not execute the core test cases of hibernate-core. Ensuring that all the test cases provided by hibernate-core pass is of great importance for the quality. We expect that GaussDB can continuously pass all the test cases. What suggestions do you have regarding this situation? We are willing to provide relevant measures according to your requirements to ensure that the strict requirements of the hibernate community are complied with. |
We should probably continue this discussion on Zulip? https://hibernate.zulipchat.com/#narrow/channel/132096-hibernate-user/topic/GaussDB.20Dialect.20Support.20-.20PR.20.2319365 |
"xmlelement(name myElement, xmlattributes('123' as attr1, '456' as `attr-2`)), " + | ||
"xmlelement(name myElement, xmlattributes('123' as attr1), 'myContent', xmlelement(name empty))", | ||
Tuple.class | ||
).getSingleResult(); |
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.
You appear to still have lots of code format changes throughout the PR?
@@ -1,5 +1,5 @@ | |||
<component name="ProjectCodeStyleConfiguration"> | |||
<state> | |||
<option name="USE_PER_PROJECT_SETTINGS" value="true" /> | |||
<option name="PREFERRED_PROJECT_CODE_STYLE" value="hibernate-style" /> |
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 don't think this was supposed to be changed in this PR?
import org.hibernate.sql.ast.tree.expression.Expression; | ||
|
||
/** | ||
* @author Steve Ebersole |
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.
You did add this file in this PR: a93ff39#diff-2880ff9a18fe32ce9c5f8b10ac94da9a9d8613a55cfa8c52adee02cd59fa9e1b
I'm not sure why, though. Maybe a rebase gone wrong?
If you have a specific reason to add this, you will probably want to explain it.
@@ -112,7 +112,7 @@ private void executeQuery(SessionFactoryScope scope, boolean criteria, boolean l | |||
final EntityGraph<?> entityGraph = session.getEntityGraph( "test-graph" ); | |||
final List<Person> resultList = query.setHint( HINT_SPEC_FETCH_GRAPH, entityGraph ).getResultList(); | |||
assertThat( resultList ).hasSize( 2 ); | |||
assertThat( resultList.stream().map( p -> p.getAddress().getId() ) ).containsExactly( 1L, 2L ); | |||
assertThat( resultList.stream().map( p -> p.getAddress().getId() ) ).contains( 1L, 2L ); |
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 changes test expectations -- and not just for GaussDB. Why?
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.
No order by, there is no guarantee of the data order. Table size, algorithms, indexes may lead to different data order.
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.
That's fair, but contains
will accept result lists that have extra (unexpected) items aside from 1
and 2
. It would also accept duplicates.
To just ignore order, this is what you should use:
assertThat( resultList.stream().map( p -> p.getAddress().getId() ) ).contains( 1L, 2L ); | |
assertThat( resultList.stream().map( p -> p.getAddress().getId() ) ).containsExactlyInAnyOrder( 1L, 2L ); |
If you made similar changes in other places, I'd recommend double-checking them.
We fixed all problems in this PR and add more feartures and improvements and created a new PR in #10093 |
HHH-19365 GaussDB Dialect Support
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.