Tim Armstrong has posted comments on this change. Change subject: IMPALA-3945: Forbid create text table with nonsensical delimiter combinations. ......................................................................
Patch Set 2: (3 comments) Thanks! I realised there is another corner case with the default escape character that we need to fix. http://gerrit.cloudera.org:8080/#/c/3839/2/fe/src/main/java/com/cloudera/impala/analysis/CreateTableStmt.java File fe/src/main/java/com/cloudera/impala/analysis/CreateTableStmt.java: PS2, Line 278: fieldDelim != null && escapeChar != null Hmm, these null checks don't do anything, since we set the values to non-null just above. Can you remove them while you're here. We should also skip these escapeChar checks if escapeChar is DEFAULT_ESCAPE_CHAR, since that disables escaping. http://gerrit.cloudera.org:8080/#/c/3839/2/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java: Line 1508: AnalyzesError("create table functional.broken_text_table (c int) " + We should actually allow this one for a subtle reason: the default escape character '\0' means "no escaping" Line 1522: "Line delimiter and escape character have same value: byte 0"); We should actually allow this one for a subtle reason: the default escape character '\0' means "no escaping" -- To view, visit http://gerrit.cloudera.org:8080/3839 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I836b45f930e680ea03274405b90b1ee14822152b Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Yuanhao Luo <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
