Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9096 )
Change subject: IMPALA-3916: Reserve SQL:2016 reserved words ...................................................................... Patch Set 4: (4 comments) 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 == "2.11.0") { > - This function is called in the ExecEnv constructor, before ExecEnv::Init( You are right. I was wrong to suggest ExecEnv::Init(). Still I don't think validation belongs here. Better to have that in init.cc in InitCommonRuntime(). For example, that's where we validate FLAGS_read_size which is passed here in L62 without validation. 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: > The scanner needs a BuiltinsDB to get a list of builtin functions. I chose Got it. Seems cleaner not to pass Catalog here anyway. 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: // Reserved words = keywordMap + sql16ReservedWords > This procedure just determines the set of the (unused) reserved words, not I see. I think we need to more clearly distinguish the two sets. The comment here states that the set of reserved words contains all keywords and sql16keywords minus builtin functions and reservation exceptions. That seems wrong. If the reserved words are unused, then they should not contain the keywordMap. 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(); > The old code constructs a "new TBackendGflags()" at L1221, where the reserv Got it, that's subtle. * Please update StatsExtrapolationTest to use the same pattern as here. * Alternatively, give reserved_words_version a default value in the .thrift -- 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: 4 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 02:10:16 +0000 Gerrit-HasComments: Yes
