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
SQL Criteria Implementation #1394
base: master
Are you sure you want to change the base?
Conversation
Initial SQL implementation of Criteria API limited to basic CRUD.
I'm sorry for delay, needed time to look around the implementation. As usual with such big contributions, it's a difficult task, all or nothing, either just take all or nitpick / fight for many significant or insignificant details to no end, which require so much time that we probably don't have. I actually want us to merge it. Here's my points:
[*]
indent_style = space
indent_size = 2
continuation_indent_size = 4
max_line_length = 120
charset = utf-8
end_of_line = lf
insert_final_newline = true Having these thoughts, I'll also will kindly summon the quick view of @asereda-gs in case there are more ideas / considerations (especially on the model discovery/reflection in case there are some better routines). |
Cool. I'll wait for any further comments then make the changes and commit. FWIW. I also dislike the reflection... I was wondering if we shouldn't have something that generates marshaling code, much like the Gson TypeAdapter code? Given the experimental nature of this, maybe that'd be overkill at this point. |
Hello, Thanks for this PR. Please give me some time to review it. |
Sure thing. Thanks! |
/** | ||
* Implementation of {@code Backend} which delegates to the underlying SQL specific commands. | ||
*/ | ||
public class SQLBackend implements Backend { |
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.
is SQLBackend duplicated? diff shows SQLBackend.java and SqlBackend.java
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.
It's a rename... case change.
Andrei, any ETA on review? I can do some work on this over the weekend. |
Sorry for the waiting. I was lucky to be almost at the center of the IAN landfall so just slowly getting back to business. It seems that @asereda-gs have no easy reservations (only deep ones, which might require in-depth investigation) I'll merge it as soon as I will get better internet, granted we fix minor stuff and highlight the experimental nature of the PR |
Cool. I'll clean up the formatting and might also clean up some of the reflection. Give me a few days to turn it around. |
Thank you! JIC in general we would prefer |
Internet is still slow (mobile internet, broadband is not restored yet). The next release will be only after I'll have good connection. But I can merge this, please remind be if anything is remaining here (style issues?). Hope to merge this promptly, thank you! |
Hi @elucash , hope you are well and recovered from the storm. What's the latest on this PR? May it be merged? |
Hi @jmoghisi I think I can merge this. Just don't want to reformat/codestyle after merge, so maybe those things can be included (cc @gtnicol).
Rename prefixes Can we remove SQL migration library and |
I'm working on an update to this which removes the Jackson RowMapper among other things.... thought being to generate a RowMapper using the generator framework instead. I also reformatted the code and renamed the classes etc.
On Sun, Nov 13, 2022, at 8:03 PM, Eugene Lukash wrote:
Hi @jmoghisi <https://github.com/jmoghisi> I think I can merge this. Just don't want to reformat/codestyle after merge, so maybe those things can be included (cc @gtnicol <https://github.com/gtnicol>).
Basically, reformat according to
`indent_style = space
indent_size = 2
continuation_indent_size = 4
`
… Rename prefixes `SQL* -> Sql*`
Can we remove SQL migration library and `test-migrations.xml` file? For tests, simple plain SQL script will work. Just `createStatement` and read file (getResourceAsStream)
—
Reply to this email directly, view it on GitHub <#1394 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHASSEUZGZOOHOUTMHVA7PDWIGFXRANCNFSM562KK3OA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Why would |
I figured it out. I will do the RowMapper generation and then do another checkin. |
I just checked in a pretty significant update which removed the Jackson and Liquibase dependencies. It now uses the generator framework to generate a default setup and a default row mapper (these can be overridden). I also renamed a lot of the files and reformatted everything. |
Thank you for update! As I've recently got my broadband internet connection restored, planning to do the long awaited maintenance release over the weekend. Let me know if you think the PR is feature complete for final review and merge or we can have some more time and include it in the release soon after. |
The one major area I can see potential issues is that it generates a RowMapper which calls the builder - so it assumes a Value.Immutable annotation and doesn't take Style into account. That said, it should be pretty much ready to go - I ran a few more tests and assuming you think this is OK, I'd say it's good enough. |
i'm doing an 2.9.3 release now, not including this, but expect this to be merged soon after. I'm assuming you're saying it's ready enough (given an uncertainty with some issues). |
Looks like we need to change the dependency to 2.9.3 from 2.9.3-SNAPSHOT? Apart from that I'd say we can merge it in - the code generation path vs reflection cleans things up a lot. |
Move to 2.9.3 release version.
This should be OK to merge... all the tests and build run cleanly. |
Anything blocking this from being merged at this point? |
I'll merge it today, seems we don't have any resources/capacity for diving into any remaining details. As I've mentioned, we will just not advertise it as something stable, will stay experimental for a while etc. |
@gtnicol just some build problems
Let me know if it's something outside of this PR. Thank you for you patience |
Looks like the SQL->Sql renaming didn't go through. I will build on Linux to make sure naming isn't an issue. |
I have deleted the duplicate files and it builds cleanly under Ubuntu 22 now |
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.
Hi @gtnicol!
Thank you for this PR and sorry that it takes so long for us to review it.
I have left small comments but think we should have a deeper discussion about SQL Backend for Criteria. Unfortunately it is not as straightforward as other backends (Mongo, Elastic etc.) since they already have POJO mapping / serialization "frameworks" included.
Probably several iterations will have to be done around POJO / ResultSet mapping and conversions.
I will continue looking and leaving comments.
|
||
import org.immutables.criteria.sql.conversion.RowMappers; | ||
|
||
public class [_setup_class_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.
Why is it necessary to generate special code for RowMapper ?
Isn't there enough type information using reflection ?
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.
it's better without the reflection, no?
import org.immutables.criteria.expression.Query; | ||
import org.immutables.criteria.sql.SqlSetup; | ||
import org.immutables.criteria.sql.reflection.SqlTypeMetadata; | ||
import org.junit.Test; |
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 use JUnit5.
pom.xml
Outdated
@@ -107,8 +107,8 @@ | |||
<reactive-streams.version>1.0.2</reactive-streams.version> | |||
<log4j2.version>2.17.1</log4j2.version> | |||
<mongo-java-server.version>1.38.0</mongo-java-server.version> | |||
<jackson.version>2.8.11</jackson.version> | |||
<jackson-databind.version>2.8.11.6</jackson-databind.version> | |||
<jackson.version>2.12.7</jackson.version> |
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 should be version properties defined in main pom.xml
.
Example: ${jackson.version}
import org.immutables.criteria.backend.JavaBeanNaming; | ||
import org.immutables.criteria.expression.Path; | ||
|
||
public class SqlPathNaming extends JavaBeanNaming { |
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.
In this case I would prefer composition (with a generic PathNaming
interface) over inheritance.
Also I'm not sure JavaBeanNaming
should be used at all.
*/ | ||
package org.immutables.criteria.sql.compiler; | ||
|
||
public interface SqlExpression extends SqlNode { |
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.
Can you please explain why do we need SqlExpression
/ SqlNode
class hierarchy ?
import java.util.Map; | ||
import java.util.stream.Collectors; | ||
|
||
public interface SqlCommand { |
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.
Is this internal interface to sql criteria ? If so, can you please better comment it?
I think we will merge the initial version, given that it is experimental/unstable API and design should be changed around how result mappers and other field mapping works, maybe even aligning/ changing how other criteria backends are mapped, to use something common / general and flexible enough. I would as to address some of the stuff like testing framework usage. Maybe remove some intermediate speculative abstractions (see latest comments by @asereda-gs ) That could be done in this or a separate PR after this merged.
|
let me have another look... I think getting rid of all the reflection stuff was a useful change |
I have a feeling this is something else... it doesn't appear to be the code but something environmental or something with the tools. |
Initial SQL implementation of Criteria API limited to basic CRUD.