Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/9096 )
Change subject: IMPALA-3916: Reserve SQL:2016 reserved words ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/9096/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9096/3//COMMIT_MSG@15 PS3, Line 15: reserve_impala3_words is added. It is true by default. > update startup option Done http://gerrit.cloudera.org:8080/#/c/9096/3/be/src/util/backend-gflag-util.cc File be/src/util/backend-gflag-util.cc: http://gerrit.cloudera.org:8080/#/c/9096/3/be/src/util/backend-gflag-util.cc@81 PS3, Line 81: if (FLAGS_reserved_words_version == "IMPALA2.11") { > We generally don't validate flags in this part of the code. - This function is called in the ExecEnv constructor, before ExecEnv::Init(). Moving it to ExecEnv::Init() would unnecessarily change the initialization code structure. - Done http://gerrit.cloudera.org:8080/#/c/9096/3/common/thrift/BackendGflags.thrift File common/thrift/BackendGflags.thrift: http://gerrit.cloudera.org:8080/#/c/9096/3/common/thrift/BackendGflags.thrift@22 PS3, Line 22: IMPALA_2_11 > indent 2 Done http://gerrit.cloudera.org:8080/#/c/9096/3/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java File fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java: http://gerrit.cloudera.org:8080/#/c/9096/3/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java@a59 PS3, Line 59: > I like this and the associated changes, but they seem irrelevant to this pa The scanner needs a BuiltinsDB to get a list of builtin functions. I chose to initilize a BuiltinsDb in the scanner instead of constructing a Catalog instance in tests. To do so I need to remove the catalog parameter here. http://gerrit.cloudera.org:8080/#/c/9096/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/9096/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@a1412 PS3, Line 1412: > Nice cleanup, but let's keep the changes minimal please and do the cleanup Done http://gerrit.cloudera.org:8080/#/c/9096/3/fe/src/main/jflex/sql-scanner.flex File fe/src/main/jflex/sql-scanner.flex: http://gerrit.cloudera.org:8080/#/c/9096/3/fe/src/main/jflex/sql-scanner.flex@59 PS3, Line 59: // - builtinFunctions - reservationExceptions > This dynamic procedure doesn't seem right. It means we can eliminate a rese This procedure just determines the set of the (unused) reserved words, not keywords. If INTERSECT has been a keyword in use and then you want to add a builtin function with the same name, you have to change the parser. Meanwhile it remains a keyword, cannot be used as a table name, and this file doesn't need any change. http://gerrit.cloudera.org:8080/#/c/9096/3/fe/src/main/jflex/sql-scanner.flex@106 PS3, Line 106: static final HashSet<String> reservationExceptions = > Add comment Done http://gerrit.cloudera.org:8080/#/c/9096/3/fe/src/main/jflex/sql-scanner.flex@357 PS3, Line 357: throw new IllegalStateException("Should not have gotten here."); > Unhandled reserved words version: <print version string> Done http://gerrit.cloudera.org:8080/#/c/9096/3/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/9096/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1219 PS3, Line 1219: TBackendGflags gflags = BackendConfig.INSTANCE.getBackendCfg(); > Why? This will just cause merge conflicts The old code constructs a "new TBackendGflags()" at L1221, where the reserved_words_version field is null. The scanner won't work. http://gerrit.cloudera.org:8080/#/c/9096/3/tests/custom_cluster/test_stats_extrapolation.py File tests/custom_cluster/test_stats_extrapolation.py: http://gerrit.cloudera.org:8080/#/c/9096/3/tests/custom_cluster/test_stats_extrapolation.py@75 PS3, Line 75: empty_test_tbl = unique_database + ".`empty`" > empty_tbl Done http://gerrit.cloudera.org:8080/#/c/9096/3/tests/query_test/test_sort.py File tests/query_test/test_sort.py: http://gerrit.cloudera.org:8080/#/c/9096/3/tests/query_test/test_sort.py@145 PS3, Line 145: select `empty`, l_orderkey, l_partkey, l_suppkey, > empty_str Done -- To view, visit http://gerrit.cloudera.org:8080/9096 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If1b295e6a77e840cf1b794c2eb73e1b9d2b8ddd6 Gerrit-Change-Number: 9096 Gerrit-PatchSet: 3 Gerrit-Owner: Tianyi Wang <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Comment-Date: Tue, 30 Jan 2018 01:17:37 +0000 Gerrit-HasComments: Yes
