Skip to content

fix: find_in_set internal error with two LargeUtf8 scalar arguments - #24802

Open
advitrocks9 wants to merge 2 commits into
apache:mainfrom
advitrocks9:spark-find-in-set
Open

fix: find_in_set internal error with two LargeUtf8 scalar arguments#24802
advitrocks9 wants to merge 2 commits into
apache:mainfrom
advitrocks9:spark-find-in-set

Conversation

@advitrocks9

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

find_in_set with two LargeUtf8 scalar arguments does not return a value, it trips the planner's type assertion:

> SELECT find_in_set(arrow_cast('ab', 'LargeUtf8'), arrow_cast('abc,b,ab,c,def', 'LargeUtf8'));
Internal error: Assertion failed: result_data_type == *expected_type: Function 'find_in_set'
returned value of type 'Int32' while the following type was promised at planning time and
expected: 'Int64'.

return_type goes through utf8_to_int_type, which widens to Int64 for LargeUtf8. The three array branches are generic over Int32Type/Int64Type and honour that. The (Scalar, Scalar) branch computed Some(position as i32) and passed it to ScalarValue::from, which is always ScalarValue::Int32, so it was the one branch that could contradict its own return_type. Nothing had executed it: string_literal.slt covers scalar/scalar for Utf8 and Utf8View only, and the LargeUtf8 block in string_query.slt.part casts columns, so they land in the array branches.

What changes are included in this PR?

  • Build the scalar result at the width return_field promised, with the same match on the return type the array branches already use four times.
  • Fill spark/string/find_in_set.slt, previously a stub. Expected values come from Spark 4.2.0, read off UTF8String.findInSet and confirmed on pyspark==4.2.0, not from running DataFusion.

The .slt is the larger half of the diff at 37 queries. Coverage that exists nowhere else in the tree: a needle containing a comma, empty and consecutive and trailing-comma elements, no trimming around delimiters, first occurrence wins on duplicates, and a 20-element list that crosses FIND_IN_SET_LOOKUP_THRESHOLD so the lookup path from #23460 gets its first end-to-end test. DataFusion matches Spark on all 37.

Are these changes tested?

Yes, the LargeUtf8 query above is the regression test. Reverting the source change and rebuilding gives exactly one failure, on that query, with the assertion above.

cargo test --profile=ci --test sqllogictests -- spark/string/find_in_set.slt
cargo test --profile=ci --test sqllogictests -- string/                       # 65 files
cargo test --profile=ci -p datafusion-functions --lib find_in_set             # 6 passed
cargo clippy --profile ci -p datafusion-functions --lib -- -D warnings

I left string/string_literal.slt alone. The new case runs in the same CI job, so a second copy next to the Utf8 block there catches nothing extra.

Are there any user-facing changes?

Yes. find_in_set over two LargeUtf8 scalars returns a value instead of raising an internal error. No API change.

@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Aug 30, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.53%. Comparing base (4b8ad88) to head (86f4a68).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/functions/src/unicode/find_in_set.rs 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #24802      +/-   ##
==========================================
- Coverage   81.53%   81.53%   -0.01%     
==========================================
  Files        1123     1123              
  Lines      406042   406106      +64     
  Branches   406042   406106      +64     
==========================================
+ Hits       331059   331108      +49     
- Misses      55621    55633      +12     
- Partials    19362    19365       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants