Baike Xia has posted comments on this change. ( http://gerrit.cloudera.org:8080/18940 )
Change subject: IMPALA-11420: Support CREATE/ALTER VIEW SET/UNSET TBLPROPERTIES ...................................................................... Patch Set 6: (7 comments) Hi all, I have repaired it, and it is ready for review. Thanks. http://gerrit.cloudera.org:8080/#/c/18940/5/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/18940/5/fe/src/main/cup/sql-parser.cup@2347 PS5, Line 2347: tbl_properties:tbl_props > nit: can we simplify these to "tbl_properties"? Yes, this is right. http://gerrit.cloudera.org:8080/#/c/18940/5/fe/src/main/java/org/apache/impala/analysis/AlterViewSetTblProperties.java File fe/src/main/java/org/apache/impala/analysis/AlterViewSetTblProperties.java: http://gerrit.cloudera.org:8080/#/c/18940/5/fe/src/main/java/org/apache/impala/analysis/AlterViewSetTblProperties.java@71 PS5, Line 71: if (!(tableRef instanceof InlineViewRef)) { > Does this work for materialized view (MV)? I think we represent MV as table Yeah, not supposed to work. In the new submission, I added a judgment, and throw exception if it was not InlineViewRef. Like you said, we represent MV as tables, so, will also throw exception. http://gerrit.cloudera.org:8080/#/c/18940/5/fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java File fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java: http://gerrit.cloudera.org:8080/#/c/18940/5/fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java@20 PS5, Line 20: import java.util.ArrayList; > nit: we avoid using * in imports Got http://gerrit.cloudera.org:8080/#/c/18940/5/fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java@22 PS5, Line 22: import java.util.List; > nit: move this to line 33 Got http://gerrit.cloudera.org:8080/#/c/18940/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/18940/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@341 PS5, Line 341: viewName > nit: viewName Got http://gerrit.cloudera.org:8080/#/c/18940/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/18940/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1501 PS5, Line 1501: AnalyzesOk("ALTER VIEW functional.alltypes_view SET TBLPROPERTIES " + > nit: don't need this trivial comment Got http://gerrit.cloudera.org:8080/#/c/18940/5/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1511 PS5, Line 1511: AnalysisError("alter view functional_orc_def.mv1_alltypes_jointbl " + > Could you add analyze test for CreateView with tblproperties as well? Got -- To view, visit http://gerrit.cloudera.org:8080/18940 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8d05bb4ec1f70f5387bb21fbe23f62c05941af18 Gerrit-Change-Number: 18940 Gerrit-PatchSet: 6 Gerrit-Owner: Baike Xia <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Baike Xia <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Wed, 14 Sep 2022 15:29:56 +0000 Gerrit-HasComments: Yes
