[CALCITE-3486] In JDBC adapter, when generating ROW value expression, generates the ROW keyword only if the dialect allows it (quxiucheng) #1568

quxiucheng · 2019-11-08T16:48:29Z

MysqlSqlDialect unparse ROW keyword does not exist

amaliujia · 2019-11-08T17:16:33Z

core/src/test/java/org/apache/calcite/sql/parser/SqlParserTest.java

@@ -3699,6 +3700,14 @@ void checkPeriodPredicate(Checker checker) {
        .ok(expected);
  }

+  @Test public void testInsertDialect() {
+    // See [CALCITE-3486] MysqlSqlDialect unparse ROW keyword does not exist


You don't need this comment. If you actually want to say this test is testing ROW, you can rename this test as testInsertWithRowDialect.

Thank you.
modified

amaliujia · 2019-11-09T04:33:53Z

LGTM

danny0405 · 2019-11-10T01:26:45Z

core/src/main/java/org/apache/calcite/sql/dialect/MysqlSqlDialect.java

@@ -179,6 +179,9 @@ public boolean supportsAliasedValues() {

      unparseFloor(writer, call);
      break;
+    case ROW:
+      unparseRow(writer, call);


Thanks for the fix @quxiucheng, i think this patch can be promoted a little, we can move this logic to the parent class SqlDialect, you can get the SqlConformance from the dialect and decide if we should generates the ROW keyword based on method SqlConformance#allowExplicitRowValueConstructor .

[1]

calcite/core/src/main/java/org/apache/calcite/sql/validate/SqlConformance.java

Line 296 in 4d1c3e5

boolean allowExplicitRowValueConstructor();

done.
but the class SqlDialect can't get the method SqlConformance#allowExplicitRowValueConstructor correctly. I added a field in the class SqlDialect, Same as the field 'SqlConformance#allowExplicitRowValueConstructor'

What do you mean by "can not get method correctly", for MySQL, the conformance is SqlConformanceEnum.MYSQL_5 which i think is correct.

[1]

calcite/core/src/main/java/org/apache/calcite/sql/SqlDialect.java

Line 1092 in 4d1c3e5

@Nonnull public SqlConformance getConformance() {

We have two ways of creating dialects.

XxxSqlDialect.DEFAULT

SqlDialect.DatabaseProduct.XXX.getDialect()

If we only call org.apache.calcite.sql.SqlDialect#getConformance this method to get conformance.
Only compatible SqlDialect.DatabaseProduct.XXX.getDialect() method. So I reset a parameter allowExplicitRowValueConstructor to be compatible with both ways of creating the dialect.

XxxSqlDialect.DEFAULT also works, if we have set up the DatabaseProduct correctly. There is no overridden of method SqlDialect#getConformance, so i believe the behavior is correct.

I see.Thanks.

I have a question:

the method SqlConformanceEnum#allowExplicitRowValueConstructor

public boolean allowExplicitRowValueConstructor() { switch (this) { case DEFAULT: case LENIENT: return true; default: return false; } }

Only SqlConformanceEnum#DEFAULT and SqlConformanceEnum#LENIENT return values are true.
If this standard doesn't change. Test classes are mostly used by default method SqlConformanceEnum#PRAGMATIC_2003,
unparse will then have a lot of ROW keyword changes. Because they don't support the ROW keyword.
Should I add SqlConformanceEnum#PRAGMATIC_2003 to a method SqlConformanceEnum#allowExplicitRowValueConstructor or change the test class.

The return value "true" means we would print the "ROW" keyword, for SqlConformanceEnum#PRAGMATIC_2003, it is expected to not allows that.

Okay, I'm going to rewrite the code

danny0405 · 2019-11-10T01:28:05Z

core/src/test/java/org/apache/calcite/sql/parser/SqlParserTest.java

@@ -3699,6 +3700,13 @@ void checkPeriodPredicate(Checker checker) {
        .ok(expected);
  }

+  @Test public void testInsertWithRowDialect() {
+    final String expected = "INSERT INTO `emps`\n"


Maybe we can rename this method to testRowValueExpression and we should test dialect that allows/not allow explicit ROW keywords.

danny0405 · 2019-11-12T01:53:39Z

core/src/main/java/org/apache/calcite/sql/SqlDialect.java

@@ -158,6 +158,7 @@
  private final Casing unquotedCasing;
  private final Casing quotedCasing;
  private final boolean caseSensitive;
+  private final boolean allowExplicitRowValueConstructor;



Remove this allowExplicitRowValueConstructor flag, we expect to get this info from the SqlConformance.

yanlin-Lynn · 2019-11-12T02:56:54Z

core/src/test/java/org/apache/calcite/sql/parser/SqlParserTest.java

+    expected = "INSERT INTO `emps`\n"
+            + "VALUES (1, 'Fred'),\n"
+            + "(2, 'Eric')";
+    sql("insert into emps values (1,'Fred'),(2, 'Eric')")


Why different value for sql() method? Shall we use the same.

sorry,I didn't notice that
Write the wrong

danny0405 · 2019-11-12T12:08:17Z

core/src/main/java/org/apache/calcite/sql/SqlDialect.java

+      }
+      if (writer.isAlwaysUseParentheses()) {
+        for (SqlNode operand : call.getOperandList()) {
+          writer.sep(",");


Can you explain why we need this decision ?

This isAlwaysUseParentheses parameter will link operations with parentheses.
For example ,
(a + b) * c.The fully-parenthesized expression, ((a + b) * c)

If this parameter is in the SqlRowOperator. There will be extra parentheses
For example,
isAlwaysUseParentheses=true and allowExplicitRowValueConstructor=false

input expression:
insert into emp valuse (1,'a')

unparse result:
insert into emp valuse ((1,'a'))

I think this is a wrong SQL.

danny0405 · 2019-11-12T12:09:35Z

core/src/main/java/org/apache/calcite/sql/fun/SqlRowOperator.java

@@ -82,7 +82,7 @@ public void unparse(
      SqlCall call,
      int leftPrec,
      int rightPrec) {
-    SqlUtil.unparseFunctionSyntax(this, writer, call);
+    writer.getDialect().unparseRow(writer, call, leftPrec, rightPrec);
  }


We should not modify the unparse logic of SqlRowOperator, instead in SqlDialect, match the ROW call and unparse it specifically.

… generates the ROW keyword only if the dialect allows it

… generates the ROW keyword only if the dialect allows it (quxiucheng) In JDBC adapter, unparse the ROW keyword for ROW value expression only if the SQL dialect allows it. Fix-up (by Danny): * Simplify the logic for ROW value expression in SqlDialect#unparseCall * Add test cases in RelToSqlConverterTest * Fix SqlParserTest, there is no need to introduce new kind of SQL dialect close apache#1568

… generates the ROW keyword only if the dialect allows it (quxiucheng) In JDBC adapter, unparse the ROW keyword for ROW value expression only if the SQL dialect allows it. Fix-up (by Danny): * Simplify the logic for ROW value expression in SqlDialect#unparseCall * Add test cases in RelToSqlConverterTest * Fix SqlParserTest, there is no need to introduce new kind of SQL dialect close apache/calcite#1568 (cherry picked from commit ce0118b)

amaliujia reviewed Nov 8, 2019

View reviewed changes

quxiucheng force-pushed the CALCITE-3486 branch from bcdaa55 to 0e30131 Compare November 9, 2019 00:41

danny0405 requested changes Nov 10, 2019

View reviewed changes

danny0405 changed the title ~~[CALCITE-3486]MysqlSqlDialect unparse ROW keyword does not exist~~ [CALCITE-3486] In JDBC adapter, when generating ROW value expression, generates the ROW keyword only if the dialect allows it (quxiucheng) Nov 10, 2019

quxiucheng force-pushed the CALCITE-3486 branch 3 times, most recently from ce9dffa to 5357f0c Compare November 10, 2019 14:21

danny0405 reviewed Nov 12, 2019

View reviewed changes

yanlin-Lynn reviewed Nov 12, 2019

View reviewed changes

quxiucheng force-pushed the CALCITE-3486 branch 2 times, most recently from d0a6a47 to bb45dbc Compare November 12, 2019 09:45

danny0405 reviewed Nov 12, 2019

View reviewed changes

[CALCITE-3486] In JDBC adapter, when generating ROW value expression,…

1b5c110

… generates the ROW keyword only if the dialect allows it

quxiucheng force-pushed the CALCITE-3486 branch from bb45dbc to 1b5c110 Compare November 12, 2019 13:23

danny0405 closed this in ce0118b Nov 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-3486] In JDBC adapter, when generating ROW value expression, generates the ROW keyword only if the dialect allows it (quxiucheng) #1568

[CALCITE-3486] In JDBC adapter, when generating ROW value expression, generates the ROW keyword only if the dialect allows it (quxiucheng) #1568

quxiucheng commented Nov 8, 2019

amaliujia Nov 8, 2019 •

edited

Loading

quxiucheng Nov 9, 2019

amaliujia commented Nov 9, 2019

danny0405 Nov 10, 2019

quxiucheng Nov 10, 2019

danny0405 Nov 11, 2019

quxiucheng Nov 11, 2019

danny0405 Nov 11, 2019

quxiucheng Nov 11, 2019 •

edited

Loading

danny0405 Nov 11, 2019

quxiucheng Nov 11, 2019

danny0405 Nov 10, 2019

danny0405 Nov 12, 2019

yanlin-Lynn Nov 12, 2019 •

edited

Loading

quxiucheng Nov 12, 2019

danny0405 Nov 12, 2019

quxiucheng Nov 12, 2019

danny0405 Nov 12, 2019

quxiucheng Nov 12, 2019

[CALCITE-3486] In JDBC adapter, when generating ROW value expression, generates the ROW keyword only if the dialect allows it (quxiucheng) #1568

[CALCITE-3486] In JDBC adapter, when generating ROW value expression, generates the ROW keyword only if the dialect allows it (quxiucheng) #1568

Conversation

quxiucheng commented Nov 8, 2019

amaliujia Nov 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaliujia commented Nov 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quxiucheng Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanlin-Lynn Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaliujia Nov 8, 2019 •

edited

Loading

quxiucheng Nov 11, 2019 •

edited

Loading

yanlin-Lynn Nov 12, 2019 •

edited

Loading