[CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times#3658
Conversation
kramerul
force-pushed
the
join-fieldname-collisions
branch
4 times, most recently
from
a4ec94a to
7aacd35
Compare
| oldSelectList.add(new SqlIdentifier(rightFieldNames.get(i), POS)); | ||
| } | ||
| } else { | ||
| oldSelectList = ImmutableList.<SqlNode>copyOf(builder.select.getSelectList().getList()); |
There was a problem hiding this comment.
I fixed the CheckerFramework finding.
kramerul
force-pushed
the
join-fieldname-collisions
branch
3 times, most recently
from
dc820ad to
0ff0d50
Compare
|
This PR could cause some problems. We had trouble with a statement like
|
| return result(join, leftResult, rightResult); | ||
| } | ||
|
|
||
| private Result maybeFixRenamedFields(Result rightResult, Join e) { |
There was a problem hiding this comment.
This doesn't seem to have any relation to JDBC.
Perhaps the issue should be renamed.
There was a problem hiding this comment.
I suspect that this change may affect other open issues on JIRA, it looks pretty basic.
Can you do a quick check?
There was a problem hiding this comment.
The class RelToSqlConverter is only used in JdbcImplementor and PigRelToSqlConverter. Both classes are related to JDBC. Therefore, I would like to keep the title of the issue. Additionally Julian Hyde added the JDBC relation to the title
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java
Outdated
Show resolved
Hide resolved
| + "FROM (VALUES (1, 2)) AS \"t\" (\"K\", \"V\")\n" | ||
| + "INNER JOIN " | ||
| + "(VALUES (1)) AS \"t0\" (\"K\") ON \"t\".\"K\" = \"t0\".\"K\""); | ||
| + "INNER JOIN (SELECT \"K\" AS \"K0\"\n" |
There was a problem hiding this comment.
It's not easy to review all these changes. I checked some of them.
There was a problem hiding this comment.
Yes it's quite hard to review all changes. I also had a look at most of the changes inside the tests.
| SqlNode column = oldSelectList.get(i); | ||
| if (!rightFieldNames.get(i).equals(fieldNames.get(offset + i))) { | ||
| column = | ||
| SqlStdOperatorTable.AS.createCall(POS, SqlUtil.stripAs(column), |
There was a problem hiding this comment.
What guarantees that this name is unique?
This may be another column name that appears on the other side.
I think you need to create a Set with all "forbidden" names and make sure that this name is not part of that set.
There was a problem hiding this comment.
The uniqueness is currently guaranteed by the function SqlValidatorUtil.deriveJoinRowType which creates the unique field names for the Join relation. But this is not really visible here. Therefore, I will add a comment.
kramerul
force-pushed
the
join-fieldname-collisions
branch
from
0ff0d50 to
951b324
Compare
…le is joined multiple times
kramerul
force-pushed
the
join-fieldname-collisions
branch
from
951b324 to
e8f266d
Compare
|
| int offset = e.getLeft().getRowType().getFieldCount(); | ||
| boolean hasFieldNameCollision = false; | ||
| for (int i = 0; i < rightFieldNames.size(); i++) { | ||
| if (!rightFieldNames.get(i).equals(fieldNames.get(offset + i))) { |
There was a problem hiding this comment.
why is it sufficient to compare with the field with the same relative index rather than compare all n^2 pairs?
|
I will rework the complete PR according to the last comment of Julian Hyde in https://issues.apache.org/jira/browse/CALCITE-6221 |
|
Closing this PR in favor of a new one. |
3 participants
No description provided.