Re: [PR] [SPARK-58186][SQL] Add bitmap_contains function [spark]

["cloud-fan (via GitHub)" <gi...@apache.org>]

cloud-fan commented on code in PR #58117:
URL: https://github.com/apache/spark/pull/58117#discussion_r3995048897


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/bitmapExpressions.scala:
##########
@@ -375,6 +383,83 @@ 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(LongType),
+          "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
+    StaticInvoke(
+      classOf[BitmapExpressionUtils],
+      BooleanType,
+      "bitmapContains",
+      Seq(bitmap, Cast(right, LongType)),

Review Comment:
   **Non-blocking (P2):** Because `StaticInvoke` short-circuits its generated arguments but interpreted `InvokeLike` evaluates all arguments before checking nulls, this argument order makes behavior depend on `spark.sql.codegen.factoryMode`. With ANSI mode enabled and a dynamic row whose bitmap is NULL and whose position overflows long, generated evaluation skips the cast and returns NULL, while `NO_CODEGEN` evaluates the cast and throws `CAST_OVERFLOW`. The new whole-stage-codegen toggle does not force the interpreted projection and therefore misses this case. Please align the two `InvokeLike` paths (or otherwise make this replacement mode-consistent) and add a `CODEGEN_ONLY`/`NO_CODEGEN` regression for the null-plus-overflow input.
   
   **Recommended change:** Make interpreted InvokeLike argument preparation stop after an argument evaluates to null when needNullCheckForIndex marks that argument as result-determining, mirroring prepareArguments. Add a focused InvokeLike/StaticInvoke regression and update BitmapExpressionsQuerySuite to select CODEGEN_ONLY and NO_CODEGEN for a dynamic null bitmap paired with an ANSI-overflowing numeric position.
   
   **Why this works:** Guard the interpreted evaluation loop with the accumulated result-null state (or break immediately when the current null is result-determining) so later expressions are not evaluated when the invocation will return null. Keep storing and passing every argument on the non-null path. Exercise both projection factory modes so generated and interpreted evaluation return the same NULL and ensure the non-null bitmap still propagates CAST_OVERFLOW.
   
   **Scope:** sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects, sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions, sql/core/src/test/scala/org/apache/spark/sql
   
   **Compatibility:** When no earlier argument determines a null result, arguments remain left-to-right, all required casts execute, the target method receives the same values, and existing method or cast failures propagate unchanged.
   
   **Risks:** Interpreted execution will no longer observe side effects or exceptions from later arguments after a result-determining null; this is an intentional parity change but may expose callers that incorrectly depended on the previous interpreted-only eagerness. A loop change must preserve evaluation order and evaluatedArgs population whenever no result-determining null is encountered.
   
   **Constraints:** Do not change evaluation of later arguments when propagateNull is false and their nullability does not require a primitive null guard. Preserve reflection exception unwrapping, return boxing, foldability, and StaticInvoke nullability declarations. Keep the bitmap function's existing numeric-to-long coercion and out-of-range false semantics unchanged.
   
   **Success:** A null result-determining argument prevents every later InvokeLike argument from being evaluated in both generated and interpreted modes. bitmap_contains returns NULL under both CODEGEN_ONLY and NO_CODEGEN for a dynamic null bitmap paired with an ANSI-overflowing numeric position. bitmap_contains still raises CAST_OVERFLOW under ANSI mode when the bitmap is non-null and the numeric position overflows long. Non-null StaticInvoke, Invoke, and NewInstance argument evaluation and invocation behavior remain unchanged. Focused Catalyst object-expression and SQL bitmap-expression suites pass.



-- 
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: reviews-unsubscribe@spark.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org