[GitHub] carbondata pull request #1653: [CARBONDATA-1893] Data load with multiple QUO...

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1653
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/2313/



---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1653#discussion_r157150591
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/EscapeSequences.java ---
    @@ -0,0 +1,49 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.sql.catalyst;
    +
    +public enum EscapeSequences {
    --- End diff --
   
    Please avoid using public


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1653#discussion_r157150632
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/EscapeSequences.java ---
    @@ -0,0 +1,49 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.sql.catalyst;
    +
    +public enum EscapeSequences {
    +
    +  NEW_LINE("\\n", '\n'), BACKSPACE("\\b", '\b'), TAB("\\t", '\t'), CARRIAGE_RETURN("\\r", '\r');
    +
    +  /**
    +   * name of the function
    +   */
    +  private String name;
    +
    +  /**
    +   * unicode of the escapechar
    +   */
    +  private char escapeChar;
    +
    +
    +  EscapeSequences(String name, char escapeChar) {
    +    this.name = name;
    +    this.escapeChar = escapeChar;
    +  }
    +
    +  public String getName() {
    +    return this.name;
    +  }
    +
    +  public String getEscapeChar() {
    +    return String.valueOf(this.escapeChar);
    +  }
    +
    +
    +}
    --- End diff --
   
    new line is needed


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1653#discussion_r157151580
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -871,6 +871,32 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
           throw new MalformedCarbonCommandException(errorMessage)
         }
     
    +    // Validate QUOTECHAR length
    +    if (options.exists(_._1.equalsIgnoreCase("QUOTECHAR"))) {
    +      val quoteChar: String = options.get("quotechar").get.head._2
    +      if (quoteChar.length > 1 ) {
    +        throw new MalformedCarbonCommandException("Quotation cannot be more than one character.")
    +      }
    +    }
    +
    +    // Validate COMMENTCHAR length
    +    if (options.exists(_._1.equalsIgnoreCase("COMMENTCHAR"))) {
    +      val commentChar: String = options.get("commentchar").get.head._2
    +      if (commentChar.length > 1) {
    +        throw new MalformedCarbonCommandException(
    +          "Comment marker cannot be more than one character.")
    --- End diff --
   
    change `Comment marker` to `COMMENTCHAR`


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1653#discussion_r157151611
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -871,6 +871,32 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
           throw new MalformedCarbonCommandException(errorMessage)
         }
     
    +    // Validate QUOTECHAR length
    +    if (options.exists(_._1.equalsIgnoreCase("QUOTECHAR"))) {
    +      val quoteChar: String = options.get("quotechar").get.head._2
    +      if (quoteChar.length > 1 ) {
    +        throw new MalformedCarbonCommandException("Quotation cannot be more than one character.")
    --- End diff --
   
    change to `QUOTECHAR`


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1653#discussion_r157151644
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -871,6 +871,32 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
           throw new MalformedCarbonCommandException(errorMessage)
         }
     
    +    // Validate QUOTECHAR length
    +    if (options.exists(_._1.equalsIgnoreCase("QUOTECHAR"))) {
    +      val quoteChar: String = options.get("quotechar").get.head._2
    +      if (quoteChar.length > 1 ) {
    +        throw new MalformedCarbonCommandException("Quotation cannot be more than one character.")
    +      }
    +    }
    +
    +    // Validate COMMENTCHAR length
    +    if (options.exists(_._1.equalsIgnoreCase("COMMENTCHAR"))) {
    +      val commentChar: String = options.get("commentchar").get.head._2
    +      if (commentChar.length > 1) {
    +        throw new MalformedCarbonCommandException(
    +          "Comment marker cannot be more than one character.")
    +      }
    +    }
    +
    +    // Validate ESCAPECHAR length
    +    if (options.exists(_._1.equalsIgnoreCase("ESCAPECHAR"))) {
    +      val escapechar: String = options.get("escapechar").get.head._2
    +      if (escapechar.length > 1 && !CommonUtil.isValidEscapeSequence(escapechar)) {
    +        throw new MalformedCarbonCommandException(
    +          "Escape character cannot be more than one character.")
    --- End diff --
   
    change to `ESCAPECHAR`


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1653#discussion_r157151775
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/CarbonDDLSqlParser.scala ---
    @@ -871,6 +871,32 @@ abstract class CarbonDDLSqlParser extends AbstractCarbonSparkSQLParser {
           throw new MalformedCarbonCommandException(errorMessage)
         }
     
    +    // Validate QUOTECHAR length
    +    if (options.exists(_._1.equalsIgnoreCase("QUOTECHAR"))) {
    +      val quoteChar: String = options.get("quotechar").get.head._2
    +      if (quoteChar.length > 1 ) {
    +        throw new MalformedCarbonCommandException("Quotation cannot be more than one character.")
    +      }
    +    }
    +
    +    // Validate COMMENTCHAR length
    +    if (options.exists(_._1.equalsIgnoreCase("COMMENTCHAR"))) {
    +      val commentChar: String = options.get("commentchar").get.head._2
    +      if (commentChar.length > 1) {
    +        throw new MalformedCarbonCommandException(
    +          "Comment marker cannot be more than one character.")
    --- End diff --
   
    move to previous line


---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1653
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/2314/



---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1653#discussion_r157152181
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/carbondata/spark/util/CommonUtil.scala ---
    @@ -649,6 +650,25 @@ object CommonUtil {
         csvColumns
       }
     
    +  def isValidEscapeSequence(escapeChar: String): Boolean = {
    +    escapeChar.equals(NEW_LINE.getName) || escapeChar.equals(CARRIAGE_RETURN.getName) ||
    +    escapeChar.equals(TAB.getName) || escapeChar.equals(BACKSPACE.getName)
    +  }
    +
    +  def getEscapeChar(escapeCharacter: String): String = {
    --- End diff --
   
    move to a private function in CarbonLoadUtil


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1653#discussion_r157152320
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/carbondata/spark/util/CommonUtil.scala ---
    @@ -649,6 +650,25 @@ object CommonUtil {
         csvColumns
       }
     
    +  def isValidEscapeSequence(escapeChar: String): Boolean = {
    --- End diff --
   
    move to a private function in CarbonLoadUtil


---

[qiuchenjian-2]

Github user dhatchayani commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/1653#discussion_r157153651
 
    --- Diff: integration/spark-common/src/main/scala/org/apache/spark/sql/catalyst/EscapeSequences.java ---
    @@ -0,0 +1,49 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one or more
    + * contributor license agreements.  See the NOTICE file distributed with
    + * this work for additional information regarding copyright ownership.
    + * The ASF licenses this file to You under the Apache License, Version 2.0
    + * (the "License"); you may not use this file except in compliance with
    + * the License.  You may obtain a copy of the License at
    + *
    + *    http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.spark.sql.catalyst;
    +
    +public enum EscapeSequences {
    --- End diff --
   
    if we donot use public then it cannot be accessed from other classes


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1653
 
    Build Success with Spark 2.2.0, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/771/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1653
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/2317/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1653
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/2004/



---

[qiuchenjian-2]

Github user dhatchayani commented on the issue:

    https://github.com/apache/carbondata/pull/1653
 
    retest this please


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1653
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/2009/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1653
 
    Build Success with Spark 2.2.0, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/782/



---

[qiuchenjian-2]

Github user ravipesala commented on the issue:

    https://github.com/apache/carbondata/pull/1653
 
    SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/2327/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1653
 
    Build Success with Spark 2.2.0, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/787/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/1653
 
    Build Failed  with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/2021/



---