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

Generate Unique alias for table names #4537

Merged
merged 1 commit into from
Feb 19, 2016
Merged

Generate Unique alias for table names #4537

merged 1 commit into from
Feb 19, 2016

Conversation

smitpatel
Copy link
Contributor

resolves #3826
Presently it is code change only. If approaches looks right then I will update SQL generated for tests and add more test to verify this scenario.

@smitpatel
Copy link
Contributor Author

@anpete @maumar

var counter = 0;

int _;
if (int.TryParse(currentAlias.Substring(1), out _))
Copy link
Contributor

Choose a reason for hiding this comment

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

we could change this to work the same for one letter aliases as well as multi letter aliases, i.e.
currently we have:
a -> a0, a1, a2
bb -> bb0, bb00, bb000

we should aim for
bb -> bb0, bb1, bb2, bb3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

@@ -179,7 +179,7 @@ var materializer

selectExpression.Predicate = oldPredicate;
selectExpression.RemoveTable(joinExpression);
selectExpression.AddTable(newJoinExpression);
selectExpression.AddTable(newJoinExpression, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use named parameter to clarify false here.

@smitpatel smitpatel force-pushed the tableAliasCollision branch 4 times, most recently from e1ea21d to 3394fba Compare February 12, 2016 22:20
@smitpatel
Copy link
Contributor Author

@anpete - Updated with different approach. I checked few tests to verify that we are preserving user-defined names.

@smitpatel smitpatel force-pushed the tableAliasCollision branch 3 times, most recently from 6dcef4a to e1a7e5c Compare February 12, 2016 23:19
@@ -165,7 +165,7 @@ var predicate

if (predicate != null)
{
var innerSelectExpression = handlerContext.SelectExpressionFactory.Create();
var innerSelectExpression = handlerContext.SelectExpressionFactory.Create(handlerContext.QueryModelVisitor.QueryCompilationContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

break those long lines into 2 separate ones, we usually do something like:

var innerSelectExpression
= handlerContext.SelectExpressionFactory.Create(handlerContext.QueryModelVisitor...)

SELECT TOP(1) [subQuery0].[Id]
FROM [Level2] AS [subQuery0]
WHERE [subQuery0].[Level1_Optional_Id] = [e1].[Id]
SELECT TOP(1) [subQuery00].[Id]
Copy link
Contributor

Choose a reason for hiding this comment

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

we can probably change the base generated name from "subquery0" to "subquery", since it's being uniquefied anyway

@smitpatel
Copy link
Contributor Author

Updated with feedback. Added some tests.

{
Check.NotNull(currentAlias, nameof(currentAlias));

if (string.Equals(currentAlias, ""))
Copy link
Contributor

Choose a reason for hiding this comment

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

Or, currentAlias.Length == 0

@anpete
Copy link
Contributor

anpete commented Feb 17, 2016

@smitpatel Is it just that not all test changes have been reverted from the earlier approach? Because, if not, it looks like we are still unique-fying too much.

@smitpatel smitpatel force-pushed the tableAliasCollision branch 2 times, most recently from ef15f26 to dbf2b98 Compare February 18, 2016 00:09
@smitpatel
Copy link
Contributor Author

Updated with 1 more change which reduced changes required.
There are mainly 3 reasons for changes occurring

  1. We are unique-fying across query model. Therefore if multiple SQL statements are executed aliases will be unique across all. Which was not the case previously.
  2. Subquery aliases - During include processing when we generate a subquery for inner join, previously we used same alias as inner table. We are making it unique now.
  3. Client Eval - If the query is using client eval then we visit the query model first time using alias names but fails to generate SQL out of it. So when we process query model again for client eval, they will use different aliases.

Most changes in tests look to be consequence of one of the above. If there is any particular test which seems to be unique-fy extra then point me to it and I will investigate it.

@smitpatel
Copy link
Contributor Author

Update to above:
4. Subquery depth - Previously we used t + subquery depth as alias so that push down subquery gets different alias, now we are unique-fying it so we don't use depth anymore. Which changes it to start from t instead of t0 as before.

@anpete
Copy link
Contributor

anpete commented Feb 18, 2016

@smitpatel Thanks for the clear explanation. 1,2 & 4 are OK. Can you see if there is any easy fix to 3? Otherwise :shipit:

@smitpatel smitpatel merged commit d307bc4 into dev Feb 19, 2016
@smitpatel smitpatel deleted the tableAliasCollision branch February 19, 2016 03:04
@smitpatel
Copy link
Contributor Author

For point 3 I tried looking into avoiding it. But we traverse the queryModel 2 times and both generate different SelectExpression. And we don't have any way to know what initiated QueryModelVisitor. The only way to do it without changing a lot of things in background would be to remove all generated aliases if client eval starts but it can have side-effects. So I did not attempt that way.

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.

4 participants