Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-3726: Add support for Kudu-specific column options ......................................................................
Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/5026/5/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: Line 402: nonterminal String ident_or_keyword, ident_or_default; > Can you move ident_or_keyword after the terminals and add a comment that th Done. Filed IMPALA-4489. Line 1498: // TODO: Consider introducing a DEFAULT keyword > remove Done http://gerrit.cloudera.org:8080/#/c/5026/5/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java File fe/src/main/java/org/apache/impala/analysis/ColumnDef.java: Line 220: // TODO: Similar checks are applied for range partition values in > Can we address this immediately after this patch goes in? The code is somew Agreed. http://gerrit.cloudera.org:8080/#/c/5026/5/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: Line 59: encodings = ["ENCODING AUTO_ENCODING", ""] > try some non-default ones? Done Line 250: c INT NOT NULL ENCODING AUTO_ENCODING COMPRESSION DEFAULT_COMPRESSION, > should we really print AUTO or DEFAULT values? For now I would say yes for the following reasons: 1. Until we implement describe for Kudu table, we don't have another way using Impala to see the full specification of the table columns. 2. In some case, it's not clear what the absence of a value means. For example, wrt compression, we have no_compression and default_compression. I believe we can revisit that decision when we have full fledged DESCRIBE. -- To view, visit http://gerrit.cloudera.org:8080/5026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I727b9ae1b7b2387db752b58081398dd3f3449c02 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-HasComments: Yes
