Drill 7955: Extend XOR bitwise functions to match AND and OR #2264

jnturton · 2021-06-24T09:45:34Z

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. An example showing the state of the scalar XOR function without this patch follows.

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

jnturton · 2021-06-24T10:49:00Z

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

cgivre

A few minor comments and questions but in general looks good.

cgivre · 2021-06-24T14:10:45Z

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: [


Would we want this for all INT types like SHORT, TINYINT etc?

See my comment for div

cgivre · 2021-06-24T14:11:14Z

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: [


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

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.

cgivre · 2021-06-24T14:12:27Z

exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/BitFunctions.java

@@ -71,22 +70,4 @@ public void eval() {
    }
  }



Out of curiosity, why is this being deleted?

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.

luocooong · 2021-06-24T15:07:23Z

@dzamo Nice work. Could you please update the docs for this extension? Math and Trig

jnturton · 2021-06-24T15:27:42Z

@dzamo Nice work. Could you please update the docs for this extension? Math and Trig

Already done, Sir. Will deploy in the next batch of doc updates.

cgivre

LGTM +1
Thanks for the contribution!

paul-rogers

Looks like this got merged before I could do a requested review. Here is the review anyway.

I would recommend coming up with function names that can be typed as regular functions: XOR(A, B) in addition to the hard-to-type \|`(A, B)`. Check what other DBs do.

Then, I would recommend SQL-based tests that exercise all types. See detailed comment.

paul-rogers · 2021-06-25T23:05:30Z

exec/java-exec/src/test/resources/functions/testBitTwiddlers.json

@@ -0,0 +1,34 @@
+{


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:

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.

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.

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

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.

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

I considered INT_AND, INT_OR and INT_XOR and indeed these did work in testing.

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?

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

…2264) * DRILL-7955: Add BIT_XOR aggregate function for bitwise XOR. * DRILL-7955: Support all integer types for the scalar XOR (^) function. * DRILL-7955: Add tests for BIT_XOR aggregate function. * DRILL-7955: Replace tabs with spaces.

jnturton added 3 commits June 24, 2021 10:58

DRILL-7955: Add BIT_XOR aggregate function for bitwise XOR.

e8edd4c

DRILL-7955: Support all integer types for the scalar XOR (^) function.

188574f

DRILL-7955: Add tests for BIT_XOR aggregate function.

6174d3c

jnturton requested review from cgivre, luocooong and paul-rogers June 24, 2021 09:45

DRILL-7955: Replace tabs with spaces.

948af32

cgivre reviewed Jun 24, 2021

View reviewed changes

cgivre approved these changes Jun 25, 2021

View reviewed changes

jnturton merged commit d088f47 into apache:master Jun 25, 2021

paul-rogers reviewed Jun 25, 2021

View reviewed changes

jnturton deleted the DRILL-7955 branch February 4, 2022 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drill 7955: Extend XOR bitwise functions to match AND and OR #2264

Drill 7955: Extend XOR bitwise functions to match AND and OR #2264

jnturton commented Jun 24, 2021 •

edited

Loading

jnturton commented Jun 24, 2021

cgivre left a comment

cgivre Jun 24, 2021

jnturton Jun 24, 2021

cgivre Jun 24, 2021

jnturton Jun 24, 2021

cgivre Jun 24, 2021

jnturton Jun 24, 2021

luocooong commented Jun 24, 2021

jnturton commented Jun 24, 2021

cgivre left a comment

paul-rogers left a comment

paul-rogers Jun 25, 2021

jnturton Jun 26, 2021

Drill 7955: Extend XOR bitwise functions to match AND and OR #2264

Drill 7955: Extend XOR bitwise functions to match AND and OR #2264

Conversation

jnturton commented Jun 24, 2021 • edited Loading

DRILL-7955: Extend XOR bitwise functions to match AND and OR

Description

Documentation

Testing

jnturton commented Jun 24, 2021

cgivre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luocooong commented Jun 24, 2021

jnturton commented Jun 24, 2021

cgivre left a comment

Choose a reason for hiding this comment

paul-rogers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnturton commented Jun 24, 2021 •

edited

Loading