spark-test commented on code in PR #58117:
URL: https://github.com/apache/spark/pull/58117#discussion_r4480492787
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/BitmapExpressionUtilsSuite.scala:
##########
@@ -93,6 +93,45 @@ class BitmapExpressionUtilsSuite extends SparkFunSuite {
assert(BitmapExpressionUtils.bitmapCount(bitmap) == 15L)
}
+ test("bitmap_contains with set and unset bits") {
+ val bitmap = createBitmap()
+ setBitmapBits(bitmap, 0, 0x81)
+ setBitmapBits(bitmap, 1, 0x01)
+ val original = bitmap.clone()
+
+ assert(BitmapExpressionUtils.bitmapContains(bitmap, 0))
+ assert(!BitmapExpressionUtils.bitmapContains(bitmap, 1))
+ assert(BitmapExpressionUtils.bitmapContains(bitmap, 7))
+ assert(BitmapExpressionUtils.bitmapContains(bitmap, 8))
+ assert(!BitmapExpressionUtils.bitmapContains(bitmap, 9))
+ assert(bitmap.sameElements(original))
+ }
+
+ test("bitmap_contains with empty and short bitmaps") {
+ val emptyBitmap = Array.empty[Byte]
+ assert(!BitmapExpressionUtils.bitmapContains(emptyBitmap, 0))
+
+ val shortBitmap = Array[Byte](0x01, 0x80.toByte)
+ assert(BitmapExpressionUtils.bitmapContains(shortBitmap, 0))
+ assert(BitmapExpressionUtils.bitmapContains(shortBitmap, 15))
+ assert(!BitmapExpressionUtils.bitmapContains(shortBitmap, 16))
+ assert(!BitmapExpressionUtils.bitmapContains(shortBitmap,
BitmapExpressionUtils.NUM_BITS - 1))
+ }
+
+ test("bitmap_contains at fixed-range and long boundaries") {
+ val bitmap = createBitmap()
+ bitmap(bitmap.length - 1) = 0x80.toByte
+ assert(BitmapExpressionUtils.bitmapContains(bitmap,
BitmapExpressionUtils.NUM_BITS - 1))
+ assert(!BitmapExpressionUtils.bitmapContains(bitmap,
BitmapExpressionUtils.NUM_BITS))
Review Comment:
**Nit (P3):** This bitmap is exactly 4096 bytes, so at `NUM_BITS` the
byte-length check would return false even without the `bitPosition >= NUM_BITS`
guard. The same holds for every boundary assertion here and for the SQL checks
with `X'01'`. If the guard were replaced with a length check such as
`bitPosition / 8 >= bitmap.length`, every test would still pass, yet a
4097-byte input with bit 32768 set would return true, against the documented
limit. Could you add a case with a bitmap longer than 4096 bytes and bit 32768
set? It should expect false at 32768 and true at a set bit below it.
**Verification:**
- **Regression:** For a bitmap longer than 4096 bytes with bit 32768 set,
bitmap_contains returns false at position 32768 and true at a set position
below 32768.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/bitmapExpressions.scala:
##########
@@ -375,6 +383,92 @@ case class BitmapXor(left: Expression, right: Expression)
extends BitmapBinaryEx
copy(left = newLeft, right = newRight)
}
+@ExpressionDescription(
+ usage = """
+ _FUNC_(bitmap, bit_position) - Returns true if the bit at the given
bucket-local position is
+ set in the bitmap, false otherwise.
+ """,
+ arguments = """
+ Arguments:
+ * bitmap - The bitmap to test.
+ An expression that evaluates to a binary. A NULL bitmap produces a
NULL result.
+ * bit_position - The bucket-local bit position to test.
+ An expression that evaluates to a numeric value and is cast to a long.
A NULL position
+ produces a NULL result. A negative position, a position at or above
32768, or a position
+ beyond the actual bitmap length returns false. To query an original
value, use
+ bitmap_bit_position(value) and match bitmap_bucket_number(value) to
the bitmap bucket.
+ """,
+ examples = """
+ Examples:
+ > SELECT _FUNC_(X '01', 0L);
+ true
+ > SELECT _FUNC_(X '01', 1L);
+ false
+ > SELECT _FUNC_(X '10', 4L);
+ true
+ """,
+ since = "4.4.0",
+ group = "misc_funcs")
+case class BitmapContains(left: Expression, right: Expression)
+ extends BinaryExpression with RuntimeReplaceable {
+
+ override def checkInputDataTypes(): TypeCheckResult = {
+ if (left.dataType != BinaryType && left.dataType != NullType) {
+ DataTypeMismatch(
+ errorSubClass = "UNEXPECTED_INPUT_TYPE",
+ messageParameters = Map(
+ "paramIndex" -> ordinalNumber(0),
+ "requiredType" -> toSQLType(BinaryType),
+ "inputSql" -> toSQLExpr(left),
+ "inputType" -> toSQLType(left.dataType)
+ )
+ )
+ } else if (!right.dataType.isInstanceOf[NumericType] && right.dataType !=
NullType) {
+ DataTypeMismatch(
+ errorSubClass = "UNEXPECTED_INPUT_TYPE",
+ messageParameters = Map(
+ "paramIndex" -> ordinalNumber(1),
+ "requiredType" -> toSQLType(NumericType),
+ "inputSql" -> toSQLExpr(right),
+ "inputType" -> toSQLType(right.dataType)
+ )
+ )
+ } else {
+ TypeCheckSuccess
+ }
+ }
+
+ override def dataType: DataType = BooleanType
+
+ override def prettyName: String = "bitmap_contains"
+
+ override lazy val replacement: Expression = {
+ // StaticInvoke needs an exact BinaryType child, including for a bare NULL
literal.
+ val bitmap = if (left.dataType == NullType) Cast(left, BinaryType) else
left
+ def invoke(bitmap: Expression): Expression = StaticInvoke(
+ classOf[BitmapExpressionUtils],
+ BooleanType,
+ "bitmapContains",
+ Seq(bitmap, Cast(right, LongType)),
+ Seq(BinaryType, LongType),
+ returnNullable = false)
+
+ if (bitmap.nullable) {
Review Comment:
**Nit (P3):** `bitmap.nullable` is also true for constant bitmaps such as
`unhex('01')`, `to_binary('01', 'hex')` and `NULL`, so they take the `With`
branch. `With` is never foldable, and the constant-only paths don't run
`RewriteWithExpression`. Inline tables check `prepareForEval(e).foldable`, and
DEFAULT analysis runs only FinishAnalysis and ConstantFolding before its
foldability check. So `SELECT * FROM VALUES (bitmap_contains(unhex('01'), 0))`,
the same expression in `INSERT ... VALUES`, and `c BOOLEAN DEFAULT
bitmap_contains(unhex('01'), 0)` fail analysis with
INVALID_INLINE_TABLE.CANNOT_EVALUATE_EXPRESSION_IN_INLINE_TABLE or
INVALID_DEFAULT_VALUE.NOT_CONSTANT. `bitmap_count(unhex('01'))` and
`bitmap_contains(X'01', 0)` are accepted there. Duplicating a constant costs
nothing, so a foldable bitmap gains nothing from `With`. Building the inline
`If(IsNull(bitmap), NULL, call(bitmap))` form when the bitmap is foldable would
keep these calls constant.
See **Shared repair plan 1** in the review body.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/bitmapExpressions.scala:
##########
@@ -375,6 +383,92 @@ case class BitmapXor(left: Expression, right: Expression)
extends BitmapBinaryEx
copy(left = newLeft, right = newRight)
}
+@ExpressionDescription(
+ usage = """
+ _FUNC_(bitmap, bit_position) - Returns true if the bit at the given
bucket-local position is
+ set in the bitmap, false otherwise.
+ """,
+ arguments = """
+ Arguments:
+ * bitmap - The bitmap to test.
+ An expression that evaluates to a binary. A NULL bitmap produces a
NULL result.
+ * bit_position - The bucket-local bit position to test.
+ An expression that evaluates to a numeric value and is cast to a long.
A NULL position
+ produces a NULL result. A negative position, a position at or above
32768, or a position
Review Comment:
**Non-blocking (P2):** These bounds are checked after `bit_position` is cast
to BIGINT (`Cast(right, LongType)` in the replacement), so they don't hold for
every NUMERIC input the function accepts. In the default ANSI mode, a FLOAT,
DOUBLE or DECIMAL position that is NaN, infinite or outside the BIGINT range
raises CAST_OVERFLOW instead of returning false, as the new tests assert for
NaN, the infinities and 18446744073709551617. With ANSI off, NaN becomes
position 0 and a DECIMAL outside the BIGINT range wraps to its low 64 bits, so
`bitmap_contains(X'01', 18446744073709551616)` returns true. In both modes
`-0.5` truncates to 0 and reads bit 0. Integral positions behave as documented.
The Scaladoc of `functions.bitmap_contains` and the PySpark docstring carry the
same sentence. Could all three say that the bounds apply to the position after
the cast, and how the cast behaves with ANSI on and off? The alternative is to
range-check the original value before narrowing. Either way thi
s becomes the 4.4.0 contract. The non-ANSI test with 18446744073709551617
can't tell the two behaviors apart: it passes only because 2^64 + 1 wraps to
bit 1, which `X'01'` leaves unset.
This extends an [existing
discussion](https://github.com/apache/spark/pull/58117#pullrequestreview-5277195576).
**Verification:**
- **Inspection:** The SQL, Scala and Python descriptions of bit_position
state the same cast-first contract, including CAST_OVERFLOW under ANSI mode and
the legacy-mode conversions.
- **Behavior:** With ANSI mode disabled, two cases return the documented
cast-first result through both SQL and DataFrame calls: a DECIMAL position
outside the BIGINT range whose legacy cast lands on a set bit, such as
18446744073709551616 against X'01', and a negative fraction such as -0.5
against X'01'.
- **Compatibility:** The existing ANSI CAST_OVERFLOW assertions for
overflowing, NaN and infinite positions, and the NULL-bitmap guard queries,
still pass unchanged.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
