Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-02-02 Thread via GitHub


kramerul commented on code in PR #3664:
URL: https://github.com/apache/calcite/pull/3664#discussion_r1475710267


##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -437,11 +437,43 @@ public Result visit(Filter e) {
   final Result x = visitInput(e, 0, Clause.WHERE);
   parseCorrelTable(e, x);
   final Builder builder = x.builder(e);
+  if (input instanceof Join) {
+final Context context = x.qualifiedContext();
+final ImmutableList.Builder selectList = 
ImmutableList.builder();
+// Fieldnames are unique since they are created by 
SqlValidatorUtil.deriveJoinRowType()
+final List uniqueFieldNames = 
input.getRowType().getFieldNames();
+boolean selectListRequired = false;
+for (int i = 0; i < context.fieldCount; i++) {
+  final SqlNode field = context.field(i);
+  final String fieldName = uniqueFieldNames.get(i);
+  if (fieldWasRenamedByJoinDueToNameClash(field, fieldName)) {
+selectListRequired = true;
+  }
+  selectList.add(
+  SqlStdOperatorTable.AS.createCall(POS, field,
+  new SqlIdentifier(fieldName, POS)));
+}
+if (selectListRequired) {
+  builder.setSelect(new SqlNodeList(selectList.build(), POS));
+}
+  }
   builder.setWhere(builder.context.toSql(null, e.getCondition()));
   return builder.result();
 }
   }
 
+  private static boolean fieldWasRenamedByJoinDueToNameClash(SqlNode field, 
String fieldName) {
+if (!(field instanceof SqlIdentifier)) {

Review Comment:
   I changed it accordingly.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-02-01 Thread via GitHub


YiwenWu commented on code in PR #3664:
URL: https://github.com/apache/calcite/pull/3664#discussion_r1475652612


##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -437,11 +437,43 @@ public Result visit(Filter e) {
   final Result x = visitInput(e, 0, Clause.WHERE);
   parseCorrelTable(e, x);
   final Builder builder = x.builder(e);
+  if (input instanceof Join) {
+final Context context = x.qualifiedContext();
+final ImmutableList.Builder selectList = 
ImmutableList.builder();
+// Fieldnames are unique since they are created by 
SqlValidatorUtil.deriveJoinRowType()
+final List uniqueFieldNames = 
input.getRowType().getFieldNames();
+boolean selectListRequired = false;
+for (int i = 0; i < context.fieldCount; i++) {
+  final SqlNode field = context.field(i);
+  final String fieldName = uniqueFieldNames.get(i);
+  if (fieldWasRenamedByJoinDueToNameClash(field, fieldName)) {
+selectListRequired = true;
+  }
+  selectList.add(
+  SqlStdOperatorTable.AS.createCall(POS, field,
+  new SqlIdentifier(fieldName, POS)));
+}
+if (selectListRequired) {
+  builder.setSelect(new SqlNodeList(selectList.build(), POS));
+}
+  }
   builder.setWhere(builder.context.toSql(null, e.getCondition()));
   return builder.result();
 }
   }
 
+  private static boolean fieldWasRenamedByJoinDueToNameClash(SqlNode field, 
String fieldName) {
+if (!(field instanceof SqlIdentifier)) {

Review Comment:
   Whether this method can satisfy?
   ```
   private static boolean fieldRequiresAlias(Result x) {
 List uniqueList = new ArrayList();
 for(SqlNode node : x.qualifiedContext().fieldList()) {
   if(node instance SqlIdentifier) { 
 SqlIdentifier field = (SqlIdentifier) node;
 if (uniqueList.contains(last(field.names))) {
 return true;
 }
 uniqueList.add(last(field.names));
   }
 }
 return false;
   }
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-02-01 Thread via GitHub


sonarcloud[bot] commented on PR #3664:
URL: https://github.com/apache/calcite/pull/3664#issuecomment-1923021602

   ## [![Quality Gate 
Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png
 'Quality Gate 
Passed')](https://sonarcloud.io/dashboard?id=apache_calcite=3664) 
**Quality Gate passed**  
   Kudos, no new issues were introduced!
   
   [0 New 
issues](https://sonarcloud.io/project/issues?id=apache_calcite=3664=false=true)
  
   [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3664=false=true)
  
   [88.6% Coverage on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3664=new_coverage=list)
  
   [0.0% Duplication on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3664=new_duplicated_lines_density=list)
  
 
   [See analysis details on 
SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3664)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-02-01 Thread via GitHub


kramerul commented on code in PR #3664:
URL: https://github.com/apache/calcite/pull/3664#discussion_r1475585120


##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -437,11 +437,43 @@ public Result visit(Filter e) {
   final Result x = visitInput(e, 0, Clause.WHERE);
   parseCorrelTable(e, x);
   final Builder builder = x.builder(e);
+  if (input instanceof Join) {
+final Context context = x.qualifiedContext();
+final ImmutableList.Builder selectList = 
ImmutableList.builder();
+// Fieldnames are unique since they are created by 
SqlValidatorUtil.deriveJoinRowType()
+final List uniqueFieldNames = 
input.getRowType().getFieldNames();
+boolean selectListRequired = false;
+for (int i = 0; i < context.fieldCount; i++) {
+  final SqlNode field = context.field(i);
+  final String fieldName = uniqueFieldNames.get(i);
+  if (fieldWasRenamedByJoinDueToNameClash(field, fieldName)) {
+selectListRequired = true;
+  }
+  selectList.add(
+  SqlStdOperatorTable.AS.createCall(POS, field,
+  new SqlIdentifier(fieldName, POS)));
+}
+if (selectListRequired) {
+  builder.setSelect(new SqlNodeList(selectList.build(), POS));
+}
+  }
   builder.setWhere(builder.context.toSql(null, e.getCondition()));
   return builder.result();
 }
   }
 
+  private static boolean fieldWasRenamedByJoinDueToNameClash(SqlNode field, 
String fieldName) {
+if (!(field instanceof SqlIdentifier)) {

Review Comment:
   I also thought about simplifying this on base of 
`Result.qualifiedContext().fieldList()`. But I had no idea how this could be 
achieved.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-02-01 Thread via GitHub


kramerul commented on PR #3664:
URL: https://github.com/apache/calcite/pull/3664#issuecomment-1922916619

   I'm not so deep into Calcite that I would be able to say if there are other 
cases. As far as I know, this is the only case because
   * columns are only renamed in the `Join` class
   * the generation of selected columns in SQL is only missing for `Filter`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-02-01 Thread via GitHub


kramerul commented on code in PR #3664:
URL: https://github.com/apache/calcite/pull/3664#discussion_r1475583359


##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -437,11 +437,43 @@ public Result visit(Filter e) {
   final Result x = visitInput(e, 0, Clause.WHERE);
   parseCorrelTable(e, x);
   final Builder builder = x.builder(e);
+  if (input instanceof Join) {
+final Context context = x.qualifiedContext();
+final ImmutableList.Builder selectList = 
ImmutableList.builder();
+// Fieldnames are unique since they are created by 
SqlValidatorUtil.deriveJoinRowType()
+final List uniqueFieldNames = 
input.getRowType().getFieldNames();
+boolean selectListRequired = false;
+for (int i = 0; i < context.fieldCount; i++) {
+  final SqlNode field = context.field(i);
+  final String fieldName = uniqueFieldNames.get(i);
+  if (fieldWasRenamedByJoinDueToNameClash(field, fieldName)) {
+selectListRequired = true;
+  }
+  selectList.add(
+  SqlStdOperatorTable.AS.createCall(POS, field,
+  new SqlIdentifier(fieldName, POS)));
+}
+if (selectListRequired) {
+  builder.setSelect(new SqlNodeList(selectList.build(), POS));
+}
+  }
   builder.setWhere(builder.context.toSql(null, e.getCondition()));
   return builder.result();
 }
   }
 
+  private static boolean fieldWasRenamedByJoinDueToNameClash(SqlNode field, 
String fieldName) {
+if (!(field instanceof SqlIdentifier)) {
+  return true;
+}
+SqlIdentifier identifier = (SqlIdentifier) field;
+if (identifier.isStar()) {
+  return true;
+}
+List names = identifier.names;
+return !names.get(names.size() - 1).equals(fieldName);

Review Comment:
   I changed it.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-02-01 Thread via GitHub


YiwenWu commented on code in PR #3664:
URL: https://github.com/apache/calcite/pull/3664#discussion_r1475566245


##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -437,11 +437,43 @@ public Result visit(Filter e) {
   final Result x = visitInput(e, 0, Clause.WHERE);
   parseCorrelTable(e, x);
   final Builder builder = x.builder(e);
+  if (input instanceof Join) {
+final Context context = x.qualifiedContext();
+final ImmutableList.Builder selectList = 
ImmutableList.builder();
+// Fieldnames are unique since they are created by 
SqlValidatorUtil.deriveJoinRowType()
+final List uniqueFieldNames = 
input.getRowType().getFieldNames();
+boolean selectListRequired = false;
+for (int i = 0; i < context.fieldCount; i++) {
+  final SqlNode field = context.field(i);
+  final String fieldName = uniqueFieldNames.get(i);
+  if (fieldWasRenamedByJoinDueToNameClash(field, fieldName)) {
+selectListRequired = true;
+  }
+  selectList.add(
+  SqlStdOperatorTable.AS.createCall(POS, field,
+  new SqlIdentifier(fieldName, POS)));
+}
+if (selectListRequired) {
+  builder.setSelect(new SqlNodeList(selectList.build(), POS));
+}
+  }
   builder.setWhere(builder.context.toSql(null, e.getCondition()));
   return builder.result();
 }
   }
 
+  private static boolean fieldWasRenamedByJoinDueToNameClash(SqlNode field, 
String fieldName) {
+if (!(field instanceof SqlIdentifier)) {

Review Comment:
   context(AliasContext) field is only allowed to return SqlIdentifier.
   and Is there any other simpler way to determine whether rename is needed?
   can we identify the Duplicate name by only 
Result.qualifiedContext().fieldList()
   
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-02-01 Thread via GitHub


YiwenWu commented on code in PR #3664:
URL: https://github.com/apache/calcite/pull/3664#discussion_r1475566245


##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -437,11 +437,43 @@ public Result visit(Filter e) {
   final Result x = visitInput(e, 0, Clause.WHERE);
   parseCorrelTable(e, x);
   final Builder builder = x.builder(e);
+  if (input instanceof Join) {
+final Context context = x.qualifiedContext();
+final ImmutableList.Builder selectList = 
ImmutableList.builder();
+// Fieldnames are unique since they are created by 
SqlValidatorUtil.deriveJoinRowType()
+final List uniqueFieldNames = 
input.getRowType().getFieldNames();
+boolean selectListRequired = false;
+for (int i = 0; i < context.fieldCount; i++) {
+  final SqlNode field = context.field(i);
+  final String fieldName = uniqueFieldNames.get(i);
+  if (fieldWasRenamedByJoinDueToNameClash(field, fieldName)) {
+selectListRequired = true;
+  }
+  selectList.add(
+  SqlStdOperatorTable.AS.createCall(POS, field,
+  new SqlIdentifier(fieldName, POS)));
+}
+if (selectListRequired) {
+  builder.setSelect(new SqlNodeList(selectList.build(), POS));
+}
+  }
   builder.setWhere(builder.context.toSql(null, e.getCondition()));
   return builder.result();
 }
   }
 
+  private static boolean fieldWasRenamedByJoinDueToNameClash(SqlNode field, 
String fieldName) {
+if (!(field instanceof SqlIdentifier)) {

Review Comment:
   context(AliasContext) field is only allowed to return SqlIdentifier.
   and Is there any other simpler way to determine whether rename is needed?
   can we identify the Duplicate name by Result.qualifiedContext().fieldList()
   
   
   



##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -437,11 +437,43 @@ public Result visit(Filter e) {
   final Result x = visitInput(e, 0, Clause.WHERE);
   parseCorrelTable(e, x);
   final Builder builder = x.builder(e);
+  if (input instanceof Join) {
+final Context context = x.qualifiedContext();
+final ImmutableList.Builder selectList = 
ImmutableList.builder();
+// Fieldnames are unique since they are created by 
SqlValidatorUtil.deriveJoinRowType()
+final List uniqueFieldNames = 
input.getRowType().getFieldNames();
+boolean selectListRequired = false;
+for (int i = 0; i < context.fieldCount; i++) {
+  final SqlNode field = context.field(i);
+  final String fieldName = uniqueFieldNames.get(i);
+  if (fieldWasRenamedByJoinDueToNameClash(field, fieldName)) {
+selectListRequired = true;
+  }
+  selectList.add(
+  SqlStdOperatorTable.AS.createCall(POS, field,
+  new SqlIdentifier(fieldName, POS)));
+}
+if (selectListRequired) {
+  builder.setSelect(new SqlNodeList(selectList.build(), POS));
+}
+  }
   builder.setWhere(builder.context.toSql(null, e.getCondition()));
   return builder.result();
 }
   }
 
+  private static boolean fieldWasRenamedByJoinDueToNameClash(SqlNode field, 
String fieldName) {
+if (!(field instanceof SqlIdentifier)) {
+  return true;
+}
+SqlIdentifier identifier = (SqlIdentifier) field;
+if (identifier.isStar()) {
+  return true;
+}
+List names = identifier.names;
+return !names.get(names.size() - 1).equals(fieldName);

Review Comment:
   can be simplified to  `last(names)equals(fieldName)`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-02-01 Thread via GitHub


sonarcloud[bot] commented on PR #3664:
URL: https://github.com/apache/calcite/pull/3664#issuecomment-1922883755

   ## [![Quality Gate 
Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png
 'Quality Gate 
Passed')](https://sonarcloud.io/dashboard?id=apache_calcite=3664) 
**Quality Gate passed**  
   Kudos, no new issues were introduced!
   
   [0 New 
issues](https://sonarcloud.io/project/issues?id=apache_calcite=3664=false=true)
  
   [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3664=false=true)
  
   [88.6% Coverage on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3664=new_coverage=list)
  
   [0.0% Duplication on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3664=new_duplicated_lines_density=list)
  
 
   [See analysis details on 
SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3664)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-02-01 Thread via GitHub


kramerul commented on code in PR #3664:
URL: https://github.com/apache/calcite/pull/3664#discussion_r147280


##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -437,11 +437,43 @@ public Result visit(Filter e) {
   final Result x = visitInput(e, 0, Clause.WHERE);
   parseCorrelTable(e, x);
   final Builder builder = x.builder(e);
+  if (input instanceof Join) {
+final Context context = x.qualifiedContext();
+final ImmutableList.Builder selectList = 
ImmutableList.builder();
+// Fieldnames are unique since they are created by 
SqlValidatorUtil.deriveJoinRowType()
+final List uniqueFieldNames = 
input.getRowType().getFieldNames();
+boolean selectListRequired = false;
+for (int i = 0; i < context.fieldCount; i++) {
+  final SqlNode field = context.field(i);
+  final String fieldName = uniqueFieldNames.get(i);
+  if (fieldRequiresAlias(field, fieldName)) {
+selectListRequired = true;
+  }
+  selectList.add(
+  SqlStdOperatorTable.AS.createCall(POS, field,
+  new SqlIdentifier(fieldName, POS)));
+}
+if (selectListRequired) {
+  builder.setSelect(new SqlNodeList(selectList.build(), POS));
+}
+  }
   builder.setWhere(builder.context.toSql(null, e.getCondition()));
   return builder.result();
 }
   }
 
+  private static boolean fieldRequiresAlias(SqlNode field, String fieldName) {
+if (!(field instanceof SqlIdentifier)) {
+  return true;
+}
+SqlIdentifier identifier = (SqlIdentifier) field;
+if (identifier.isStar()) {
+  return true;
+}
+List names = identifier.names;
+return !names.get(names.size() - 1).equals(fieldName);

Review Comment:
   I renamed this function to `fieldWasRenamedByJoinDueToNameClash`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-02-01 Thread via GitHub


mihaibudiu commented on code in PR #3664:
URL: https://github.com/apache/calcite/pull/3664#discussion_r1474977545


##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -437,11 +437,43 @@ public Result visit(Filter e) {
   final Result x = visitInput(e, 0, Clause.WHERE);
   parseCorrelTable(e, x);
   final Builder builder = x.builder(e);
+  if (input instanceof Join) {
+final Context context = x.qualifiedContext();
+final ImmutableList.Builder selectList = 
ImmutableList.builder();
+// Fieldnames are unique since they are created by 
SqlValidatorUtil.deriveJoinRowType()
+final List uniqueFieldNames = 
input.getRowType().getFieldNames();
+boolean selectListRequired = false;
+for (int i = 0; i < context.fieldCount; i++) {
+  final SqlNode field = context.field(i);
+  final String fieldName = uniqueFieldNames.get(i);
+  if (fieldRequiresAlias(field, fieldName)) {
+selectListRequired = true;
+  }
+  selectList.add(
+  SqlStdOperatorTable.AS.createCall(POS, field,
+  new SqlIdentifier(fieldName, POS)));
+}
+if (selectListRequired) {
+  builder.setSelect(new SqlNodeList(selectList.build(), POS));
+}
+  }
   builder.setWhere(builder.context.toSql(null, e.getCondition()));
   return builder.result();
 }
   }
 
+  private static boolean fieldRequiresAlias(SqlNode field, String fieldName) {
+if (!(field instanceof SqlIdentifier)) {
+  return true;
+}
+SqlIdentifier identifier = (SqlIdentifier) field;
+if (identifier.isStar()) {
+  return true;
+}
+List names = identifier.names;
+return !names.get(names.size() - 1).equals(fieldName);

Review Comment:
   This is the inverse of what I expected.
   Maybe you need to document this function if this is correct.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-02-01 Thread via GitHub


sonarcloud[bot] commented on PR #3664:
URL: https://github.com/apache/calcite/pull/3664#issuecomment-1921528712

   ## [![Quality Gate 
Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png
 'Quality Gate 
Passed')](https://sonarcloud.io/dashboard?id=apache_calcite=3664) 
**Quality Gate passed**  
   The SonarCloud Quality Gate passed, but some issues were introduced.
   
   [2 New 
issues](https://sonarcloud.io/project/issues?id=apache_calcite=3664=false=true)
  
   [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3664=false=true)
  
   [88.6% Coverage on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3664=new_coverage=list)
  
   [0.0% Duplication on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3664=new_duplicated_lines_density=list)
  
 
   [See analysis details on 
SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3664)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-02-01 Thread via GitHub


kramerul closed pull request #3658: [CALCITE-6221] JDBC adapter generates 
invalid query when the same table is joined multiple times
URL: https://github.com/apache/calcite/pull/3658


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-02-01 Thread via GitHub


kramerul commented on PR #3658:
URL: https://github.com/apache/calcite/pull/3658#issuecomment-1921384505

   Closing this PR in favor of a new one.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-01-31 Thread via GitHub


kramerul commented on PR #3658:
URL: https://github.com/apache/calcite/pull/3658#issuecomment-1918819890

   I will rework the complete PR according to the last comment of Julian Hyde 
in https://issues.apache.org/jira/browse/CALCITE-6221


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-01-30 Thread via GitHub


mihaibudiu commented on code in PR #3658:
URL: https://github.com/apache/calcite/pull/3658#discussion_r1471799166


##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -257,6 +258,60 @@ public Result visit(Join e) {
 return result(join, leftResult, rightResult);
   }
 
+  private Result maybeFixRenamedFields(Result rightResult, Join e) {
+// TableModify is treated in a special way inside visit(TableModify 
modify).
+// Therefore, we need to skip it here
+Frame last = stack.peekLast();
+if (last != null && last.r instanceof TableModify) {
+  return rightResult;
+}
+// If there is a filter on the stack an additional "SELECT * ... AS tx" is 
added
+// as wrapper. This could hide the original aliases used inside.
+// So we need to reflect the renaming of the fields in the join condition,
+// which was already done in SqlValidatorUtil.deriveJoinRowType
+if (stack.stream().noneMatch(it -> it.r instanceof Filter)) {
+  return rightResult;
+}
+List rightFieldNames = e.getRight().getRowType().getFieldNames();
+List fieldNames = e.getRowType().getFieldNames();
+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))) {

Review Comment:
   why is it sufficient to compare with the field with the same relative index 
rather than compare all n^2 pairs?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-01-30 Thread via GitHub


sonarcloud[bot] commented on PR #3658:
URL: https://github.com/apache/calcite/pull/3658#issuecomment-1916821078

   ## [![Quality Gate 
Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png
 'Quality Gate 
Passed')](https://sonarcloud.io/dashboard?id=apache_calcite=3658) 
**Quality Gate passed**  
   The SonarCloud Quality Gate passed, but some issues were introduced.
   
   [1 New 
issue](https://sonarcloud.io/project/issues?id=apache_calcite=3658=false=true)
  
   [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3658=false=true)
  
   [98.2% Coverage on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3658=new_coverage=list)
  
   [0.0% Duplication on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3658=new_duplicated_lines_density=list)
  
 
   [See analysis details on 
SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3658)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-01-30 Thread via GitHub


sonarcloud[bot] commented on PR #3658:
URL: https://github.com/apache/calcite/pull/3658#issuecomment-1916812639

   ## [![Quality Gate 
Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png
 'Quality Gate 
Passed')](https://sonarcloud.io/dashboard?id=apache_calcite=3658) 
**Quality Gate passed**  
   The SonarCloud Quality Gate passed, but some issues were introduced.
   
   [1 New 
issue](https://sonarcloud.io/project/issues?id=apache_calcite=3658=false=true)
  
   [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3658=false=true)
  
   [98.2% Coverage on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3658=new_coverage=list)
  
   [0.0% Duplication on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3658=new_duplicated_lines_density=list)
  
 
   [See analysis details on 
SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3658)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-01-30 Thread via GitHub


kramerul commented on code in PR #3658:
URL: https://github.com/apache/calcite/pull/3658#discussion_r1470726277


##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -257,6 +257,48 @@ public Result visit(Join e) {
 return result(join, leftResult, rightResult);
   }
 
+  private Result maybeFixRenamedFields(Result rightResult, Join e) {
+Frame last = stack.peekLast();
+if (last != null && last.r instanceof TableModify) {
+  return rightResult;
+}
+List rightFieldNames = e.getRight().getRowType().getFieldNames();
+List fieldNames = e.getRowType().getFieldNames();
+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))) {
+hasFieldNameCollision = true;
+  }
+}
+if (!hasFieldNameCollision) {
+  return rightResult;
+}
+Builder builder = rightResult.builder(e);
+List oldSelectList = new ArrayList<>();
+if (builder.select.getSelectList() == SqlNodeList.SINGLETON_STAR) {
+  for (int i = 0; i < rightFieldNames.size(); i++) {
+oldSelectList.add(new SqlIdentifier(rightFieldNames.get(i), POS));
+  }
+} else {
+  for (SqlNode node : builder.select.getSelectList().getList()) {
+oldSelectList.add(requireNonNull(node, "node"));
+  }
+}
+List selectList = new ArrayList<>();
+for (int i = 0; i < rightFieldNames.size(); i++) {
+  SqlNode column = oldSelectList.get(i);
+  if (!rightFieldNames.get(i).equals(fieldNames.get(offset + i))) {
+column =
+SqlStdOperatorTable.AS.createCall(POS, SqlUtil.stripAs(column),

Review 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.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-01-29 Thread via GitHub


kramerul commented on code in PR #3658:
URL: https://github.com/apache/calcite/pull/3658#discussion_r1470726277


##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -257,6 +257,48 @@ public Result visit(Join e) {
 return result(join, leftResult, rightResult);
   }
 
+  private Result maybeFixRenamedFields(Result rightResult, Join e) {
+Frame last = stack.peekLast();
+if (last != null && last.r instanceof TableModify) {
+  return rightResult;
+}
+List rightFieldNames = e.getRight().getRowType().getFieldNames();
+List fieldNames = e.getRowType().getFieldNames();
+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))) {
+hasFieldNameCollision = true;
+  }
+}
+if (!hasFieldNameCollision) {
+  return rightResult;
+}
+Builder builder = rightResult.builder(e);
+List oldSelectList = new ArrayList<>();
+if (builder.select.getSelectList() == SqlNodeList.SINGLETON_STAR) {
+  for (int i = 0; i < rightFieldNames.size(); i++) {
+oldSelectList.add(new SqlIdentifier(rightFieldNames.get(i), POS));
+  }
+} else {
+  for (SqlNode node : builder.select.getSelectList().getList()) {
+oldSelectList.add(requireNonNull(node, "node"));
+  }
+}
+List selectList = new ArrayList<>();
+for (int i = 0; i < rightFieldNames.size(); i++) {
+  SqlNode column = oldSelectList.get(i);
+  if (!rightFieldNames.get(i).equals(fieldNames.get(offset + i))) {
+column =
+SqlStdOperatorTable.AS.createCall(POS, SqlUtil.stripAs(column),

Review 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 check.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-01-29 Thread via GitHub


kramerul commented on code in PR #3658:
URL: https://github.com/apache/calcite/pull/3658#discussion_r1470720546


##
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##
@@ -809,8 +809,8 @@ private static String toSql(RelNode root, SqlDialect 
dialect,
 // RelFieldTrimmer maybe build the RelNode.
 relFn(fn).ok("SELECT \"t\".\"V\" AS \"l_v\"\n"
 + "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"

Review Comment:
   Yes it's quite hard to review all changes. I also had a look at most of the 
changes inside the tests.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-01-29 Thread via GitHub


kramerul commented on code in PR #3658:
URL: https://github.com/apache/calcite/pull/3658#discussion_r1470719490


##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -225,7 +225,7 @@ public Result visit(Join e) {
   break;
 }
 final Result leftResult = visitInput(e, 0).resetAlias();
-final Result rightResult = visitInput(e, 1).resetAlias();
+Result rightResult = maybeFixRenamedFields(visitInput(e, 1).resetAlias(), 
e);

Review Comment:
   I changed this.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-01-29 Thread via GitHub


kramerul commented on code in PR #3658:
URL: https://github.com/apache/calcite/pull/3658#discussion_r1470712364


##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -257,6 +257,48 @@ public Result visit(Join e) {
 return result(join, leftResult, rightResult);
   }
 
+  private Result maybeFixRenamedFields(Result rightResult, Join e) {

Review 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



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-01-29 Thread via GitHub


mihaibudiu commented on code in PR #3658:
URL: https://github.com/apache/calcite/pull/3658#discussion_r1470106502


##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -257,6 +257,48 @@ public Result visit(Join e) {
 return result(join, leftResult, rightResult);
   }
 
+  private Result maybeFixRenamedFields(Result rightResult, Join e) {
+Frame last = stack.peekLast();
+if (last != null && last.r instanceof TableModify) {
+  return rightResult;
+}
+List rightFieldNames = e.getRight().getRowType().getFieldNames();
+List fieldNames = e.getRowType().getFieldNames();
+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))) {
+hasFieldNameCollision = true;
+  }
+}
+if (!hasFieldNameCollision) {
+  return rightResult;
+}
+Builder builder = rightResult.builder(e);
+List oldSelectList = new ArrayList<>();
+if (builder.select.getSelectList() == SqlNodeList.SINGLETON_STAR) {
+  for (int i = 0; i < rightFieldNames.size(); i++) {
+oldSelectList.add(new SqlIdentifier(rightFieldNames.get(i), POS));
+  }
+} else {
+  for (SqlNode node : builder.select.getSelectList().getList()) {
+oldSelectList.add(requireNonNull(node, "node"));
+  }
+}
+List selectList = new ArrayList<>();
+for (int i = 0; i < rightFieldNames.size(); i++) {
+  SqlNode column = oldSelectList.get(i);
+  if (!rightFieldNames.get(i).equals(fieldNames.get(offset + i))) {
+column =
+SqlStdOperatorTable.AS.createCall(POS, SqlUtil.stripAs(column),

Review 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.



##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -257,6 +257,48 @@ public Result visit(Join e) {
 return result(join, leftResult, rightResult);
   }
 
+  private Result maybeFixRenamedFields(Result rightResult, Join e) {

Review Comment:
   This doesn't seem to have any relation to JDBC.
   Perhaps the issue should be renamed.



##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -225,7 +225,7 @@ public Result visit(Join e) {
   break;
 }
 final Result leftResult = visitInput(e, 0).resetAlias();
-final Result rightResult = visitInput(e, 1).resetAlias();
+Result rightResult = maybeFixRenamedFields(visitInput(e, 1).resetAlias(), 
e);

Review Comment:
   Can these two calls be on different lines? Helps with debugging.
   Looks like you can keep the "final" too.



##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -257,6 +257,48 @@ public Result visit(Join e) {
 return result(join, leftResult, rightResult);
   }
 
+  private Result maybeFixRenamedFields(Result rightResult, Join e) {

Review Comment:
   I suspect that this change may affect other open issues on JIRA, it looks 
pretty basic.
   Can you do a quick check?



##
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##
@@ -809,8 +809,8 @@ private static String toSql(RelNode root, SqlDialect 
dialect,
 // RelFieldTrimmer maybe build the RelNode.
 relFn(fn).ok("SELECT \"t\".\"V\" AS \"l_v\"\n"
 + "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"

Review Comment:
   It's not easy to review all these changes. I checked some of them.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-01-29 Thread via GitHub


kramerul commented on PR #3658:
URL: https://github.com/apache/calcite/pull/3658#issuecomment-1914826743

   This PR could cause some problems. We had trouble with a statement like
   
   ```
   SELECT 
   FROM "TA" AS "A"  
   INNER JOIN "TB" AS "B"
ON "A"."APPLICATION_ID" =  "B"."CHILD_APPLICATION_ID"
   INNER JOIN "TC" AS "C"
ON "C"."APPLICATION_ID" = "B"."PARENT_APPLICATION_ID"
   ```
   
   `"B"."CHILD_APPLICATION_ID"` was hidden from the additional SELECT.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-01-29 Thread via GitHub


sonarcloud[bot] commented on PR #3658:
URL: https://github.com/apache/calcite/pull/3658#issuecomment-1914753294

   ## [![Quality Gate 
Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png
 'Quality Gate 
Passed')](https://sonarcloud.io/dashboard?id=apache_calcite=3658) 
**Quality Gate passed**  
   The SonarCloud Quality Gate passed, but some issues were introduced.
   
   [2 New 
issues](https://sonarcloud.io/project/issues?id=apache_calcite=3658=false=true)
  
   [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3658=false=true)
  
   [98.0% Coverage on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3658=new_coverage=list)
  
   [0.0% Duplication on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3658=new_duplicated_lines_density=list)
  
 
   [See analysis details on 
SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3658)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-01-29 Thread via GitHub


kramerul commented on code in PR #3658:
URL: https://github.com/apache/calcite/pull/3658#discussion_r1469610198


##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -257,6 +257,47 @@ public Result visit(Join e) {
 return result(join, leftResult, rightResult);
   }
 
+  private Result maybeFixRenamedFields(Result rightResult, Join e) {
+Frame last = stack.peekLast();
+if (last != null && last.r instanceof TableModify) {
+  return rightResult;
+}
+List rightFieldNames = e.getRight().getRowType().getFieldNames();
+List fieldNames = e.getRowType().getFieldNames();
+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))) {
+hasFieldNameCollision = true;
+  }
+}
+if (!hasFieldNameCollision) {
+  return rightResult;
+}
+Builder builder = rightResult.builder(e);
+List oldSelectList;
+if (builder.select.getSelectList() == SqlNodeList.SINGLETON_STAR) {
+  oldSelectList = new ArrayList<>();
+  for (int i = 0; i < rightFieldNames.size(); i++) {
+oldSelectList.add(new SqlIdentifier(rightFieldNames.get(i), POS));
+  }
+} else {
+  oldSelectList = 
ImmutableList.copyOf(builder.select.getSelectList().getList());

Review Comment:
   I fixed the CheckerFramework finding.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-01-29 Thread via GitHub


JiajunBernoulli commented on code in PR #3658:
URL: https://github.com/apache/calcite/pull/3658#discussion_r1469580846


##
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##
@@ -257,6 +257,47 @@ public Result visit(Join e) {
 return result(join, leftResult, rightResult);
   }
 
+  private Result maybeFixRenamedFields(Result rightResult, Join e) {
+Frame last = stack.peekLast();
+if (last != null && last.r instanceof TableModify) {
+  return rightResult;
+}
+List rightFieldNames = e.getRight().getRowType().getFieldNames();
+List fieldNames = e.getRowType().getFieldNames();
+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))) {
+hasFieldNameCollision = true;
+  }
+}
+if (!hasFieldNameCollision) {
+  return rightResult;
+}
+Builder builder = rightResult.builder(e);
+List oldSelectList;
+if (builder.select.getSelectList() == SqlNodeList.SINGLETON_STAR) {
+  oldSelectList = new ArrayList<>();
+  for (int i = 0; i < rightFieldNames.size(); i++) {
+oldSelectList.add(new SqlIdentifier(rightFieldNames.get(i), POS));
+  }
+} else {
+  oldSelectList = 
ImmutableList.copyOf(builder.select.getSelectList().getList());

Review Comment:
   Please fix CI



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-01-29 Thread via GitHub


sonarcloud[bot] commented on PR #3658:
URL: https://github.com/apache/calcite/pull/3658#issuecomment-1914674069

   ## [![Quality Gate 
Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png
 'Quality Gate 
Passed')](https://sonarcloud.io/dashboard?id=apache_calcite=3658) 
**Quality Gate passed**  
   The SonarCloud Quality Gate passed, but some issues were introduced.
   
   [2 New 
issues](https://sonarcloud.io/project/issues?id=apache_calcite=3658=false=true)
  
   [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3658=false=true)
  
   [97.9% Coverage on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3658=new_coverage=list)
  
   [0.0% Duplication on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3658=new_duplicated_lines_density=list)
  
 
   [See analysis details on 
SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3658)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-01-29 Thread via GitHub


sonarcloud[bot] commented on PR #3658:
URL: https://github.com/apache/calcite/pull/3658#issuecomment-1914500576

   ## [![Quality Gate 
Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png
 'Quality Gate 
Passed')](https://sonarcloud.io/dashboard?id=apache_calcite=3658) 
**Quality Gate passed**  
   The SonarCloud Quality Gate passed, but some issues were introduced.
   
   [2 New 
issues](https://sonarcloud.io/project/issues?id=apache_calcite=3658=false=true)
  
   [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3658=false=true)
  
   [97.9% Coverage on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3658=new_coverage=list)
  
   [0.0% Duplication on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3658=new_duplicated_lines_density=list)
  
 
   [See analysis details on 
SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3658)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] [CALCITE-6221] JDBC adapter generates invalid query when the same table is joined multiple times [calcite]

2024-01-29 Thread via GitHub


sonarcloud[bot] commented on PR #3658:
URL: https://github.com/apache/calcite/pull/3658#issuecomment-1914337344

   ## [![Quality Gate 
Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png
 'Quality Gate 
Passed')](https://sonarcloud.io/dashboard?id=apache_calcite=3658) 
**Quality Gate passed**  
   The SonarCloud Quality Gate passed, but some issues were introduced.
   
   [2 New 
issues](https://sonarcloud.io/project/issues?id=apache_calcite=3658=false=true)
  
   [0 Security 
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3658=false=true)
  
   [100.0% Coverage on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3658=new_coverage=list)
  
   [0.0% Duplication on New 
Code](https://sonarcloud.io/component_measures?id=apache_calcite=3658=new_duplicated_lines_density=list)
  
 
   [See analysis details on 
SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3658)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org