[SPARK-57436][SQL] Support start and occurrence parameters in instr function by loftiest · Pull Request #56498 · apache/spark

loftiest · 2026-06-14T13:10:04Z

What changes were proposed in this pull request?

This PR extends the instr function to accept optional start and occurrence parameters, supporting forward and backward search with full collation awareness. The changes include:

A new expression StringInstr4 that handles 3‑ and 4‑argument invocations, while the existing two‑argument instr continues to use StringInstr.
A new StringInstrExpressionBuilder that routes calls based on the number of arguments: 2 arguments → StringInstr, 3 or 4 arguments → StringInstr4.
Extended UTF8String.indexOf(pattern, start, occurrence) with both forward (positive start) and backward (negative start) search, using efficient single‑pass UTF‑8 byte scanning.
Extended CollationAwareUTF8String.lowercaseIndexOf and ICU indexOf to support multi‑occurrence search for UTF8_BINARY_LCASE and ICU collations.
Updated FunctionRegistry to use the new StringInstrExpressionBuilder.
New overloads in functions.scala and corresponding Python APIs (pyspark.sql.functions.builtin) with proper default‑parameter handling.
New error class OCCURRENCE for invalid (≤ 0) occurrence values.
Comprehensive unit tests covering binary, lowercase, and ICU collations, forward/backward/overlapping searches, multi‑byte characters, and boundary conditions.

The behavior when substr is empty remains unchanged (returns start position), preserving backward compatibility with the existing two‑argument instr.

Why are the changes needed?

Currently, Spark's instr only supports two arguments, forcing users to write complex workarounds when they need to start searching from a specific position or find a particular occurrence. Oracle, Impala, and Db2 all provide a four‑argument INSTR with these capabilities. This PR aligns Spark SQL with those databases, simplifies user queries, and reduces migration friction.

Does this PR introduce any user-facing change?

Yes, but only additive.

New SQL syntax: instr(str, substr [, start [, occurrence]]) where start and occurrence are optional integers (default 1). Negative start performs backward search.
New Scala/Python API overloads with corresponding optional parameters.
Error raised when occurrence <= 0.
All existing two‑argument usage remains completely unaffected.

Documentation for the new parameters is included in the expression description and will be surfaced in SQL function docs.

How was this patch tested?

Added extensive unit tests to StringFunctionsSuite, CollationAwareUTF8StringSuite, and UTF8StringSuite covering:
- Forward and backward search with multiple occurrences
- Overlapping matches (e.g., 'aa' in 'aaa')
- Multi‑byte and supplementary characters
- Collation scenarios: UTF8_BINARY, UTF8_BINARY_LCASE, UNICODE_CI (including sigma variants, German ß, and Turkish İ expansion)
- Boundary conditions (start = 0, occurrence <= 0, start out of range, empty substring)
Verified error condition formatting in SparkThrowableSuite.
Verified Python API behavior matches SQL execution through existing PySpark doctests.

Was this patch authored or co-authored using generative AI tooling?

DeepSeek

loftiest · 2026-06-16T12:48:16Z

@uros-b @sarutak Could you please take a look at this PR when you have a chance? Thanks!

uros-b · 2026-06-16T12:59:33Z

+
+  override def nullSafeEval(string: Any, sub: Any, start: Any, occurrence: Any): Any = {
+    val occ = occurrence.asInstanceOf[Int]
+    if (occ <= 0) {


ANSI-gated error for occurrence <= 0 is inconsistent with convention. INVALID_PARAMETER_VALUE.* errors are usage errors and are conventionally thrown unconditionally, not gated on spark.sql.ansi.enabled (see e.g. BIT_POSITION_RANGE, PATTERN, etc.). Returning null in non-ANSI mode is surprising and differs from how a clearly-invalid argument is normally treated.

I'd recommend either always throwing. cc @cloud-fan @MaxGekk @srielau for this design decision

uros-b · 2026-06-16T13:00:25Z

      },
+      "OCCURRENCE" : {
+        "message" : [
+          "expects a positive value for `occurrence`, but got <actual>."


Error message restates the parameter name and hardcodes backticks. The parent INVALID_PARAMETER_VALUE message already prepends The value of parameter(s) occurrence in instr is invalid: using the / params you pass. Restating for occurrence is redundant with the convention used by every other subclass (they say e.g. "expects a binary value..." with no parameter name). Suggestion to use instead: "expects a positive integer, but got ."

Also cc @srielau for error handling ^^

uros-b · 2026-06-16T13:01:23Z

    }
  }

+  public static class StringInstr4 {


Nit: StringInstr4 is a poor class name. Naming the expression after its arity (4) is unusual for the codebase and will read oddly in stack traces / future maintenance. Something like StringInstrWithOccurrence (or folding the logic so there's one expression) would be more clear.

I've renamed StringInstr4 to StringInstrWithOccurrence, both in the expression class and in CollationSupport. This makes the purpose clearer.

uros-b · 2026-06-16T13:02:20Z

  since = "1.5.0",
  group = "string_funcs")
 // scalastyle:on line.size.limit
+object StringInstrExpressionBuilder extends ExpressionBuilder {


There are some unused auxiliary constructors. StringInstr4 defines three def this(...) overloads (2/3/4-arg), but StringInstrExpressionBuilder constructs the case class directly with explicit Literal(1) defaults, so these constructors appear to be dead code. Either use them from the builder or drop them.

Thanks for catching that. I've removed the unused def this(...) overloads.

uros-b · 2026-06-16T13:02:59Z

+      IntegerType
+    )
+
+  override def contextIndependentFoldable: Boolean = super.contextIndependentFoldable


This looks like a no-op override (it just calls super). It was copied from StringInstr (which has the same pointless line), but there's no need to propagate it.

Removed the no-op override. Thanks!

uros-b · 2026-06-16T13:06:59Z

+  public int indexOf(UTF8String pattern, int start, int occurrence) {
+    assert occurrence > 0;
+    if (pattern.numBytes() == 0) {
+      return indexOfEmpty(start);


There is one thing that I don't particularly like here, and it's reinforcing an old issue (noted in https://issues.apache.org/jira/browse/SPARK-48284).

The empty-pattern path delegates to indexOfEmpty(start), which unconditionally returns 0 (→ result 1) regardless of sign. So instr('hello', '', -1) returns 1, which is a bit arbitrary for a backward search.

I guess we're consistent with the old semantics (2-arg behavior) in the current PR, but I would suggest to keep this in mind for future improvements.

In any case, at least add some tests for this @loftiest.

I've added tests for empty substring with negative start.

Co-authored-by: Uros Bojanic <221401595+uros-b@users.noreply.github.com>

…e, clean up, add tests - Rename StringInstr4 to StringInstrWithOccurrence for clarity. - Remove unused auxiliary constructors (def this(...) overloads). - Remove no-op contextIndependentFoldable override. - Add unit tests for instr with empty substring and negative start. - Ensure collation-aware code references the updated class name.

Add start and occurrence parameters to instr function

0c19edb

uros-b reviewed Jun 16, 2026

View reviewed changes

Comment thread common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationAwareUTF8String.java Outdated

uros-b reviewed Jun 16, 2026

View reviewed changes

Comment thread python/pyspark/sql/functions/builtin.py Outdated

uros-b reviewed Jun 16, 2026

View reviewed changes

loftiest and others added 2 commits June 16, 2026 22:56

Apply suggestions from code review

874d033

Co-authored-by: Uros Bojanic <221401595+uros-b@users.noreply.github.com>

Conversation

loftiest commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

loftiest commented Jun 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

loftiest commented Jun 14, 2026 •

edited

Loading