julianhyde commented on code in PR #3375:
URL: https://github.com/apache/calcite/pull/3375#discussion_r1300746348
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -451,7 +453,8 @@ public Result visit(Project e) {
}
parseCorrelTable(e, x);
final Builder builder = x.builder(e);
- if (!isStar(e.getProjects(), e.getInput().getRowType(), e.getRowType())) {
+ if (isJoinDuplicateName(e.getInput()) || !isStar(e.getProjects(),
e.getInput().getRowType(),
Review Comment:
Break the line so that '||' is at start of line.
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -478,6 +481,26 @@ public Result visit(Project e) {
return builder.result();
}
+ /**
+ * Whether the relNode is a join and whether its inputs have duplicate field
names.
+ * For example, EMP JOIN DEPT, both of which have columns named DEPTNO
+ */
+ private boolean isJoinDuplicateName(RelNode relNode) {
+ if (relNode instanceof Join || relNode instanceof MultiJoin) {
+ List<RelNode> inputs = relNode.getInputs();
+ RelNode firstRel = inputs.get(0);
+ List<String> firstRelFiledNames =
Lists.newArrayList(firstRel.getRowType().getFieldNames());
+ for (int i = 1; i < inputs.size(); i++) {
+ RelNode input = inputs.get(i);
Review Comment:
What if the target SQL dialect is case-insensitive? For such a dialect
"select t0.x, t1.X from t0, t1" would be a problem.
Maybe add a `boolean caseSensitive` parameter to `Util.isDistinct`, default
true, and if it's false create a set with `new
TreeSet<String>(String.CASE_INSENSITIVE_ORDER)`
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -3529,21 +3553,39 @@ private SqlDialect nonOrdinalDialect() {
+ "from \"foodmart\".\"employee\" A "
+ "join \"foodmart\".\"department\" B\n"
+ "on A.\"department_id\" = B.\"department_id\"";
- final String expected = "SELECT *\n"
+ final String expected = "SELECT employee.employee_id, employee.full_name,
employee.first_name, "
+ + "employee.last_name, employee.position_id, employee.position_title,
employee.store_id, "
+ + "employee.department_id, employee.birth_date, employee.hire_date,
employee.end_date, "
+ + "employee.salary, employee.supervisor_id, employee.education_level, "
+ + "employee.marital_status, employee.gender, employee.management_role,
"
+ + "department.department_id AS department_id0,
department.department_description\n"
+ "FROM foodmart.employee AS employee\n"
- + "INNER JOIN foodmart.department AS department "
- + "ON employee.department_id = department.department_id";
Review Comment:
this diff (and a few others below) is spurious. you've just moved `ON` from
one line to the next. please revert to the original formatting.
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -478,6 +481,26 @@ public Result visit(Project e) {
return builder.result();
}
+ /**
+ * Whether the relNode is a join and whether its inputs have duplicate field
names.
+ * For example, EMP JOIN DEPT, both of which have columns named DEPTNO
+ */
+ private boolean isJoinDuplicateName(RelNode relNode) {
+ if (relNode instanceof Join || relNode instanceof MultiJoin) {
+ List<RelNode> inputs = relNode.getInputs();
+ RelNode firstRel = inputs.get(0);
+ List<String> firstRelFiledNames =
Lists.newArrayList(firstRel.getRowType().getFieldNames());
+ for (int i = 1; i < inputs.size(); i++) {
+ RelNode input = inputs.get(i);
Review Comment:
logic doesn't seem right if there are more than two inputs, and inputs 1 and
2 (0-based) have duplicate fields.
maybe write the field names into a list, and use `Util.isDistinct`
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/RelToSqlConverter.java:
##########
@@ -478,6 +481,26 @@ public Result visit(Project e) {
return builder.result();
}
+ /**
+ * Whether the relNode is a join and whether its inputs have duplicate field
names.
+ * For example, EMP JOIN DEPT, both of which have columns named DEPTNO
+ */
+ private boolean isJoinDuplicateName(RelNode relNode) {
+ if (relNode instanceof Join || relNode instanceof MultiJoin) {
+ List<RelNode> inputs = relNode.getInputs();
+ RelNode firstRel = inputs.get(0);
+ List<String> firstRelFiledNames =
Lists.newArrayList(firstRel.getRowType().getFieldNames());
+ for (int i = 1; i < inputs.size(); i++) {
+ RelNode input = inputs.get(i);
Review Comment:
If it is a join with `USING` or `NATURAL JOIN` then the duplicate columns
will have been filtered out. It's worth adding a test for `select * from emp
join dept using (deptno)` and `select * from emp natural join dept` and make
sure that `deptno` only occurs once in the output.
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -3750,17 +3792,35 @@ private SqlDialect nonOrdinalDialect() {
+ "on \"t1\".\"product_id\" = \"t3\".\"product_id\" or "
+ "(\"t1\".\"product_id\" is not null or "
+ "\"t3\".\"product_id\" is not null)";
- String expected = "SELECT *\n"
+ String expected = "SELECT \"sales_fact_1997\".\"product_id\",
\"sales_fact_1997\".\"time_id\", "
+ + "\"sales_fact_1997\".\"customer_id\",
\"sales_fact_1997\".\"promotion_id\", "
+ + "\"sales_fact_1997\".\"store_id\",
\"sales_fact_1997\".\"store_sales\", "
+ + "\"sales_fact_1997\".\"store_cost\",
\"sales_fact_1997\".\"unit_sales\", "
+ + "\"customer\".\"customer_id\" AS \"customer_id0\",
\"customer\".\"account_num\", "
+ + "\"customer\".\"lname\", \"customer\".\"fname\",
\"customer\".\"mi\", "
+ + "\"customer\".\"address1\", \"customer\".\"address2\",
\"customer\".\"address3\", "
+ + "\"customer\".\"address4\", \"customer\".\"city\",
\"customer\".\"state_province\", "
+ + "\"customer\".\"postal_code\", \"customer\".\"country\", "
+ + "\"customer\".\"customer_region_id\", \"customer\".\"phone1\",
\"customer\".\"phone2\", "
+ + "\"customer\".\"birthdate\", \"customer\".\"marital_status\", "
+ + "\"customer\".\"yearly_income\", \"customer\".\"gender\", "
+ + "\"customer\".\"total_children\",
\"customer\".\"num_children_at_home\", "
+ + "\"customer\".\"education\", \"customer\".\"date_accnt_opened\", "
+ + "\"customer\".\"member_card\", \"customer\".\"occupation\",
\"customer\".\"houseowner\", "
+ + "\"customer\".\"num_cars_owned\", \"customer\".\"fullname\", "
+ + "\"product\".\"product_class_id\", \"product\".\"product_id\" AS
\"product_id0\", "
+ + "\"product\".\"brand_name\", \"product\".\"product_name\",
\"product\".\"SKU\", "
+ + "\"product\".\"SRP\", \"product\".\"gross_weight\",
\"product\".\"net_weight\", "
+ + "\"product\".\"recyclable_package\", \"product\".\"low_fat\", "
+ + "\"product\".\"units_per_case\", \"product\".\"cases_per_pallet\", "
+ + "\"product\".\"shelf_width\", \"product\".\"shelf_height\",
\"product\".\"shelf_depth\"\n"
+ "FROM \"foodmart\".\"sales_fact_1997\"\n"
- + "INNER JOIN \"foodmart\".\"customer\" "
- + "ON \"sales_fact_1997\".\"customer_id\" =
\"customer\".\"customer_id\""
- + " OR \"sales_fact_1997\".\"customer_id\" IS NULL"
- + " AND \"customer\".\"customer_id\" IS NULL"
- + " OR \"customer\".\"occupation\" IS NULL\n"
- + "INNER JOIN \"foodmart\".\"product\" "
- + "ON \"sales_fact_1997\".\"product_id\" = \"product\".\"product_id\""
- + " OR \"sales_fact_1997\".\"product_id\" IS NOT NULL"
- + " OR \"product\".\"product_id\" IS NOT NULL";
+ + "INNER JOIN \"foodmart\".\"customer\" ON
\"sales_fact_1997\".\"customer_id\" = "
Review Comment:
this diff is spurious. and the formatting is worse than before.
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -7013,6 +7157,38 @@ private void checkLiteral2(String expression, String
expected) {
.withOracle(11).throws_("Lower Oracle version(<12) doesn't support
offset/fetch syntax!");
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-5583";>[CALCITE-5583]
+ * Rel2Sql will get an error when select * and join is present</a>. */
+ @Test void testSelectStarWithJoin() {
+ final RelBuilder builder = relBuilder();
Review Comment:
modern tests use the `relFn()` method rather than `toSql(RelNode)`; don't
call `relBuilder()` explicitly
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
