[CALCITE-3697] Implement BITCOUNT scalar function #3927

normanj-bitquill · 2024-08-21T22:00:05Z

All libraries now have a BITCOUNT() function
The MySQL library also has BIT_COUNT() which is an alias for BITCOUNT()
BITCOUNT() counts all of the bits set in an integer or bytestring
Return NULL if the argument is NULL

This is based on a previous PR that appears dormant.
#1746

core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java

core/src/test/resources/sql/functions.iq

NobiGo · 2024-08-22T11:17:00Z

site/_docs/reference.md

@@ -2752,6 +2752,8 @@ In the following:
 | * | ATANH(numeric)                                 | Returns the inverse hyperbolic tangent of *numeric*
 | f | BITAND_AGG(value)                              | Equivalent to `BIT_AND(value)`
 | f | BITOR_AGG(value)                               | Equivalent to `BIT_OR(value)`
+| * | BITCOUNT(integer)                             | Returns the bitwise COUNT of non-null *integer*


Then the test case maybe should include a negative number and zero.

In MYSQL:

select bit_count(3.5); return 1 select bit_count(3); return 2 select bit_count(5.5); return 2

So support float number?

In calcite these are decimal numbers

Since the decimal representation of a number is not specified, how can these functions be implemented?

I have updated the MySQL implementation to support decimal values. It will only count bits set in the integer portion of a decimal value. This matches MySQL behaviour.

I think you have to say this in the documentation. It's a very bizarre choice, frankly.

The documentation has been updated. If it makes more sense, both BITCOUNT() and BIT_COUNT() can use the MySQL behaviour and become identical.

@normanj-bitquill Are you sure Spark also has a bigcount function? I didn't find the corresponding record in the documentation
https://spark.apache.org/docs/3.4.0/sql-ref-functions.html#content

There is a BIT_COUNT function in Spark. I'll enable it for the Spark library.

There was discussion in this ticket
https://issues.apache.org/jira/browse/CALCITE-3732

that all libraries would get the BITCOUNT function.

BIT_COUNT is now enabled for Apache Spark.

julianhyde · 2024-08-22T19:35:05Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

@@ -15523,6 +15523,58 @@ void checkBitAnd(SqlOperatorFixture f0, FunctionAlias functionAlias) {
    f0.forEachLibrary(list(functionAlias.libraries), consumer);
  }

+  @Test public void testBitCountFunc() {


add javadoc. explain that one tests BITCOUNT and the other tests BIT_COUNT.

are these methods identical? do you intend them to stay identical? If so, share code, don't copy-paste. We have done that for other operators.

They are not identical. BIT_COUNT() supports decimal values. I viewed this as a unique feature in MySQL. If it makes more sense, I can use the MySQL behaviour for both BITCOUNT() and BIT_COUNT().

julianhyde · 2024-08-22T19:36:15Z

site/_docs/reference.md

@@ -2752,6 +2752,8 @@ In the following:
 | * | ATANH(numeric)                                 | Returns the inverse hyperbolic tangent of *numeric*
 | f | BITAND_AGG(value)                              | Equivalent to `BIT_AND(value)`
 | f | BITOR_AGG(value)                               | Equivalent to `BIT_OR(value)`
+| * | BITCOUNT(integer)                              | Returns the bitwise COUNT of non-null *integer*
+| m | BIT_COUNT(numeric)                             | Returns the bitwise COUNT of the integer portion of non-null *numeric*


do these functions apply to binary, varbinary , decimal, float? if so, say so

Updated the documentation to mention that it also supports binary and numeric (in the case of BIT_COUNT()).

julianhyde · 2024-08-22T19:38:21Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+          false);
+      f.checkScalar("bit_count(8)", "1", "BIGINT NOT NULL");
+      f.checkScalar("bit_count(CAST(x'ad' AS BINARY(1)))", "5", "BIGINT NOT NULL");
+      f.checkScalar("bit_count(5.23)", "2", "BIGINT NOT NULL");


so it ignores bits in the fractional part? that's rather subtle. worth calling out in the test, and also in the bitCount(BigDecimal) function

Yeah, it ignores the fractional part. I have updated the documentation as well as the Javadoc for bitCount(BigDecimal).

julianhyde · 2024-08-22T19:39:40Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+    f.checkNull("bitcount(cast(NULL as BINARY))");
+  }
+
+  @Test public void testBitCountMySQLFunc() {


what are the results when applied to negative numbers? need tests for that.

do you assume two's complement? do you assume that a 32 bit integer becomes a 32 bit negative integer? or does it become a 64 bit negative integer?

The input number is converted to a 64-bit integer using two's complement. It will count the bits set in this, so bitcount(-1) will return 64.

Added a test.

mihaibudiu · 2024-08-23T05:04:32Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

+  /** Helper function for implementing <code>BITCOUNT</code>. Counts the number
+   * of bits set in the integer portion of a decimal value. */
+  public static long bitCount(BigDecimal b) {
+    return Long.bitCount(b.longValue());


This also rounds down the decimal number, which is not obvious.
Please also note that a decimal may not fit into a long since we merged #3589
So I am not sure this implementation is correct

Personally I think this behavior is very unexpected, and I think decimals should not be supported at all.
Moreover, this is not the number of 1 bits in the actual internal representation, but in the conversion of the decimal to a long.

I did some more testing with MySQL.

bitcount(18446744073709551615) = 64

bitcount(18446744073709551616 or greater) = 63

bitcount(-9223372036854775808 or less) = 1

I have updating the implementation of bitcount(BigDecimal) to match this behaviour. This is only enabled in the MySQL library.

It should match MySQL behaviour at the moment, but since this is not really documented
https://dev.mysql.com/doc/refman/8.4/en/bit-functions.html#function_bit-count
perhaps this is not worth keeping and could change at any time.

In general is it better to match MySQL behaviour (for a MySQL specific function) or is it better to take the simpler approach?

mihaibudiu · 2024-08-24T00:29:46Z

testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java

+      f.checkScalar(functionName + "(8)", "1", "BIGINT NOT NULL");
+      f.checkScalar(functionName + "(CAST(x'ad' AS BINARY(1)))", "5", "BIGINT NOT NULL");
+      f.checkScalar(functionName + "(-1)", "64", "BIGINT NOT NULL");
+      f.checkNull(functionName + "(cast(NULL as TINYINT))");


how about a test with a null without a cast?
this trips some function implementations

Added a test for NULL without a cast.

mihaibudiu · 2024-08-26T18:22:47Z

From my pov this can be merged, but let's see if the other reviewers have other comments.

NobiGo · 2024-08-27T08:15:49Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

+    return Long.bitCount(b);
+  }
+
+  private static final BigDecimal BITCOUNT_MAX = new BigDecimal("18446744073709551615");


Why this number？

This is 2^64 - 1. I found it through testing in MySQL. I added a comment to say that it is 2^64 - 1.

Maybe we can use

Math.pow(2,64) -1

generate it?

BigDecimal has s shiftleft method

BigInteger has a shiftleft, BigDecimal does not. I have updated the constants to use BigDecimal power, subtract and negate.

NobiGo · 2024-08-27T08:19:00Z

site/_docs/reference.md

@@ -2752,6 +2752,11 @@ In the following:
 | * | ATANH(numeric)                                 | Returns the inverse hyperbolic tangent of *numeric*
 | f | BITAND_AGG(value)                              | Equivalent to `BIT_AND(value)`
 | f | BITOR_AGG(value)                               | Equivalent to `BIT_OR(value)`
+| * | BITCOUNT(integer)                              | Returns the bitwise COUNT of *integer* or NULL if *integer* is NULL
+| * | BITCOUNT(binary)                               | Returns the bitwise COUNT of *binary* of NULL if *binary* is NULL


Merge these two functions into one?

normanj-bitquill · 2024-08-27T18:57:25Z

Squashed the commits.

caicancai

LGTM

* All libraries now have a BITCOUNT() function that supports integer and binary values * The MySQL and BigQuery libraries also hav BIT_COUNT() which is an alias for BITCOUNT() * The MySQL version of BIT_COUNT() also supports decimal values (only looks at the integer portion) * BITCOUNT() counts all of the bits set in an integer or bytestring * Return NULL if the argument is NULL

sonarqubecloud · 2024-08-28T21:36:19Z

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
92.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

normanj-bitquill · 2024-09-05T16:57:32Z

@mihaibudiu Is there anything further needed on this PR?

NobiGo requested changes Aug 22, 2024

View reviewed changes

julianhyde reviewed Aug 22, 2024

View reviewed changes

mihaibudiu reviewed Aug 23, 2024

View reviewed changes

mihaibudiu approved these changes Aug 24, 2024

View reviewed changes

NobiGo reviewed Aug 27, 2024

View reviewed changes

NobiGo approved these changes Aug 27, 2024

View reviewed changes

mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Aug 27, 2024

normanj-bitquill force-pushed the calcite-3697 branch from 6930fa9 to c9a6de7 Compare August 27, 2024 18:56

caicancai approved these changes Aug 28, 2024

View reviewed changes

normanj-bitquill force-pushed the calcite-3697 branch from 85dd533 to d3b5a06 Compare August 28, 2024 21:02

NobiGo approved these changes Aug 29, 2024

View reviewed changes

mihaibudiu merged commit 67405c3 into apache:main Sep 9, 2024
19 checks passed

[CALCITE-3697] Implement BITCOUNT scalar function #3927

[CALCITE-3697] Implement BITCOUNT scalar function #3927

Conversation

normanj-bitquill commented Aug 21, 2024

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

caicancai Aug 26, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaibudiu commented Aug 26, 2024

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

normanj-bitquill commented Aug 27, 2024

caicancai left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 28, 2024

Quality Gate passed

normanj-bitquill commented Sep 5, 2024

caicancai Aug 26, 2024 •

edited

Loading