Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/24205 )
Change subject: IMPALA-14884: Implement SHOW CURRENT GROUPS statement ...................................................................... Patch Set 7: (2 comments) > Patch Set 7: > > (2 comments) > > Some high level comments - the code looks good to me, but I didn't look that > deep. I will revise the commit message based on Csaba's suggestion and will submit the revised patch set soon. http://gerrit.cloudera.org:8080/#/c/24205/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24205/7//COMMIT_MSG@9 PS7, Line 9: SHOW CURRENT GROUPS > Is this syntax used by some other database or it is something new? It looks like no major relational database management system (RDBMS) (like MySQL, PostgreSQL, or SQL Server) uses the exact command of "SHOW CURRENT GROUPS". I chose to use "SHOW CURRENT GROUPS" because Impala already supports "SHOW CURRENT ROLES" that displays roles assigned to the current user as described at https://github.com/apache/impala/blob/master/docs/topics/impala_show.xml#L386C7-L392C11 so "SHOW CURRENT GROUPS" may be more intuitive for Impala users to adopt. > I don't have problems with syntax SHOW CURRENT GROUPS, but it could be noted > in commit message that this is non-standard and why you chose this solution. I will note in the commit message that this is non-standard and why I chose this solution. http://gerrit.cloudera.org:8080/#/c/24205/7//COMMIT_MSG@15 PS7, Line 15: This also slightly extends how 'struct_field_def' can grow via 'word' so : that we could still support the following statement afterward. : : CREATE TABLE t (i STRUCT<groups:INT>); > Where is this part in the code? I saw neither tests or parser changes around > complex types. I think we test this in https://github.com/apache/impala/blame/042b915/fe/src/test/java/org/apache/impala/analysis/ParserTest.java#L3663-L3671 as follows. Since we added "GROUPS" as a keyword in this patch, one more keyword was added to that keywordMap below, and hence we additionally had this test case run in ParserTest.java. // Test that struct-field names can be identifiers or keywords even if unquoted. // This behavior is needed to parse type strings from the Hive Metastore which // may have unquoted identifiers corresponding to keywords. for (String keyword: SqlScanner.keywordMap.keySet()) { // Skip keywords that are not valid field/column names in the Metastore. if (!MetastoreShim.validateName(keyword)) continue; String structType = "STRUCT<" + keyword + ":INT>"; TypeDefsParseOk(structType); } private void TypeDefsParseOk(String... typeDefs) { for (String typeDefStr: typeDefs) { ParsesOk(String.format("CREATE TABLE t (i %s)", typeDefStr)); ParsesOk(String.format("SELECT CAST (i AS %s)", typeDefStr)); // Test typeDefStr in complex types. ParsesOk(String.format("CREATE TABLE t (i MAP<%s, %s>)", typeDefStr, typeDefStr)); ParsesOk(String.format("CREATE TABLE t (i ARRAY<%s>)", typeDefStr)); ParsesOk(String.format("CREATE TABLE t (i STRUCT<f:%s>)", typeDefStr)); } } > It could be mentioned that GROUPS is already a reserved word (see > sql-scanner.flex // Add SQL:2016 reserved words) Yes I will add more details about this in the commit message. Specifically, before this patch, "CREATE TABLE t (i STRUCT<groups:INT>)" was supported because "GROUPS" was parsed as a SqlParserSymbols.UNUSED_RESERVED_WORD (https://github.com/apache/impala/blob/042b915/fe/src/main/jflex/sql-scanner.flex#L578), and UNUSED_RESERVED_WORD could be grown from 'word' at https://github.com/apache/impala/blob/042b915/fe/src/main/cup/sql-parser.cup#L4938. After this patch, since "GROUPS" is parsed as a keyword, we need to add KW_GROUPS to the production rule of 'word'. On a related note, the following is supported in Apache Hive, but it's not supported before and after this patch. create table t (groups string); -- To view, visit http://gerrit.cloudera.org:8080/24205 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7d65c6d038e9bb2d56c6c2510eaf52a8c27965c8 Gerrit-Change-Number: 24205 Gerrit-PatchSet: 7 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Wed, 06 May 2026 01:15:48 +0000 Gerrit-HasComments: Yes
