Skip to content

HHH-19297 Register SingleStore json functions to community SingleStoreDialect #9927

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 4 commits into
base: main
Choose a base branch
from

Conversation

oyeliseiev-ua
Copy link
Contributor

@oyeliseiev-ua oyeliseiev-ua commented Mar 28, 2025

HHH-19297
Small improvement for community SingleStoreDialect


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.


https://hibernate.atlassian.net/browse/HHH-19297

Copy link
Member

@beikov beikov left a comment

Choose a reason for hiding this comment

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

Instead of using ANY for JSON arguments, please use FunctionParameterType.IMPLICIT_JSON.

Also, instead of adding the dialect specific functions, it would probably be nicer if you could try to implement the "standardized" functions as described here for this dialect when possible/reasonable: https://docs.jboss.org/hibernate/orm/7.0/userguide/html_single/Hibernate_User_Guide.html#hql-functions-json

@oyeliseiev-ua oyeliseiev-ua force-pushed the singlestore_json_functions branch from 9c7c200 to 34fc204 Compare April 11, 2025 17:20
Copy link
Member

@beikov beikov left a comment

Choose a reason for hiding this comment

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

This is looking good already, but the current implementation doesn't seem to support all types of data in JSON yet. Also, checking just for instanceof UnparsedNumericLiteral<?> isn't really checking if an expression is a numeric. An expression obviously can also be arithmetic or come from a column etc.. I proposed a different solution to this typing issue.
I can also merge the PR as it is, though it's incomplete AFAICT. Let me know what you want.

@oyeliseiev-ua
Copy link
Contributor Author

This is looking good already, but the current implementation doesn't seem to support all types of data in JSON yet. Also, checking just for instanceof UnparsedNumericLiteral<?> isn't really checking if an expression is a numeric. An expression obviously can also be arithmetic or come from a column etc.. I proposed a different solution to this typing issue. I can also merge the PR as it is, though it's incomplete AFAICT. Let me know what you want.

@beikov Thank you for your review. I'll try to make improvements based on your suggestions and will let you know.

@oyeliseiev-ua oyeliseiev-ua requested a review from beikov April 16, 2025 16:25
@oyeliseiev-ua
Copy link
Contributor Author

@beikov take a look please

Copy link
Member

@beikov beikov left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks for putting in the effort to make this compatible with our standard functions :)

One last question. Do you run the hibernate-core testsuite against a SingleStore database? If you do, it might be nice to add an entry to docker_db.sh and local-build-plugins/src/main/groovy/local.databases.gradle to allow other users to also do that.

I have the feeling that if you try to e.g. set a JSON object as value on a JSON object, that the current logic might fail. Consider an HQL expression like json_set('{}', '$.a', json_array()).

AFAIU your code, it calls to_json() on the result of json_array(). I guess for the database, the result of json_array() is of type json, but in case it treats this as "string", the invocation might produce {"a":"[]"}, whereas it should produce {"a":[]}. This is something that could be caught by running the hibernate-core testsuite.

@oyeliseiev-ua oyeliseiev-ua force-pushed the singlestore_json_functions branch from e9b491d to 129c154 Compare April 17, 2025 18:37
@oyeliseiev-ua
Copy link
Contributor Author

This looks great! Thanks for putting in the effort to make this compatible with our standard functions :)

One last question. Do you run the hibernate-core testsuite against a SingleStore database? If you do, it might be nice to add an entry to docker_db.sh and local-build-plugins/src/main/groovy/local.databases.gradle to allow other users to also do that.

I have the feeling that if you try to e.g. set a JSON object as value on a JSON object, that the current logic might fail. Consider an HQL expression like json_set('{}', '$.a', json_array()).

AFAIU your code, it calls to_json() on the result of json_array(). I guess for the database, the result of json_array() is of type json, but in case it treats this as "string", the invocation might produce {"a":"[]"}, whereas it should produce {"a":[]}. This is something that could be caught by running the hibernate-core testsuite.

@beikov
I have the test suite configuration on a separate branch, intended for local execution only. I'm concerned about maintainability due to specific SingleStore limitations—many tests need to be marked as skipped or have their logic adjusted. If that's acceptable, I'd like to create a separate PR for this.

The latest commit was tested using:
org.hibernate.orm.test.function.json.* and
org.hibernate.orm.test.query.hql.JsonFunctionTests.java

@oyeliseiev-ua oyeliseiev-ua force-pushed the singlestore_json_functions branch from 129c154 to e3d3910 Compare April 18, 2025 06:35
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.

None yet

2 participants