Support criteria aliases in SqlProjection and SQLCriterion #2358

bahusoid · 2020-04-22T16:55:25Z

Fixes #896

src/NHibernate/Criterion/Expression.cs

fredericDelaporte · 2020-05-10T15:55:23Z

src/NHibernate/Criterion/Projections.cs

-		/// <param name="types"></param>
-		/// <returns></returns>
-		public static IProjection SqlProjection(string sql, string[] columnAliases, IType[] types)
+		public static SQLProjection SqlProjection(string sql, string[] columnAliases, IType[] types)


Deprecate and add Projections.Sql instead?

Ok. Maybe for consistency do the same with SqlGroupProjection and GroupProjection.

Well, it may conflict with a Group method, but it seems to me to be a bit the same thing. I do not really make sense of these variations here...

Done ( I didn't touch GroupProjection)

hazzik · 2020-05-13T21:25:02Z

src/NHibernate/Criterion/ICriteriaQuery.cs

@@ -6,6 +6,15 @@

 namespace NHibernate.Criterion
 {
+	//TODO 6.0: Add to ICriteriaQuery interface
+	internal interface ICriteriaQueryNextVer


nit: I don't like the name

It's internal and name explicitly suggests that it will be merged to main interface. What's not to like? ) I like it! Your suggestion?

I got rid of interface

Well, I thought also about "niting" here, but have let it due to being internal.

So what's wrong with NextVer suffix? I just want to stop thinking how to name my next temp internal interface that should be merged. I thought it's a good plan to use some name convention for such interfaces. If ISomethingNextVer is not good enough can you suggest something better?

I intentionally prefixed my comment with ‘nit:’

I understand the rationale here, and I do not have a better idea, that is was I had not written anything initially. I would not reject this practice, I was just sharing a feeling.

src/NHibernate/Criterion/SQLCriterion.cs

src/NHibernate/Criterion/SQLProjection.cs

hazzik · 2020-05-16T01:56:24Z

src/NHibernate/Criterion/SQLCriterion.cs

+		/// Provide list of criteria aliases that's used in SQL projection.
+		/// To be replaced with SQL aliases.
+		/// </summary>
+		public SQLCriterion AddAliases(params string[] criteriaAliases)


After a close inspection of the changes I concluded that this method is unnecessary. All information has already presented in the query.

We have 2 ways forward:

Extract aliases from SQLCriterion/SQLProjection with Regex; This approach requires just small adjustments, but I don't quite like it as it requires regex processing, which could be expensive. This can be slightly optimized if aliases are extracted on SQLCriterion/SQLProjection is construction.

Get aliases from Criteria.

I like the second approach slightly more.

I like automatic aliases extraction over manual as is less error prone.

Extract aliases from SQLCriterion/SQLProjection with Regex

I don't like any automatic parsing routines and intentionally made it manual. All automatic parsing logic should at least be optional - as in most cases it's not required. And in some cases it's harmful (like when user wants {alias} be present in SQL projection without any transformation), So I don't like your PR which now adds mandatory Replace calls for all criteria aliases in query.

And initially I wanted to support not only aliases but any projections in this template:

Projections.SQL(" {value1} + {x}.Value2") .AddAliases("x") .AddProjection("value1", Projections.Property("Value1"))

But projections support requires SqlString.Replace(string, sqlString) which is missing. And I left implementation for later.

So I'm for manual explicit alias registration (similar how it's done in hql SQL - ISQLQuery.AddEntity). As convenience we can add method (like ResolveAliases) which would parse string via regex and populate _criteriaAliases but don't see necessity in it (user can implement it himself).

Get aliases from Criteria.

ICriteriaQuery contains this data so why can't it be exposed? It feels just like proper place for retrieving such information. And I'm not sure I understand how it can be retrieved from ICriteria. Could you clarify?

And in some cases it's harmful (like when user wants {alias} be present in SQL projection without any transformation)
...
So I'm for manual explicit alias registration
...
PR which now adds mandatory Replace calls for all criteria aliases in query.

I don't understand why would anyone want to leave untranslated {something} in the SQL. So, I don't see a way when there is an alias in SQLCriterion/SQLProjection which would not be registered in AddAliases(...) call.

For me it feels more natural and convenient to translate all aliases rather than manually register them for translation.

If you are talking about performance here, it is negligible in my view as CriteriaQueryTranslator already does some unnecessary heavy lifting "just in case".

I'm for manual explicit alias registration (similar how it's done in hql SQL - ISQLQuery.AddEntity)

AddEntity is a little different than this. It add a necessary information to the query which it would not have otherwise (entity type for example). On the other hand, aliases are already known and readily available to use, so AddAliases not add real value.

Get aliases from Criteria.

ICriteriaQuery contains this data so why can't it be exposed? It feels just like proper place for retrieving such information.

This is exactly what I meant and how I implemented it.

And I'm not sure I understand how it can be retrieved from ICriteria. Could you clarify?

I did not say ICriteria, I meant somewhere in the Criteria. There are aliases in ICriteria, but they would need to be passed to the ICriteriaQuery to get SQLAliases. I tried to implement it this way, but was more complex.

And initially I wanted to support not only aliases but any projections in this template

Nothing in your PR or my addition stops you to implement this, and the solution would be orthogonal to the resolving aliases. The only similarity would be that it would be happening at the same place.

I think we would need to invoke @fredericDelaporte so he can express his view and resolve the arguments.

I don't understand why would anyone want to leave untranslated {something}

Because it's part of some unrelated string in this projection.

Nothing in your PR or my addition stops you to implement this

Mostly user would use either projection or criteria alias but not both - so unnecessary additional work. And it could clash with user aliases for projections. With explicit registering it's less probable.

In the other hand, aliases are already known and readily available to use, so AddAliases not add real value

Explicity is added value for me. Do the job only if it's requested.

If you are talking about performance here, ... CriteriaQueryTranslator already does some unnecessary heavy lifting "just in case"

That's a not good point for adding additionally unnecessary heavy lifting. We should minimize it instead.
And yes that's my main concern - I really don't like that for each SQL projection/criterion we generate dictionary with existing aliases, then generate possible aliases to replace in template and call Replace for them.

In any way your approach is different implementation so if @fredericDelaporte also thinks it's better just create own PR and I will close mine.

it's better just create own PR and I will close mine.

Created

As I am mostly a HQL and Linq "guy", having not used a lot Criteria, I do not think my view should be taken with a lot of weight.

This said, I am more in favor of Alexander viewpoint, at least about its rationales. But I have not thoroughly reviewed again this PR, and not yet Alexander's one either. I think I will do this tomorrow.

hazzik · 2020-05-16T08:13:16Z

Let's probably split this PR into 2 parts:

obsolete methods returning AbstractCriterion and addition of methods returining SQLCriterion
implementation of NH-1432 - Expression.Sql should support aliases other than {alias} #896

Because we don't have disagreement on the first part.

fredericDelaporte

Even if the other one is chosen, this one should be kept, cleaned-up for having only the "obsolete members" part.

src/NHibernate/Criterion/Projections.cs

src/NHibernate/Criterion/Restrictions.cs

src/NHibernate/Criterion/SQLCriterion.cs

src/NHibernate/Criterion/SQLProjection.cs

hazzik · 2020-05-17T22:49:06Z

src/NHibernate.Test/Criteria/CriteriaQueryTest.cs

+				var subquery = QueryOver.Of(() => studentSubquery)
+				         .And(
+					         Restrictions.Sql("{e}.studentId = 667 and {studentSubquery}.studentId = 667")
+					                   .AddAliases("e", "studentSubquery")).Select(Projections.Id());


Will there be cases when user will want to .AddAliases(...) later? If not I think it is better to add params[] aliases parameter to the Restrictions.Sql method instead of introducing a new method.

hazzik · 2020-05-17T22:59:19Z

src/NHibernate.Test/Criteria/CriteriaQueryTest.cs

+							                    TypeFactory.HeuristicType(typeof(long)),
+							                    TypeFactory.HeuristicType(typeof(short)),
+							                    TypeFactory.HeuristicType(typeof(string)),
+							                    TypeFactory.HeuristicType(typeof(string)),


A little side-track rant: I know, it is a copy-paste from other test, but columns and types have Connascence of Position and CoV which is a sign of a bad design. (NHibernate has this across all code-base, but I think we should start progressing to remove that.

Yeah I agree we should fight it. Though not sure about this place.. This is dictated by method signature. No? To avoid it we need to provide method where all related data is supplied in single parameter. Such wrapping in tests seems excessive...

But this is not only in tests. The test reflect real-like use cases.

Co-authored-by: Frédéric Delaporte <12201973+fredericDelaporte@users.noreply.github.com>

bahusoid changed the title ~~Support criteria aliases in SqlProjection~~ WIP Support criteria aliases in SqlProjection Apr 22, 2020

bahusoid changed the title ~~WIP Support criteria aliases in SqlProjection~~ WIP Support criteria aliases in SqlProjection and SQLCriterion Apr 22, 2020

bahusoid changed the title ~~WIP Support criteria aliases in SqlProjection and SQLCriterion~~ Support criteria aliases in SqlProjection and SQLCriterion Apr 22, 2020

fredericDelaporte added the t: Fix label May 10, 2020

fredericDelaporte reviewed May 10, 2020

View reviewed changes

hazzik reviewed May 13, 2020

View reviewed changes

src/NHibernate/Criterion/SQLCriterion.cs Outdated Show resolved Hide resolved

hazzik reviewed May 13, 2020

View reviewed changes

src/NHibernate/Criterion/SQLProjection.cs Outdated Show resolved Hide resolved

bahusoid added 8 commits May 14, 2020 15:39

Support criteria aliases in SqlProjection

173dce2

Support criteria aliases in SQLCriterion

c26e1bc

Adjust test case to use SQLCriterion

ef69e28

Support aliases in subqueries

28afb5a

Reduce nesting

60f53a9

Avoid breaking changes

19fdf99

AddCriteriaAliases -> AddAliases

086bd64

Fix build

26ceac4

bahusoid force-pushed the projectionAliases branch from c412e1d to 26ceac4 Compare May 14, 2020 12:44

bahusoid added 2 commits May 14, 2020 18:31

Get rid of temp interface

bcf2f11

clean up

9c81579

hazzik reviewed May 16, 2020

View reviewed changes

hazzik force-pushed the projectionAliases branch 2 times, most recently from b7b6632 to 9c81579 Compare May 16, 2020 03:20

hazzik requested a review from fredericDelaporte May 16, 2020 07:40

hazzik mentioned this pull request May 16, 2020

Support for criteria aliases in SqlProjection and SQLCriterion #2383

Merged

fredericDelaporte reviewed May 17, 2020

View reviewed changes

hazzik reviewed May 17, 2020

View reviewed changes

Update comments

2c23c7e

Co-authored-by: Frédéric Delaporte <12201973+fredericDelaporte@users.noreply.github.com>

bahusoid closed this May 24, 2020

bahusoid added r: Rejected and removed t: Fix labels May 24, 2020

fredericDelaporte added r: Replaced and removed r: Rejected labels May 24, 2020

Support criteria aliases in SqlProjection and SQLCriterion #2358

Support criteria aliases in SqlProjection and SQLCriterion #2358

Uh oh!

Conversation

bahusoid commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bahusoid May 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte May 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hazzik May 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bahusoid May 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik May 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bahusoid May 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented May 16, 2020

Uh oh!

fredericDelaporte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bahusoid May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

bahusoid commented Apr 22, 2020 •

edited

Loading

bahusoid May 16, 2020 •

edited

Loading

fredericDelaporte May 17, 2020 •

edited

Loading

hazzik May 16, 2020 •

edited

Loading

bahusoid May 16, 2020 •

edited

Loading

hazzik May 16, 2020 •

edited

Loading

bahusoid May 16, 2020 •

edited

Loading

bahusoid May 20, 2020 •

edited

Loading