[GitHub] [carbondata] akashrn5 opened a new pull request #3962: [wip]Fix the insert issue when the column name contains \ or backslash

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] akashrn5 opened a new pull request #3962: [wip]Fix the insert issue when the column name contains \ or backslash

GitBox

akashrn5 opened a new pull request #3962:
URL: https://github.com/apache/carbondata/pull/3962


    ### Why is this PR needed?
   
   
    ### 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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3962: [wip]Fix the insert issue when the column name contains \ or backslash

GitBox

CarbonDataQA1 commented on pull request #3962:
URL: https://github.com/apache/carbondata/pull/3962#issuecomment-700164625


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


----------------------------------------------------------------
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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3962: [wip]Fix the insert issue when the column name contains \ or backslash

GitBox
In reply to this post by GitBox

CarbonDataQA1 commented on pull request #3962:
URL: https://github.com/apache/carbondata/pull/3962#issuecomment-700169378


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


----------------------------------------------------------------
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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3962: [CARBONDATA-4017]Fix the insert issue when the column name contains '\' and fix SI creation issue

GitBox
In reply to this post by GitBox

CarbonDataQA1 commented on pull request #3962:
URL: https://github.com/apache/carbondata/pull/3962#issuecomment-701344984


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


----------------------------------------------------------------
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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3962: [CARBONDATA-4017]Fix the insert issue when the column name contains '\' and fix SI creation issue

GitBox
In reply to this post by GitBox

CarbonDataQA1 commented on pull request #3962:
URL: https://github.com/apache/carbondata/pull/3962#issuecomment-701346144


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


----------------------------------------------------------------
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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] Indhumathi27 commented on a change in pull request #3962: [CARBONDATA-4017]Fix the insert issue when the column name contains '\' and fix SI creation issue

GitBox
In reply to this post by GitBox

Indhumathi27 commented on a change in pull request #3962:
URL: https://github.com/apache/carbondata/pull/3962#discussion_r498158964



##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithSecondryIndex.scala
##########
@@ -423,6 +423,17 @@ class TestSIWithSecondryIndex extends QueryTest with BeforeAndAfterAll {
     sql("drop table table2")
   }
 
+  test("test SI creation with special char column") {
+    sql("drop table if exists special_char")

Review comment:
       can add drop table to afterAll also

##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java
##########
@@ -1881,7 +1881,9 @@ public static TableInfo convertGsonToTableInfo(Map<String, String> properties) {
     gsonBuilder.registerTypeAdapter(DataType.class, new DataTypeAdapter());
 
     Gson gson = gsonBuilder.create();
-    TableInfo tableInfo = gson.fromJson(builder.toString(), TableInfo.class);
+    // if the column name contains backslash in the column name, then fromJson will remove that,
+    // so replace like below to keep the "\" in column name and write the proper name in the schema
+    TableInfo tableInfo = gson.fromJson(builder.toString().replace("\\", "\\\\"), TableInfo.class);

Review comment:
       can you add a test case with carbon session

##########
File path: integration/spark/src/main/scala/org/apache/spark/sql/secondaryindex/command/SICreationCommand.scala
##########
@@ -340,7 +336,7 @@ private[sql] case class CarbonCreateSecondaryIndexCommand(
         try {
           sparkSession.sql(
             s"""CREATE TABLE $databaseName.$indexTableName
-               |(${ fields.mkString(",") })
+               |(${ rawSchema })

Review comment:
       ```suggestion
                  ($rawSchema)
   ```




----------------------------------------------------------------
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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] akashrn5 commented on a change in pull request #3962: [CARBONDATA-4017]Fix the insert issue when the column name contains '\' and fix SI creation issue

GitBox
In reply to this post by GitBox

akashrn5 commented on a change in pull request #3962:
URL: https://github.com/apache/carbondata/pull/3962#discussion_r498175600



##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java
##########
@@ -1881,7 +1881,9 @@ public static TableInfo convertGsonToTableInfo(Map<String, String> properties) {
     gsonBuilder.registerTypeAdapter(DataType.class, new DataTypeAdapter());
 
     Gson gson = gsonBuilder.create();
-    TableInfo tableInfo = gson.fromJson(builder.toString(), TableInfo.class);
+    // if the column name contains backslash in the column name, then fromJson will remove that,
+    // so replace like below to keep the "\" in column name and write the proper name in the schema
+    TableInfo tableInfo = gson.fromJson(builder.toString().replace("\\", "\\\\"), TableInfo.class);

Review comment:
       actually `QueryTest ` has spark session without `carbonSession`, and we cannot create another `sparkSession `when one is running. In example we cant add as test case, so i think since i have added the proper comment, it should be fine i guess.

##########
File path: index/secondary-index/src/test/scala/org/apache/carbondata/spark/testsuite/secondaryindex/TestSIWithSecondryIndex.scala
##########
@@ -423,6 +423,17 @@ class TestSIWithSecondryIndex extends QueryTest with BeforeAndAfterAll {
     sql("drop table table2")
   }
 
+  test("test SI creation with special char column") {
+    sql("drop table if exists special_char")

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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] Indhumathi27 commented on a change in pull request #3962: [CARBONDATA-4017]Fix the insert issue when the column name contains '\' and fix SI creation issue

GitBox
In reply to this post by GitBox

Indhumathi27 commented on a change in pull request #3962:
URL: https://github.com/apache/carbondata/pull/3962#discussion_r498176312



##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java
##########
@@ -1881,7 +1881,9 @@ public static TableInfo convertGsonToTableInfo(Map<String, String> properties) {
     gsonBuilder.registerTypeAdapter(DataType.class, new DataTypeAdapter());
 
     Gson gson = gsonBuilder.create();
-    TableInfo tableInfo = gson.fromJson(builder.toString(), TableInfo.class);
+    // if the column name contains backslash in the column name, then fromJson will remove that,
+    // so replace like below to keep the "\" in column name and write the proper name in the schema
+    TableInfo tableInfo = gson.fromJson(builder.toString().replace("\\", "\\\\"), TableInfo.class);

Review comment:
       you can add in CarbonSessionExample




----------------------------------------------------------------
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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] Indhumathi27 commented on a change in pull request #3962: [CARBONDATA-4017]Fix the insert issue when the column name contains '\' and fix SI creation issue

GitBox
In reply to this post by GitBox

Indhumathi27 commented on a change in pull request #3962:
URL: https://github.com/apache/carbondata/pull/3962#discussion_r498178139



##########
File path: core/src/main/java/org/apache/carbondata/core/util/CarbonUtil.java
##########
@@ -1881,7 +1881,9 @@ public static TableInfo convertGsonToTableInfo(Map<String, String> properties) {
     gsonBuilder.registerTypeAdapter(DataType.class, new DataTypeAdapter());
 
     Gson gson = gsonBuilder.create();
-    TableInfo tableInfo = gson.fromJson(builder.toString(), TableInfo.class);
+    // if the column name contains backslash in the column name, then fromJson will remove that,
+    // so replace like below to keep the "\" in column name and write the proper name in the schema
+    TableInfo tableInfo = gson.fromJson(builder.toString().replace("\\", "\\\\"), TableInfo.class);

Review comment:
       ok




----------------------------------------------------------------
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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3962: [CARBONDATA-4017]Fix the insert issue when the column name contains '\' and fix SI creation issue

GitBox
In reply to this post by GitBox

CarbonDataQA1 commented on pull request #3962:
URL: https://github.com/apache/carbondata/pull/3962#issuecomment-702138034


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


----------------------------------------------------------------
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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3962: [CARBONDATA-4017]Fix the insert issue when the column name contains '\' and fix SI creation issue

GitBox
In reply to this post by GitBox

CarbonDataQA1 commented on pull request #3962:
URL: https://github.com/apache/carbondata/pull/3962#issuecomment-702138964


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


----------------------------------------------------------------
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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] Indhumathi27 commented on pull request #3962: [CARBONDATA-4017]Fix the insert issue when the column name contains '\' and fix SI creation issue

GitBox
In reply to this post by GitBox

Indhumathi27 commented on pull request #3962:
URL: https://github.com/apache/carbondata/pull/3962#issuecomment-704028143


   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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3962: [CARBONDATA-4017]Fix the insert issue when the column name contains '\' and fix SI creation issue

GitBox
In reply to this post by GitBox

CarbonDataQA1 commented on pull request #3962:
URL: https://github.com/apache/carbondata/pull/3962#issuecomment-704068740


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


----------------------------------------------------------------
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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] CarbonDataQA1 commented on pull request #3962: [CARBONDATA-4017]Fix the insert issue when the column name contains '\' and fix SI creation issue

GitBox
In reply to this post by GitBox

CarbonDataQA1 commented on pull request #3962:
URL: https://github.com/apache/carbondata/pull/3962#issuecomment-704070143


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


----------------------------------------------------------------
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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] Indhumathi27 commented on pull request #3962: [CARBONDATA-4017]Fix the insert issue when the column name contains '\' and fix SI creation issue

GitBox
In reply to this post by GitBox

Indhumathi27 commented on pull request #3962:
URL: https://github.com/apache/carbondata/pull/3962#issuecomment-704178705


   LGTM


----------------------------------------------------------------
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]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [carbondata] asfgit closed pull request #3962: [CARBONDATA-4017]Fix the insert issue when the column name contains '\' and fix SI creation issue

GitBox
In reply to this post by GitBox

asfgit closed pull request #3962:
URL: https://github.com/apache/carbondata/pull/3962


   


----------------------------------------------------------------
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]