[GitHub] [calcite] danny0405 commented on a change in pull request #2366: [CALCITE-4526] Fixup the unparse's output of SqlSnapshot,when tableRel is a SqlAsOperator (jibiyr)

[GitBox <gi...@apache.org>]

danny0405 commented on a change in pull request #2366:
URL: https://github.com/apache/calcite/pull/2366#discussion_r591270779



##########
File path: core/src/main/java/org/apache/calcite/sql/SqlAsOperator.java
##########
@@ -69,13 +70,25 @@ protected SqlAsOperator(String name, SqlKind kind, int prec,
       SqlCall call,
       int leftPrec,
       int rightPrec) {
+    unparse(writer, call, leftPrec, rightPrec, null);
+  }
+
+  public void unparse(
+      SqlWriter writer,
+      SqlCall call,
+      int leftPrec,
+      int rightPrec,
+      Consumer<Object> beforeWriteAlias) {
     assert call.operandCount() >= 2;
     final SqlWriter.Frame frame =
         writer.startList(
             SqlWriter.FrameTypeEnum.AS);
     call.operand(0).unparse(writer, leftPrec, getLeftPrec());
     final boolean needsSpace = true;
     writer.setNeedWhitespace(needsSpace);
+    if (beforeWriteAlias != null) {
+      beforeWriteAlias.accept(null);

Review comment:
       This is not necessary, please remove it.

##########
File path: core/src/main/java/org/apache/calcite/sql/SqlSnapshot.java
##########
@@ -127,10 +129,23 @@ private SqlSnapshotOperator() {
         int leftPrec,
         int rightPrec) {
       final SqlSnapshot snapshot = (SqlSnapshot) call;
-
-      snapshot.tableRef.unparse(writer, 0, 0);
-      writer.keyword("FOR SYSTEM_TIME AS OF");
-      snapshot.period.unparse(writer, 0, 0);
+      SqlNode tableRef = snapshot.tableRef;
+
+      if (tableRef instanceof SqlBasicCall
+          && ((SqlBasicCall) tableRef).getOperator() instanceof SqlAsOperator) {
+        SqlBasicCall basicCall = (SqlBasicCall) tableRef;
+        SqlAsOperator operator = (SqlAsOperator) ((SqlBasicCall) tableRef).getOperator();
+        operator.unparse(
+            writer, basicCall, 0, 0, ignore -> writeKeywordAndPeriod(writer, snapshot));

Review comment:
       There is no need to use the `SqlAsOperator#unparse`, just append the `AS table alias` in the last of the statement.

##########
File path: core/src/test/java/org/apache/calcite/sql/parser/SqlParserTest.java
##########
@@ -7766,6 +7770,32 @@ public void subTestIntervalSecondFailsValidation() {
         SqlParserUtil.addCarets("abcdef", 1, 7, 1, 7));
   }
 
+  @Test void testSnapshot() {
+    SqlNode tableRef = new SqlBasicCall(
+        new SqlAsOperator(),
+        new SqlNode[]{
+            new SqlIdentifier("default_db.products", new SqlParserPos(8, 11)),
+            new SqlIdentifier("products1", new SqlParserPos(8, 11))
+        },
+        new SqlParserPos(8, 11)
+    );
+    SqlNode period = new SqlIdentifier("orders.proctime", new SqlParserPos(8, 42));
+    SqlSnapshot sqlSnapshot = new SqlSnapshot(new SqlParserPos(8, 11), tableRef, period);
+
+    SqlWriter sqlWriter = new SqlPrettyWriter();
+    sqlSnapshot.unparse(sqlWriter, 0, 0);
+
+    assertEquals(sqlWriter.toSqlString().getSql(), "\"default_db.products\" "
+        + "FOR SYSTEM_TIME AS OF \"orders.proctime\" AS \"products1\"");

Review comment:
       No need to construct the `SqlNode` manually, there is a `SqlUnParserTest` that extends `SqlParserTest` already.
   
   If the test also succeed in `SqlUnParserTest`, that means the unparse sql can be parsed correctly.




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

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