Skip to content
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

[WIP] Improved SQL generation by setting correct aliases. #1405

Merged
merged 15 commits into from
Dec 5, 2018

Conversation

sdanyliv
Copy link
Member

@sdanyliv sdanyliv commented Nov 15, 2018

Improved Aliases for SQL queries.

  • Queries become readable and and needs almost nothing for sharing between others.
  • Aliases for Associations are now valuable (configurable)
  • Added AliasName for AssociationAttribute.

Sample query before fix:

SELECT
	[p].[FirstName] as [FirstName1],
	[p].[PersonID] as [PersonID1],
	[p].[LastName] as [LastName1],
	[p].[MiddleName] as [MiddleName1],
	[p].[Gender] as [Gender1]
FROM
	(
		SELECT
			[t1].[PersonID] + 1 as [c1],
			[t1].[FirstName],
			[t1].[PersonID],
			[t1].[LastName],
			[t1].[MiddleName],
			[t1].[Gender]
		FROM
			[Person] [t1]
		WHERE
			[t1].[PersonID] = 1
	) [p]
WHERE
	[p].[c1] + 1 = 3 AND [p].[c1] = 2

After fix

SELECT
	[p_2].[FirstName],
	[p_2].[PersonID],
	[p_2].[LastName],
	[p_2].[MiddleName],
	[p_2].[Gender]
FROM
	(
		SELECT
			[p_1].[PersonID] + 1 as [ID],
			[p_1].[FirstName],
			[p_1].[PersonID],
			[p_1].[LastName],
			[p_1].[MiddleName],
			[p_1].[Gender]
		FROM
			[Person] [p_1]
		WHERE
			[p_1].[PersonID] = 1
	) [p_2]
WHERE
	[p_2].[ID] + 1 = 3 AND [p_2].[ID] = 2

To be done:

  • Fix tests.
  • Show query samples with improvements.
  • Check T4 generation because of changes in MakeUniqueNames function.

Sorry, something went wrong.

/// Set this value to <c>null</c> to disable special alias generation queries.
/// </remarks>
/// </summary>
public static string AssociationAlias { get; set; } = "A_{0}";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public static string AssociationAlias { get; set; } = "A_{0}";
public static string AssociationAlias { get; set; } = "a_{0}";

/// </code>
/// </example>
/// </summary>
public static bool GenerateFinalAliases { get; set; } = true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public static bool GenerateFinalAliases { get; set; } = true;
public static bool GenerateFinalAliases { get; set; } = false;

@@ -1050,7 +1050,7 @@ public void SetAlias(string alias)
if (alias == null)
return;

if (alias.Contains('<'))
if (!alias.Contains('<'))
Copy link
Member

Choose a reason for hiding this comment

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

Is it was a bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

Strange but it will not allow aliasing. For me it looks like a bug because here we are trying do not use anonymous type name but in wrong way.


public void RemoveAlias(string alias)
{
if (_aliases != null)
{
alias = alias.ToUpper();
if (_aliases.ContainsKey(alias))
if (_aliases.Contains(alias))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (_aliases.Contains(alias))

You do not need to check out if you use HashSet.

case QueryElementType.SqlParameter:
{
var p = (SqlParameter)expr;

if (p.IsQueryParameter)
{
if (!objs.ContainsKey(expr))
if (!paramsVisited.Contains(expr))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!paramsVisited.Contains(expr))
if (!paramsVisited.Add(expr))

{
objs.Add(expr, expr);
p.Name = GetAlias(p.Name, "p");
paramsVisited.Add(p);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
paramsVisited.Add(p);

@MaceWindu MaceWindu added this to the 2.6.0 milestone Nov 27, 2018
@MaceWindu MaceWindu merged commit 5edfd9c into master Dec 5, 2018
@MaceWindu
Copy link
Contributor

T4 will be fixed as part of #1433

@MaceWindu MaceWindu deleted the feature_aliases branch December 5, 2018 14:02
@Fruchuxs
Copy link

Fruchuxs commented Feb 6, 2019

And how I can set it back to the old t1 behavior to fix my tests?

@Fruchuxs
Copy link

Fruchuxs commented Feb 6, 2019

Checked out and it seems, that the first alias wins.

queryable.Where(x => x.Count > 5).Where(y => y.Id == 5).Select(v => v.Id); the alias is x. Seems to works fine with yoda syntax, but looks volatile for the method syntax.

@sdanyliv
Copy link
Member Author

sdanyliv commented Feb 6, 2019

And how I can set it back to the old t1 behavior to fix my tests?

It will be a problem even in future. It is bad to check generated SQL in tests. Especially when we apply additional optimizations.

@Fruchuxs
Copy link

Fruchuxs commented Feb 7, 2019

It's just an integration test to evaluate the lib on top we built our application. Small changes are not a problem, but this change was hard to understand in the method-syntax and is rather a configuration thing as a optimization for me.

@sdanyliv
Copy link
Member Author

sdanyliv commented Feb 7, 2019

We have implemented this feature to simplify understanding queries. Feel free to open issue when we can improve aliasing. Samples are wellcome.
And yes implementation is closer to linq query syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants