issue/DATAJDBC-353 - Fix problem with Join clause on Column with Alias #246
Conversation
uaihebert changed the title uaihebert force-pushed the issue/DATAJDBC-353 branch 2 times, most recently from 2497543 to 30d7dd2 Compare 
| @Test // DATAJDBC-353 | ||
| public void shouldRenderWithColumnAlias() { | ||
| Table a = SQL.table("a").as("a_alias"); Column aColumn = a.column("a_col").as("a_col_alias"); | ||
| Table b = SQL.table("b").as("b_alias"); Column bColumn = b.column("a_col").as("b_col_alias"); |
There was a problem hiding this comment.
For the column name we should use "b_col" makes it easier to understand.
This needs to change in the assert statement as well.
| | ||
| @Test // DATAJDBC-353 | ||
| public void shouldRenderWithColumnAlias() { | ||
| Table a = SQL.table("a").as("a_alias"); Column aColumn = a.column("a_col").as("a_col_alias"); |
There was a problem hiding this comment.
Only one assignment per line, please. Same for the next line.
| public SelectOnConditionComparison on(Expression column) { | ||
| | ||
| this.from = column; | ||
| this.from = new Column((Column) column); |
There was a problem hiding this comment.
While this solves the problem at hand, I really don't like the looks of it.
- Creating a
Columnfrom aColumnimmediately begs the question "Why?". - The answer of course is it drops the alias in the process, which is really unexpected when you just look at the constructor.
We therefore should really solve the problem where the SQL is actually generated and make sure that there the unaliased name of the Column is used.
There was a problem hiding this comment.
What would you suggest here? During query generation, I do not have any access to this information, as far that I know.
There was a problem hiding this comment.
I just had a quick look, so if you think some or all of the following is wrong, please feel free to correct me:
The rendering happens in the JoinVisitor which in turn delegates it to a ConditionVisitor. Since the ConditionVisitor is probably in other places we need a specialised version (JoinConditionVisitor) which doesn't simply uses builder::append but checks if it deals with a column and if so uses table + name instead of the alias.
There was a problem hiding this comment.
I will check that and prepare a new PR. Thank you.
There was a problem hiding this comment.
I think for this task should be for those that have more experience on this project.
I still do not see a way to use a Visitor there. At that point, I have only an expression and do not have access to any other value.
I will close this PR and let someone else work on it.
thank you for your time.
uaihebert force-pushed the issue/DATAJDBC-353 branch from 30d7dd2 to 33ee461 Compare 
if we use Alias column on Join Conditions we will have alias used, what will cause an error. We should not have `join b on a.col = b.alias`. b.alias should not be present in the join, it will raise an error. The correct version should be `join b on a.col = b.col`.
uaihebert force-pushed the issue/DATAJDBC-353 branch from 33ee461 to 01c29fa Compare 
2 participants
The problem is that when we have a column with an alias, this alias is used in the join condition.
When we use an alias in a join clause, we get an error; we should use the "original" column name. That is why the solution that I found here was to instead of passing forward a possible AliasColumn; we downgrade the param to Column.
I added some questions to the ticket. If my assumptions are correct, I will not need to edit my test.
For me, this is my first PR in this project. Let me know what I should change in my approach.