[GitHub] [carbondata] akkio-97 opened a new pull request #3920: [CARBONDATA-3981] Presto filter check on binary datatype

[GitBox]

akkio-97 opened a new pull request #3920:
URL: https://github.com/apache/carbondata/pull/3920


    ### Why is this PR needed?
    Due to the absence of binary datatype check, there was a problem during object serialisation in presto filter queries.
   
    ### What changes were proposed in this PR?
   Binary datatype check has been added in prestoFIlterUtil.java
       
    ### Does this PR introduce any user interface change?
    - No
   
    ### Is any new testcase added?
    - 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]

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


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


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

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


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


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

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


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


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

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


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


----------------------------------------------------------------
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 #3920:
URL: https://github.com/apache/carbondata/pull/3920#discussion_r486758917



##########
File path: integration/presto/src/main/prestosql/org/apache/carbondata/presto/PrestoFilterUtil.java
##########
@@ -78,6 +78,8 @@ private static DataType spi2CarbondataTypeMapper(HiveColumnHandle columnHandle)
     HiveType colType = columnHandle.getHiveType();
     if (colType.equals(HiveType.HIVE_BOOLEAN)) {
       return DataTypes.BOOLEAN;
+    } else if (colType.equals(HiveType.HIVE_BINARY)) {

Review comment:
       I can see byte and float data type is also missing. can you add and test for 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]

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



##########
File path: integration/presto/src/test/scala/org/apache/carbondata/presto/integrationtest/PrestoTestNonTransactionalTableFiles.scala
##########
@@ -230,6 +230,37 @@ class PrestoTestNonTransactionalTableFiles extends FunSuiteLike with BeforeAndAf
     }
   }
 
+  def buildOnlyBinary(rows: Int, sortColumns: Array[String], path : String): Any = {

Review comment:
       existing binary testcase only can you add filter query ? I guess no need to add new testcase for it, it will slow down CI running time.




----------------------------------------------------------------
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 #3920:
URL: https://github.com/apache/carbondata/pull/3920#discussion_r486758917



##########
File path: integration/presto/src/main/prestosql/org/apache/carbondata/presto/PrestoFilterUtil.java
##########
@@ -78,6 +78,8 @@ private static DataType spi2CarbondataTypeMapper(HiveColumnHandle columnHandle)
     HiveType colType = columnHandle.getHiveType();
     if (colType.equals(HiveType.HIVE_BOOLEAN)) {
       return DataTypes.BOOLEAN;
+    } else if (colType.equals(HiveType.HIVE_BINARY)) {

Review comment:
       I can see byte and float data type is also missing. can you add and test for it ?

##########
File path: integration/presto/src/test/scala/org/apache/carbondata/presto/integrationtest/PrestoTestNonTransactionalTableFiles.scala
##########
@@ -230,6 +230,37 @@ class PrestoTestNonTransactionalTableFiles extends FunSuiteLike with BeforeAndAf
     }
   }
 
+  def buildOnlyBinary(rows: Int, sortColumns: Array[String], path : String): Any = {

Review comment:
       existing binary testcase only can you add filter query ? I guess no need to add new testcase for it, it will slow down CI running time.




----------------------------------------------------------------
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 #3920:
URL: https://github.com/apache/carbondata/pull/3920#discussion_r486758917



##########
File path: integration/presto/src/main/prestosql/org/apache/carbondata/presto/PrestoFilterUtil.java
##########
@@ -78,6 +78,8 @@ private static DataType spi2CarbondataTypeMapper(HiveColumnHandle columnHandle)
     HiveType colType = columnHandle.getHiveType();
     if (colType.equals(HiveType.HIVE_BOOLEAN)) {
       return DataTypes.BOOLEAN;
+    } else if (colType.equals(HiveType.HIVE_BINARY)) {

Review comment:
       I can see byte and float data type is also missing. can you add and test for it ?

##########
File path: integration/presto/src/test/scala/org/apache/carbondata/presto/integrationtest/PrestoTestNonTransactionalTableFiles.scala
##########
@@ -230,6 +230,37 @@ class PrestoTestNonTransactionalTableFiles extends FunSuiteLike with BeforeAndAf
     }
   }
 
+  def buildOnlyBinary(rows: Int, sortColumns: Array[String], path : String): Any = {

Review comment:
       existing binary testcase only can you add filter query ? I guess no need to add new testcase for it, it will slow down CI running time.

##########
File path: integration/presto/src/main/prestosql/org/apache/carbondata/presto/PrestoFilterUtil.java
##########
@@ -78,6 +78,8 @@ private static DataType spi2CarbondataTypeMapper(HiveColumnHandle columnHandle)
     HiveType colType = columnHandle.getHiveType();
     if (colType.equals(HiveType.HIVE_BOOLEAN)) {
       return DataTypes.BOOLEAN;
+    } else if (colType.equals(HiveType.HIVE_BINARY)) {

Review comment:
       I can see byte and float data type is also missing. can you add and test for it ?

##########
File path: integration/presto/src/test/scala/org/apache/carbondata/presto/integrationtest/PrestoTestNonTransactionalTableFiles.scala
##########
@@ -230,6 +230,37 @@ class PrestoTestNonTransactionalTableFiles extends FunSuiteLike with BeforeAndAf
     }
   }
 
+  def buildOnlyBinary(rows: Int, sortColumns: Array[String], path : String): Any = {

Review comment:
       existing binary testcase only can you add filter query ? I guess no need to add new testcase for it, it will slow down CI running time.

##########
File path: integration/presto/src/main/prestosql/org/apache/carbondata/presto/PrestoFilterUtil.java
##########
@@ -78,6 +78,8 @@ private static DataType spi2CarbondataTypeMapper(HiveColumnHandle columnHandle)
     HiveType colType = columnHandle.getHiveType();
     if (colType.equals(HiveType.HIVE_BOOLEAN)) {
       return DataTypes.BOOLEAN;
+    } else if (colType.equals(HiveType.HIVE_BINARY)) {

Review comment:
       I can see byte and float data type is also missing. can you add and test for it ?

##########
File path: integration/presto/src/test/scala/org/apache/carbondata/presto/integrationtest/PrestoTestNonTransactionalTableFiles.scala
##########
@@ -230,6 +230,37 @@ class PrestoTestNonTransactionalTableFiles extends FunSuiteLike with BeforeAndAf
     }
   }
 
+  def buildOnlyBinary(rows: Int, sortColumns: Array[String], path : String): Any = {

Review comment:
       existing binary testcase only can you add filter query ? I guess no need to add new testcase for it, it will slow down CI running time.

##########
File path: integration/presto/src/main/prestosql/org/apache/carbondata/presto/PrestoFilterUtil.java
##########
@@ -78,6 +78,8 @@ private static DataType spi2CarbondataTypeMapper(HiveColumnHandle columnHandle)
     HiveType colType = columnHandle.getHiveType();
     if (colType.equals(HiveType.HIVE_BOOLEAN)) {
       return DataTypes.BOOLEAN;
+    } else if (colType.equals(HiveType.HIVE_BINARY)) {

Review comment:
       I can see byte and float data type is also missing. can you add and test for it ?

##########
File path: integration/presto/src/test/scala/org/apache/carbondata/presto/integrationtest/PrestoTestNonTransactionalTableFiles.scala
##########
@@ -230,6 +230,37 @@ class PrestoTestNonTransactionalTableFiles extends FunSuiteLike with BeforeAndAf
     }
   }
 
+  def buildOnlyBinary(rows: Int, sortColumns: Array[String], path : String): Any = {

Review comment:
       existing binary testcase only can you add filter query ? I guess no need to add new testcase for it, it will slow down CI running time.

##########
File path: integration/presto/src/main/prestosql/org/apache/carbondata/presto/PrestoFilterUtil.java
##########
@@ -78,6 +78,8 @@ private static DataType spi2CarbondataTypeMapper(HiveColumnHandle columnHandle)
     HiveType colType = columnHandle.getHiveType();
     if (colType.equals(HiveType.HIVE_BOOLEAN)) {
       return DataTypes.BOOLEAN;
+    } else if (colType.equals(HiveType.HIVE_BINARY)) {

Review comment:
       I can see byte and float data type is also missing. can you add and test for it ?

##########
File path: integration/presto/src/test/scala/org/apache/carbondata/presto/integrationtest/PrestoTestNonTransactionalTableFiles.scala
##########
@@ -230,6 +230,37 @@ class PrestoTestNonTransactionalTableFiles extends FunSuiteLike with BeforeAndAf
     }
   }
 
+  def buildOnlyBinary(rows: Int, sortColumns: Array[String], path : String): Any = {

Review comment:
       existing binary testcase only can you add filter query ? I guess no need to add new testcase for it, it will slow down CI running time.

##########
File path: integration/presto/src/main/prestosql/org/apache/carbondata/presto/PrestoFilterUtil.java
##########
@@ -78,6 +78,8 @@ private static DataType spi2CarbondataTypeMapper(HiveColumnHandle columnHandle)
     HiveType colType = columnHandle.getHiveType();
     if (colType.equals(HiveType.HIVE_BOOLEAN)) {
       return DataTypes.BOOLEAN;
+    } else if (colType.equals(HiveType.HIVE_BINARY)) {

Review comment:
       I can see byte and float data type is also missing. can you add and test for it ?

##########
File path: integration/presto/src/test/scala/org/apache/carbondata/presto/integrationtest/PrestoTestNonTransactionalTableFiles.scala
##########
@@ -230,6 +230,37 @@ class PrestoTestNonTransactionalTableFiles extends FunSuiteLike with BeforeAndAf
     }
   }
 
+  def buildOnlyBinary(rows: Int, sortColumns: Array[String], path : String): Any = {

Review comment:
       existing binary testcase only can you add filter query ? I guess no need to add new testcase for it, it will slow down CI running time.

##########
File path: integration/presto/src/main/prestosql/org/apache/carbondata/presto/PrestoFilterUtil.java
##########
@@ -78,6 +78,8 @@ private static DataType spi2CarbondataTypeMapper(HiveColumnHandle columnHandle)
     HiveType colType = columnHandle.getHiveType();
     if (colType.equals(HiveType.HIVE_BOOLEAN)) {
       return DataTypes.BOOLEAN;
+    } else if (colType.equals(HiveType.HIVE_BINARY)) {

Review comment:
       I can see byte and float data type is also missing. can you add and test for it ?

##########
File path: integration/presto/src/test/scala/org/apache/carbondata/presto/integrationtest/PrestoTestNonTransactionalTableFiles.scala
##########
@@ -230,6 +230,37 @@ class PrestoTestNonTransactionalTableFiles extends FunSuiteLike with BeforeAndAf
     }
   }
 
+  def buildOnlyBinary(rows: Int, sortColumns: Array[String], path : String): Any = {

Review comment:
       existing binary testcase only can you add filter query ? I guess no need to add new testcase for it, it will slow down CI running time.

##########
File path: integration/presto/src/main/prestosql/org/apache/carbondata/presto/PrestoFilterUtil.java
##########
@@ -78,6 +78,8 @@ private static DataType spi2CarbondataTypeMapper(HiveColumnHandle columnHandle)
     HiveType colType = columnHandle.getHiveType();
     if (colType.equals(HiveType.HIVE_BOOLEAN)) {
       return DataTypes.BOOLEAN;
+    } else if (colType.equals(HiveType.HIVE_BINARY)) {

Review comment:
       I can see byte and float data type is also missing. can you add and test for it ?

##########
File path: integration/presto/src/test/scala/org/apache/carbondata/presto/integrationtest/PrestoTestNonTransactionalTableFiles.scala
##########
@@ -230,6 +230,37 @@ class PrestoTestNonTransactionalTableFiles extends FunSuiteLike with BeforeAndAf
     }
   }
 
+  def buildOnlyBinary(rows: Int, sortColumns: Array[String], path : String): Any = {

Review comment:
       existing binary testcase only can you add filter query ? I guess no need to add new testcase for it, it will slow down CI running time.




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

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


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


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

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


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


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

akkio-97 commented on a change in pull request #3920:
URL: https://github.com/apache/carbondata/pull/3920#discussion_r488734274



##########
File path: integration/presto/src/main/prestosql/org/apache/carbondata/presto/PrestoFilterUtil.java
##########
@@ -78,6 +78,8 @@ private static DataType spi2CarbondataTypeMapper(HiveColumnHandle columnHandle)
     HiveType colType = columnHandle.getHiveType();
     if (colType.equals(HiveType.HIVE_BOOLEAN)) {
       return DataTypes.BOOLEAN;
+    } else if (colType.equals(HiveType.HIVE_BINARY)) {

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]

akkio-97 commented on a change in pull request #3920:
URL: https://github.com/apache/carbondata/pull/3920#discussion_r488734849



##########
File path: integration/presto/src/test/scala/org/apache/carbondata/presto/integrationtest/PrestoTestNonTransactionalTableFiles.scala
##########
@@ -230,6 +230,37 @@ class PrestoTestNonTransactionalTableFiles extends FunSuiteLike with BeforeAndAf
     }
   }
 
+  def buildOnlyBinary(rows: Int, sortColumns: Array[String], path : String): Any = {

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]

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


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


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

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


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


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

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


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


----------------------------------------------------------------
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 #3920:
URL: https://github.com/apache/carbondata/pull/3920#discussion_r489158120



##########
File path: integration/presto/src/main/prestosql/org/apache/carbondata/presto/PrestoFilterUtil.java
##########
@@ -78,16 +78,22 @@ private static DataType spi2CarbondataTypeMapper(HiveColumnHandle columnHandle)
     HiveType colType = columnHandle.getHiveType();
     if (colType.equals(HiveType.HIVE_BOOLEAN)) {
       return DataTypes.BOOLEAN;
+    } else if (colType.equals(HiveType.HIVE_BINARY)) {
+      return DataTypes.BINARY;
     } else if (colType.equals(HiveType.HIVE_SHORT)) {
       return DataTypes.SHORT;
     } else if (colType.equals(HiveType.HIVE_INT)) {
       return DataTypes.INT;
+    } else if (colType.equals(HiveType.HIVE_FLOAT)) {

Review comment:
       please handle the same file changes in prestodb also and compile prestodb profile once with latest changes




----------------------------------------------------------------
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 #3920:
URL: https://github.com/apache/carbondata/pull/3920#discussion_r489158514



##########
File path: integration/presto/src/main/prestosql/org/apache/carbondata/presto/PrestoFilterUtil.java
##########
@@ -78,16 +78,22 @@ private static DataType spi2CarbondataTypeMapper(HiveColumnHandle columnHandle)
     HiveType colType = columnHandle.getHiveType();
     if (colType.equals(HiveType.HIVE_BOOLEAN)) {
       return DataTypes.BOOLEAN;
+    } else if (colType.equals(HiveType.HIVE_BINARY)) {
+      return DataTypes.BINARY;
     } else if (colType.equals(HiveType.HIVE_SHORT)) {
       return DataTypes.SHORT;
     } else if (colType.equals(HiveType.HIVE_INT)) {
       return DataTypes.INT;
+    } else if (colType.equals(HiveType.HIVE_FLOAT)) {

Review comment:
       also manually run new testcases once in prestodb profile




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

akkio-97 commented on a change in pull request #3920:
URL: https://github.com/apache/carbondata/pull/3920#discussion_r490034868



##########
File path: integration/presto/src/main/prestosql/org/apache/carbondata/presto/PrestoFilterUtil.java
##########
@@ -78,16 +78,22 @@ private static DataType spi2CarbondataTypeMapper(HiveColumnHandle columnHandle)
     HiveType colType = columnHandle.getHiveType();
     if (colType.equals(HiveType.HIVE_BOOLEAN)) {
       return DataTypes.BOOLEAN;
+    } else if (colType.equals(HiveType.HIVE_BINARY)) {
+      return DataTypes.BINARY;
     } else if (colType.equals(HiveType.HIVE_SHORT)) {
       return DataTypes.SHORT;
     } else if (colType.equals(HiveType.HIVE_INT)) {
       return DataTypes.INT;
+    } else if (colType.equals(HiveType.HIVE_FLOAT)) {

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]

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


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


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