You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2021/06/24 09:45:48 UTC

[GitHub] [drill] dzamo opened a new pull request #2264: Drill 7955: Extend XOR bitwise functions to match AND and OR

dzamo opened a new pull request #2264:
URL: https://github.com/apache/drill/pull/2264


   # [DRILL-7955](https://issues.apache.org/jira/browse/DRILL-7955): Extend XOR bitwise functions to match AND and OR
   
   ## Description
   The bitwise XOR function (^) is only implemented for Int while AND (&) and OR (|) are implemented for all widths of integer.  Also there is no BIT_XOR aggregate while there are both BIT_AND and BIT_OR and all three operations are associative.  This improvement proposes to bring XOR in line with AND and OR in both of these respects.  E.g. without this patch we have
   ```
   select `|`(cast(2 as bigint), cast(4 as bigint)) -- bitwise OR, works
   EXPR$0|
   ------|
        6|
   
   select `^`(cast(2 as bigint), cast(4 as bigint)) -- bitwise XOR, breaks
   
   SQL Error: FUNCTION ERROR: ^ does not support operand types (BIGINT,BIGINT)
   ```
   
   ## Documentation
   Descriptions of &, |, ^ and BIT_XOR will be added to the user docs.
   
   ## Testing
   
   Tests for LSHIFT, RSHIFT (two related and testless functions), XOR, BIT_XOR added to
   
   exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewMathFunctions.java
   exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestNewAggregateFunctions.java


-- 
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:
users@infra.apache.org

[GitHub] [drill] dzamo merged pull request #2264: Drill 7955: Extend XOR bitwise functions to match AND and OR

Posted by GitBox <gi...@apache.org>.
dzamo merged pull request #2264:
URL: https://github.com/apache/drill/pull/2264


   


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

To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [drill] dzamo commented on a change in pull request #2264: Drill 7955: Extend XOR bitwise functions to match AND and OR

Posted by GitBox <gi...@apache.org>.
dzamo commented on a change in pull request #2264:
URL: https://github.com/apache/drill/pull/2264#discussion_r659149206



##########
File path: exec/java-exec/src/test/resources/functions/testBitTwiddlers.json
##########
@@ -0,0 +1,34 @@
+{

Review comment:
       Thanks for the review @paul-rogers, I'll certain allow more time for reviews to arrive, in future.  The naming of these functions put me into a bit of a spin, to the point where I couldn't formulate very good questions about naming when I asked on the Slack channel.
   
   1. I started out trying for names of `AND`, `OR` and `XOR` but the first two seem to be taken elsewhere Drill and would not work, while the last is fine and indeed was already synonymous with `^` before this patch (but only working for 32-bit arguments).
   2. Investigating `AND` and `OR`, I found `booleanAnd` and `booleanOr` in FunctionNames.java and then also logic in other code using these names to decide if something is a boolean expression, which bitwise logical operators are not.  I decided at that point that I should not overload function names that were intended to be boolean, and abandoned `AND` and `OR`.
   3. I considered `BIT_AND`, `BIT_OR` and `BIT_XOR` but already these belong to the aggregate bitwise functions and do not allow overloading for the scalar case (I got runtime errors when I tried).
   4. I considered `INT_AND`, `INT_OR` and `INT_XOR` and indeed these did work in testing.
   5. Somehow I uncovered that `&`, `|`, `^` are _already_ working scalar bitwise functions in Drill, in spite of my being unable to find any implementation of them in the code.  Looking at physical query plans did not give me any clues and I told myself that some "magic" is happening upstream, possibly code gen by Calcite?
   6. Since Drill already has the names in (5) for bitwise functions, even if they're made of special characters and nasty, I stuck with them because it's consistent with what's already there.
   
   I'd be grateful for any suggestion of where to take this in a follow-up commit...




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

To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [drill] luocooong commented on pull request #2264: Drill 7955: Extend XOR bitwise functions to match AND and OR

Posted by GitBox <gi...@apache.org>.
luocooong commented on pull request #2264:
URL: https://github.com/apache/drill/pull/2264#issuecomment-867712456


   @dzamo Nice work. Could you please update the docs for this extension? [Math and Trig](https://github.com/apache/drill/blob/gh-pages/_docs/en/sql-reference/sql-functions/010-math-and-trig.md)


-- 
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:
users@infra.apache.org

[GitHub] [drill] dzamo commented on pull request #2264: Drill 7955: Extend XOR bitwise functions to match AND and OR

Posted by GitBox <gi...@apache.org>.
dzamo commented on pull request #2264:
URL: https://github.com/apache/drill/pull/2264#issuecomment-867731330


   > @dzamo Nice work. Could you please update the docs for this extension? [Math and Trig](https://github.com/apache/drill/blob/gh-pages/_docs/en/sql-reference/sql-functions/010-math-and-trig.md)
   
   Already done, Sir. Will deploy in the next batch of doc updates. 


-- 
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:
users@infra.apache.org

[GitHub] [drill] dzamo commented on a change in pull request #2264: Drill 7955: Extend XOR bitwise functions to match AND and OR

Posted by GitBox <gi...@apache.org>.
dzamo commented on a change in pull request #2264:
URL: https://github.com/apache/drill/pull/2264#discussion_r658003252



##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/BitFunctions.java
##########
@@ -71,22 +70,4 @@ public void eval() {
     }
   }
 

Review comment:
       The addition of xor to MathFunc.tdd made it a redundant special case: 32-bit ints are already provided for by the new FreeMarker version.  We get a small bit of clean-up for free here because the deleted IntXor function was out of place in BitFunctions.java, which otherwise only contains functions that operate on the boolean type BIT. 




-- 
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:
users@infra.apache.org

[GitHub] [drill] paul-rogers commented on a change in pull request #2264: Drill 7955: Extend XOR bitwise functions to match AND and OR

Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #2264:
URL: https://github.com/apache/drill/pull/2264#discussion_r659076575



##########
File path: exec/java-exec/src/test/resources/functions/testBitTwiddlers.json
##########
@@ -0,0 +1,34 @@
+{

Review comment:
       Suggestion: this form of test was used very early on in Drill before the full SQL stack worked. Consider writing a SQL-based test.
   
   Since we are trying to support all types, I wonder if we should have a test which generates the needed SQL. Something like:
   
   ```sql
   SELECT CAST(A AS SMALLINT) XOR CAST(B AS SMALLINT)
   FROM (VALUES ...)
   ```
   
   Then generate the SQL for each supported data type and verify the results.
   
   The `CAST` is suggested because there is no good way in Drill to create a data file that has data of every types (since Drill has no way to specify a schema with data types.) I suppose you could create a Parquet file with the required types, but that seems overkill.




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

To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [drill] cgivre commented on a change in pull request #2264: Drill 7955: Extend XOR bitwise functions to match AND and OR

Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2264:
URL: https://github.com/apache/drill/pull/2264#discussion_r657984415



##########
File path: exec/java-exec/src/main/codegen/data/MathFunc.tdd
##########
@@ -138,7 +138,7 @@ unaryMathFunctions : [
       {input: "UInt8", outputType: "UInt8", castType: "long"}
      ] 
     },
-    {className: "LeftShift", funcName: "lshift", javaFunc: " << ", types: [
+    {className: "LeftShift", funcName: "lshift", aliasName: "", javaFunc: " << ", types: [

Review comment:
       Would we want this for all `INT` types like `SHORT`, `TINYINT` etc?

##########
File path: exec/java-exec/src/main/codegen/data/MathFunc.tdd
##########
@@ -125,7 +125,7 @@ unaryMathFunctions : [
       {input: "UInt8", outputType: "UInt8", castType: "long"}     
      ] 
     },
-    {className: "Mod", funcName: "mod", javaFunc : " % ", types: [
+    {className: "Mod", funcName: "mod", aliasName: "", javaFunc : " % ", types: [

Review comment:
       Same comment as elsewhere... would we want this for other `INT` like data types or does Drill handle that elsewhere?

##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/BitFunctions.java
##########
@@ -71,22 +70,4 @@ public void eval() {
     }
   }
 

Review comment:
       Out of curiosity, why is this being deleted?




-- 
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:
users@infra.apache.org

[GitHub] [drill] dzamo commented on a change in pull request #2264: Drill 7955: Extend XOR bitwise functions to match AND and OR

Posted by GitBox <gi...@apache.org>.
dzamo commented on a change in pull request #2264:
URL: https://github.com/apache/drill/pull/2264#discussion_r657997077



##########
File path: exec/java-exec/src/main/codegen/data/MathFunc.tdd
##########
@@ -125,7 +125,7 @@ unaryMathFunctions : [
       {input: "UInt8", outputType: "UInt8", castType: "long"}     
      ] 
     },
-    {className: "Mod", funcName: "mod", javaFunc : " % ", types: [
+    {className: "Mod", funcName: "mod", aliasName: "", javaFunc : " % ", types: [

Review comment:
       I believe it does handle all widths already.  If you look a full listing of MathFunc.tdd, rather than what this diff reveals, you'll see integer types from 8 bits to 64 bits.




-- 
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:
users@infra.apache.org

[GitHub] [drill] dzamo commented on a change in pull request #2264: Drill 7955: Extend XOR bitwise functions to match AND and OR

Posted by GitBox <gi...@apache.org>.
dzamo commented on a change in pull request #2264:
URL: https://github.com/apache/drill/pull/2264#discussion_r657997666



##########
File path: exec/java-exec/src/main/codegen/data/MathFunc.tdd
##########
@@ -138,7 +138,7 @@ unaryMathFunctions : [
       {input: "UInt8", outputType: "UInt8", castType: "long"}
      ] 
     },
-    {className: "LeftShift", funcName: "lshift", javaFunc: " << ", types: [
+    {className: "LeftShift", funcName: "lshift", aliasName: "", javaFunc: " << ", types: [

Review comment:
       See my comment for `div`




-- 
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:
users@infra.apache.org

[GitHub] [drill] dzamo commented on pull request #2264: Drill 7955: Extend XOR bitwise functions to match AND and OR

Posted by GitBox <gi...@apache.org>.
dzamo commented on pull request #2264:
URL: https://github.com/apache/drill/pull/2264#issuecomment-867536800


   I was unsure about how to select a reviewer, apologies if I've spammed you.


-- 
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:
users@infra.apache.org