SQL Criteria Implementation #1394

gtnicol · 2022-08-17T17:21:58Z

Initial SQL implementation of Criteria API limited to basic CRUD.

elucash · 2022-09-01T23:53:24Z

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:

Need to add to readme file notes (or add more notes) about the experimental nature of this implementation, that it's a subject to change (API or impl, or both) and all the "caveat emptor" stuff.
I would ask to review dependencies (such as some jackson modules) to be marked <optional>true or <scope>provided if they can be made so, i.e. if they are not absolutely necessary.
In general, I'm not comfortable with the way reflection is used for some model discovery and how Jackson marshaling is integrated. But because we don't have good standard approaches for these, we'll let it fly as it is. I've used some experimental stuff in another project which reads SQL datasets into immutable objects, but that approach also use quite experimental generated Datatypes and likewise experimental Codec runtime bridges (with JSON delegated to Moshi/ok-io), so I cannot recommend that approach right now (if curious, you can look into https://github.com/immutables/io/blob/master/io/regres/src/io/immutables/regres/Coding.java and other classes around in that repo). So let it be, given that we'll mark experimental nature of this implementation.
Please adjust the formatting the Java files in this PR, basically you can use Google Java code conventions or at least something like this .editorconfig (It's our bad that we don't have formatter or this editor config file attached, we'll fix this eventually)

[*]
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).
And I think we will be able to merge this. Thank you!

gtnicol · 2022-09-02T03:43:58Z

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.

asereda-gs · 2022-09-07T03:37:37Z

Hello,

Thanks for this PR. Please give me some time to review it.

gtnicol · 2022-09-07T17:38:46Z

Sure thing. Thanks!

jmoghisi · 2022-09-23T07:16:58Z

criteria/sql/src/org/immutables/criteria/sql/SqlBackend.java

+/**
+ * Implementation of {@code Backend} which delegates to the underlying SQL specific commands.
+ */
+public class SQLBackend implements Backend {


is SQLBackend duplicated? diff shows SQLBackend.java and SqlBackend.java

It's a rename... case change.

gtnicol · 2022-09-23T17:55:30Z

Hello,

Thanks for this PR. Please give me some time to review it.

Andrei, any ETA on review? I can do some work on this over the weekend.

elucash · 2022-10-04T07:57:46Z

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

gtnicol · 2022-10-04T12:29:32Z

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.

elucash · 2022-10-04T15:39:59Z

Thank you! JIC in general we would prefer SqlSomething, not SQLSomething. It's rooted in Google Java conventions, some other conventions (like Google's Go lang etc) could prefer other styles.

elucash · 2022-10-18T02:29:11Z

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!

jmoghisi · 2022-11-12T10:25:12Z

Hi @elucash , hope you are well and recovered from the storm. What's the latest on this PR? May it be merged?

elucash · 2022-11-14T01:03:40Z

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).
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)

gtnicol · 2022-11-14T01:48:38Z

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: ***@***.***>

gtnicol · 2022-11-14T17:56:00Z

Why would @Generator.Template public class Sql extends AbstractTemplate {} not result in Sql.generator being called to generate the code? I was mirroring what is in samplegenerators where you can see the output in target

gtnicol · 2022-11-14T18:12:32Z

I figured it out. I will do the RowMapper generation and then do another checkin.

gtnicol · 2022-11-25T22:21:55Z

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.

elucash · 2022-11-25T22:26:33Z

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.

gtnicol · 2022-11-25T22:35:03Z

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.

elucash · 2022-12-13T20:30:06Z

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).

gtnicol · 2022-12-13T20:40:37Z

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.

gtnicol · 2022-12-25T20:46:45Z

This should be OK to merge... all the tests and build run cleanly.

gtnicol · 2023-01-06T19:37:52Z

Anything blocking this from being merged at this point?

elucash · 2023-01-06T19:50:54Z

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.

elucash · 2023-01-06T19:53:09Z

@gtnicol just some build problems

[INFO] -------------------------------------------------------------
Error:  COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
Error:  /home/runner/work/immutables/immutables/criteria/sql/src/org/immutables/criteria/sql/commands/SqlCountCommand.java: Internal compiler error: java.util.ServiceConfigurationError: javax.annotation.processing.Processor: Provider org.immutables.generator.processor.Processor could not be instantiated at java.util.ServiceLoader.fail(ServiceLoader.java:232)
Error:  /home/runner/work/immutables/immutables/criteria/sql/src/org/immutables/criteria/sql/commands/SqlCountCommand.java:[35,14] The type SqlCountCommand is already defined

Let me know if it's something outside of this PR. Thank you for you patience

gtnicol · 2023-01-07T21:10:01Z

Looks like the SQL->Sql renaming didn't go through. I will build on Linux to make sure naming isn't an issue.

gtnicol · 2023-01-07T23:44:14Z

I have deleted the duplicate files and it builds cleanly under Ubuntu 22 now

asereda-gs

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.

asereda-gs · 2023-01-08T00:09:36Z

criteria/sql/src/org/immutables/criteria/sql/generator/Sql.generator

+
+import org.immutables.criteria.sql.conversion.RowMappers;
+
+public class [_setup_class_name_] {


Why is it necessary to generate special code for RowMapper ?

Isn't there enough type information using reflection ?

it's better without the reflection, no?

asereda-gs · 2023-01-08T00:12:22Z

criteria/sql/test/org/immutables/criteria/sql/compiler/SqlCompilerTests.java

+import org.immutables.criteria.expression.Query;
+import org.immutables.criteria.sql.SqlSetup;
+import org.immutables.criteria.sql.reflection.SqlTypeMetadata;
+import org.junit.Test;


please use JUnit5.

asereda-gs · 2023-01-08T00:14:00Z

pom.xml

@@ -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 should be version properties defined in main pom.xml.

Example: ${jackson.version}

asereda-gs · 2023-01-08T00:18:39Z

criteria/sql/src/org/immutables/criteria/sql/compiler/SqlPathNaming.java

+import org.immutables.criteria.backend.JavaBeanNaming;
+import org.immutables.criteria.expression.Path;
+
+public class SqlPathNaming extends JavaBeanNaming {


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.

asereda-gs · 2023-01-08T00:42:38Z

criteria/sql/src/org/immutables/criteria/sql/compiler/SqlExpression.java

+ */
+package org.immutables.criteria.sql.compiler;
+
+public interface SqlExpression extends SqlNode {


Can you please explain why do we need SqlExpression / SqlNode class hierarchy ?

asereda-gs · 2023-01-08T00:47:52Z

criteria/sql/src/org/immutables/criteria/sql/commands/SqlCommand.java

+import java.util.Map;
+import java.util.stream.Collectors;
+
+public interface SqlCommand {


Is this internal interface to sql criteria ? If so, can you please better comment it?

elucash · 2023-01-10T03:38:06Z

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.
As of now there is build problem. It might again be something unrelated to this PR, but don't have energy to investigate it now, will return in 2-3 days.

Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project criteria-sql: Compilation failure
Error:  /home/runner/work/immutables/immutables/criteria/sql/src/org/immutables/criteria/sql/commands/SqlCountCommand.java: Internal compiler error: java.util.ServiceConfigurationError: javax.annotation.processing.Processor: Provider org.immutables.generator.processor.Processor could not be instantiated at java.util.ServiceLoader.fail(ServiceLoader.java:232)

gtnicol · 2023-01-10T13:15:20Z

let me have another look... I think getting rid of all the reflection stuff was a useful change

gtnicol · 2023-02-02T15:29:55Z

I have a feeling this is something else... it doesn't appear to be the code but something environmental or something with the tools.

SQL Criteria Implementation

d5c35bb

Initial SQL implementation of Criteria API limited to basic CRUD.

gtnicol mentioned this pull request Aug 18, 2022

does Criteria has support jdbc? #1257

Open

gtnicol added 2 commits August 24, 2022 22:11

Added copyright headers, fixed a naming/case issue

fe60fad

Tracking Immutables updates

53fab7a

jmoghisi reviewed Sep 23, 2022

View reviewed changes

Major changes to remove Jackson dependencies etc.

00c3f33

Merge branch 'master' into feature/sql-criteria

45bd7e3

gtnicol added 3 commits December 15, 2022 09:07

Update pom.xml

2255b9a

Move to 2.9.3 release version.

Merge branch 'master' into feature/sql-criteria

81e7ceb

Cleaning up generator dependencies

614d3bc

Removing duplicates caused by name case issues on macos

9f7ffa0

asereda-gs force-pushed the master branch from d4d9c06 to a031992 Compare January 7, 2023 23:52

asereda-gs reviewed Jan 8, 2023

View reviewed changes

Merge branch 'master' into feature/sql-criteria

6585458


		import org.immutables.criteria.sql.conversion.RowMappers;

		public class [_setup_class_name_] {

SQL Criteria Implementation #1394

Are you sure you want to change the base?

SQL Criteria Implementation #1394

Conversation

gtnicol commented Aug 17, 2022

elucash commented Sep 1, 2022

gtnicol commented Sep 2, 2022

asereda-gs commented Sep 7, 2022

gtnicol commented Sep 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtnicol commented Sep 23, 2022

elucash commented Oct 4, 2022

gtnicol commented Oct 4, 2022

elucash commented Oct 4, 2022

elucash commented Oct 18, 2022

jmoghisi commented Nov 12, 2022

elucash commented Nov 14, 2022

gtnicol commented Nov 14, 2022 via email

gtnicol commented Nov 14, 2022

gtnicol commented Nov 14, 2022

gtnicol commented Nov 25, 2022

elucash commented Nov 25, 2022

gtnicol commented Nov 25, 2022 • edited

elucash commented Dec 13, 2022

gtnicol commented Dec 13, 2022

gtnicol commented Dec 25, 2022

gtnicol commented Jan 6, 2023

elucash commented Jan 6, 2023

elucash commented Jan 6, 2023

gtnicol commented Jan 7, 2023

gtnicol commented Jan 7, 2023

asereda-gs 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

elucash commented Jan 10, 2023

gtnicol commented Jan 10, 2023

gtnicol commented Feb 2, 2023

gtnicol commented Nov 25, 2022 •

edited