Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12568 )
Change subject: IMPALA-7645: Add a query option to set the default table file format ...................................................................... Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/12568/5/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/12568/5/be/src/service/query-options.cc@785 PS5, Line 785: TEXT, RC_FILE, SEQUENCE_FILE, AVRO, PARQUET, KUDU " : "and ORC. > nit: will probably become stale. My thinking is every time we add a new file format, we will definitely need to update this file because of the if/else logic. So technically we should never miss updating the error message. It's also nice to know which values are allowed since we aren't very consistent in matching the enum names vs the SQL keywords, e.g. TEXTFILE (SQL) vs TEXT (enum) RCFILE (SQL) vs RC_FILE (enum) etc. Let me know what you think. http://gerrit.cloudera.org:8080/#/c/12568/5/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/12568/5/common/thrift/ImpalaInternalService.thrift@328 PS5, Line 328: at > Can we just set the default here? default_file_format = TEXT Good idea. Done. http://gerrit.cloudera.org:8080/#/c/12568/5/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/12568/5/fe/src/main/cup/sql-parser.cup@1322 PS5, Line 1322: comment, parser.getQueryOptions() > Also, how about moving this into tbl_options? There we can resolve the effe The syntax for CREATE TABLE <table> PRODUCED BY DATA SOURCE <data source> doesn't support tbl_options. The only thing it supports from tbl_options is comment. That's probably why it's slightly different than regular CREATE TABLE. http://gerrit.cloudera.org:8080/#/c/12568/5/fe/src/main/cup/sql-parser.cup@1322 PS5, Line 1322: parser.getQueryOptions( > Do we need to pass the whole opts list? Seems kinda weird that TableDef has No required, but it's actually nice if in the future we need to get more things from query options. We no longer need to update the parser. Similar example: https://github.com/apache/impala/blob/master/fe/src/main/cup/sql-parser.cup#L3014-L3020 http://gerrit.cloudera.org:8080/#/c/12568/5/fe/src/main/java/org/apache/impala/analysis/TableDef.java File fe/src/main/java/org/apache/impala/analysis/TableDef.java: http://gerrit.cloudera.org:8080/#/c/12568/5/fe/src/main/java/org/apache/impala/analysis/TableDef.java@137 PS5, Line 137: / The file format passed via STORED AS <file format> has a higher precedence than : // the one set in query options. : this.fileFormat = (fileFormat != null) ? : fileFormat : queryOptions.getDefault_file_format(); : this.location = location; : this.cachingOp = cachingOp; : Preconditions.checkNotNull(tblProperties); : t > I think we can move all of this into the parser itself. Please refer to my I'm actually not a big fan of putting too much logic in the parser. It can make the code somewhat unreadable. -- To view, visit http://gerrit.cloudera.org:8080/12568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic857c38076d973ad749a41fecd1b470c7881db5e Gerrit-Change-Number: 12568 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 26 Feb 2019 05:00:28 +0000 Gerrit-HasComments: Yes
