[GitHub] [carbondata] maheshrajus opened a new pull request #4139: [WIP] update table for primitive column not working when complex child column name and primitive column name match

[GitBox]

maheshrajus opened a new pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139


   
   
    ### Why is this PR needed?
    need to update
   
    ### What changes were proposed in this PR?
   
       
    ### Does this PR introduce any user interface change?
    - No
    - Yes. (please explain the change and update document)
   
    ### Is any new testcase added?
    - No
    - Yes
   
       
   


--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA2 commented on pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#issuecomment-847067362


   Build Failed  with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5418/
   


--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA2 commented on pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#issuecomment-847068972


   Build Failed  with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/3673/
   


--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA2 commented on pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#issuecomment-847420023


   Build Failed  with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5420/
   


--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA2 commented on pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#issuecomment-847424196


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/3675/
   


--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

maheshrajus commented on pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#issuecomment-847551516


   retest this please


--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

ajantha-bhat commented on a change in pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#discussion_r638476624



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala
##########
@@ -257,6 +259,25 @@ class CarbonSpark2SqlParser extends CarbonDDLSqlParser {
     (SET ~> "(" ~> repsep(element, ",") <~ ")") ~
     ("=" ~> restInput) <~ opt(";") ^^ {
       case tab ~ columns ~ rest =>
+        // If update is received for complex data types then throw exception
+        var finalColumns = List.empty[String]
+        val updateColumns: java.util.List[String] = new util.ArrayList[String]()

Review comment:
       use `scala.collection.mutable.ListBuffer`, instead of java list




--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

ajantha-bhat commented on a change in pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#discussion_r638476920



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala
##########
@@ -257,6 +259,25 @@ class CarbonSpark2SqlParser extends CarbonDDLSqlParser {
     (SET ~> "(" ~> repsep(element, ",") <~ ")") ~
     ("=" ~> restInput) <~ opt(";") ^^ {
       case tab ~ columns ~ rest =>
+        // If update is received for complex data types then throw exception
+        var finalColumns = List.empty[String]
+        val updateColumns: java.util.List[String] = new util.ArrayList[String]()
+        columns.foreach { column =>
+          if (column.contains('.')) {
+            val columnFullName = column.split('.')
+            if (columnFullName.length >= 3) {

Review comment:
       you can combine condition 1 and 3 by keeping (columnFullName.length < 3 && condition 2)




--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

ajantha-bhat commented on a change in pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#discussion_r638477137



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala
##########
@@ -257,6 +259,25 @@ class CarbonSpark2SqlParser extends CarbonDDLSqlParser {
     (SET ~> "(" ~> repsep(element, ",") <~ ")") ~
     ("=" ~> restInput) <~ opt(";") ^^ {
       case tab ~ columns ~ rest =>
+        // If update is received for complex data types then throw exception
+        var finalColumns = List.empty[String]
+        val updateColumns: java.util.List[String] = new util.ArrayList[String]()
+        columns.foreach { column =>
+          if (column.contains('.')) {
+            val columnFullName = column.split('.')
+            if (columnFullName.length >= 3) {

Review comment:
       can you add some comments about why are you checking with 3 here ?




--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

ajantha-bhat commented on a change in pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#discussion_r638485028



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala
##########
@@ -377,7 +399,12 @@ class CarbonSpark2SqlParser extends CarbonDDLSqlParser {
 
   protected lazy val element: Parser[String] =
     (ident <~ ".").? ~ ident ^^ {
-      case table ~ column => column.toLowerCase
+      case table ~ column =>
+        if (table.isDefined) {
+          table.get + "." + column.toLowerCase

Review comment:
       No need to convert `table.get` to lowercase ?




--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

ajantha-bhat commented on a change in pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#discussion_r638485028



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala
##########
@@ -377,7 +399,12 @@ class CarbonSpark2SqlParser extends CarbonDDLSqlParser {
 
   protected lazy val element: Parser[String] =
     (ident <~ ".").? ~ ident ^^ {
-      case table ~ column => column.toLowerCase
+      case table ~ column =>
+        if (table.isDefined) {
+          table.get + "." + column.toLowerCase

Review comment:
       No need to convert `table.get` to lowercase ? you can modify the testcase to have the mixedCase and check it.




--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA2 commented on pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#issuecomment-847612963


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/3676/
   


--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA2 commented on pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#issuecomment-847615174


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5421/
   


--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

maheshrajus commented on a change in pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#discussion_r638645013



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala
##########
@@ -257,6 +259,25 @@ class CarbonSpark2SqlParser extends CarbonDDLSqlParser {
     (SET ~> "(" ~> repsep(element, ",") <~ ")") ~
     ("=" ~> restInput) <~ opt(";") ^^ {
       case tab ~ columns ~ rest =>
+        // If update is received for complex data types then throw exception
+        var finalColumns = List.empty[String]
+        val updateColumns: java.util.List[String] = new util.ArrayList[String]()

Review comment:
       done




--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

maheshrajus commented on a change in pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#discussion_r638645767



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala
##########
@@ -257,6 +259,25 @@ class CarbonSpark2SqlParser extends CarbonDDLSqlParser {
     (SET ~> "(" ~> repsep(element, ",") <~ ")") ~
     ("=" ~> restInput) <~ opt(";") ^^ {
       case tab ~ columns ~ rest =>
+        // If update is received for complex data types then throw exception
+        var finalColumns = List.empty[String]
+        val updateColumns: java.util.List[String] = new util.ArrayList[String]()
+        columns.foreach { column =>
+          if (column.contains('.')) {
+            val columnFullName = column.split('.')
+            if (columnFullName.length >= 3) {

Review comment:
       columnFullName.length >= 3  this case is removed as we can not give normally [tablename.complextype.childtype].




--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

maheshrajus commented on a change in pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#discussion_r638645983



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala
##########
@@ -257,6 +259,25 @@ class CarbonSpark2SqlParser extends CarbonDDLSqlParser {
     (SET ~> "(" ~> repsep(element, ",") <~ ")") ~
     ("=" ~> restInput) <~ opt(";") ^^ {
       case tab ~ columns ~ rest =>
+        // If update is received for complex data types then throw exception
+        var finalColumns = List.empty[String]
+        val updateColumns: java.util.List[String] = new util.ArrayList[String]()
+        columns.foreach { column =>
+          if (column.contains('.')) {
+            val columnFullName = column.split('.')
+            if (columnFullName.length >= 3) {

Review comment:
       columnFullName.length >= 3 this case is removed as we can not give normally [tablename.complextype.childtype].




--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

maheshrajus commented on a change in pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#discussion_r638656600



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala
##########
@@ -377,7 +399,12 @@ class CarbonSpark2SqlParser extends CarbonDDLSqlParser {
 
   protected lazy val element: Parser[String] =
     (ident <~ ".").? ~ ident ^^ {
-      case table ~ column => column.toLowerCase
+      case table ~ column =>
+        if (table.isDefined) {
+          table.get + "." + column.toLowerCase

Review comment:
       Done




--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

ajantha-bhat commented on a change in pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#discussion_r638713548



##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/parser/CarbonSpark2SqlParser.scala
##########
@@ -257,6 +258,23 @@ class CarbonSpark2SqlParser extends CarbonDDLSqlParser {
     (SET ~> "(" ~> repsep(element, ",") <~ ")") ~
     ("=" ~> restInput) <~ opt(";") ^^ {
       case tab ~ columns ~ rest =>
+        // If update is received for complex data types then throw exception
+        var finalColumns = List.empty[String]
+        var updateColumns = new ListBuffer[String]()
+        columns.foreach { column =>
+          if (column.contains('.')) {
+            val columnFullName = column.split('.')
+            if ((tab._3.isDefined && tab._3.get.equals(columnFullName(0)))
+                || tab._4.table.equals(columnFullName(0))) {
+              updateColumns += columnFullName(1)

Review comment:
       what about multi level complex column ?
   a struct<b int, c struct <d int, e int>>, so what happens if query contains a.c.e ?




--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA2 commented on pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#issuecomment-847808297


   Build Success with Spark 2.4.5, Please check CI http://121.244.95.60:12602/job/ApacheCarbon_PR_Builder_2.4.5/3677/
   


--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]

[GitBox]

CarbonDataQA2 commented on pull request #4139:
URL: https://github.com/apache/carbondata/pull/4139#issuecomment-847809817


   Build Success with Spark 2.3.4, Please check CI http://121.244.95.60:12602/job/ApacheCarbonPRBuilder2.3/5422/
   


--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]