You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/01/15 02:27:52 UTC
[PR] feat: add support for array_position expression [datafusion-comet]
andygrove opened a new pull request, #3172:
URL: https://github.com/apache/datafusion-comet/pull/3172
## Summary
- Adds native Comet support for Spark's `array_position` function
- Returns the 1-based position of an element in an array, or 0 if not found
## Implementation Details
This required a custom Rust implementation because DataFusion's `array_position` returns `UInt64` and `null` when not found, while Spark returns `Int64` (LongType) and `0`.
Key implementation details:
- Returns `Int64` to match Spark's `LongType`
- Returns `0` when element is not found (Spark behavior)
- Returns `null` when array is `null` or search element is `null`
- Supports both `List` and `LargeList` array types
## Test Plan
- Added unit tests for `array_position` in `CometArrayExpressionSuite`
- Tests cover:
- Finding elements in integer arrays
- Finding elements in string arrays
- Element not found (returns 0)
- Arrays with null elements
- Column-based queries (not just literals)
- All existing tests pass
> **Note:** This PR was generated with AI assistance.
Closes #3153
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add support for array_position expression [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3172:
URL: https://github.com/apache/datafusion-comet/pull/3172#issuecomment-3824211229
Moving this to draft until https://github.com/apache/datafusion-comet/pull/3328 is merged
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add support for array_position expression [datafusion-comet]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3172:
URL: https://github.com/apache/datafusion-comet/pull/3172#issuecomment-3755472674
## [Codecov](https://app.codecov.io/gh/apache/datafusion-comet/pull/3172?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `13 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 9.92%. Comparing base ([`f09f8af`](https://app.codecov.io/gh/apache/datafusion-comet/commit/f09f8af64c6599255e116a376f4f008f2fd63b43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`8cb27ec`](https://app.codecov.io/gh/apache/datafusion-comet/commit/8cb27ec7c67aa02715e9e862babca7daf5cd900f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 849 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion-comet/pull/3172?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...src/main/scala/org/apache/comet/serde/arrays.scala](https://app.codecov.io/gh/apache/datafusion-comet/pull/3172?src=pr&el=tree&filepath=spark%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fcomet%2Fserde%2Farrays.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3Bhcmsvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9jb21ldC9zZXJkZS9hcnJheXMuc2NhbGE=) | 0.00% | [12 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion-comet/pull/3172?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../scala/org/apache/comet/serde/QueryPlanSerde.scala](https://app.codecov.io/gh/apache/datafusion-comet/pull/3172?src=pr&el=tree&filepath=spark%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fcomet%2Fserde%2FQueryPlanSerde.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3Bhcmsvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9jb21ldC9zZXJkZS9RdWVyeVBsYW5TZXJkZS5zY2FsYQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion-comet/pull/3172?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #3172 +/- ##
============================================
- Coverage 56.12% 9.92% -46.21%
+ Complexity 976 234 -742
============================================
Files 119 168 +49
Lines 11743 15584 +3841
Branches 2251 2565 +314
============================================
- Hits 6591 1547 -5044
- Misses 4012 13860 +9848
+ Partials 1140 177 -963
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/datafusion-comet/pull/3172?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add support for array_position expression [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3172:
URL: https://github.com/apache/datafusion-comet/pull/3172#issuecomment-4285413856
> Thanks @andygrove we would need to backport it to datafusion-spark crate.
>
> For testing should we test all possible datatypes?
I added support for timestamp_ntz
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add support for array_position expression [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #3172:
URL: https://github.com/apache/datafusion-comet/pull/3172#issuecomment-4245078575
Claude summarized my notes for me. Hopefully it didn't transcribe anything wrong or hallucinate :)
> ## PR #3172: `array_position`
>
> Good implementation overall. Spark compatibility looks correct: returns `Int64`/`0` (not DataFusion's `UInt64`/`null`), NaN equality handled properly, null array/element returns null, null values within arrays are skipped, 1-based indexing is right. Benchmarks show 1.1-1.2X faster than Spark.
>
> ### NULL buffer handling
>
> All the typed paths (`position_primitive`, `position_float`, `position_boolean`, `position_string`, `position_fallback`) build a `Vec<Option<i64>>` and convert to `Int64Array::from()`. This constructs the null buffer element-by-element.
>
> Recent DataFusion optimizations (e.g. apache/datafusion#21464, apache/datafusion#21471, apache/datafusion#21482) show 3-37% improvement by computing the null buffer upfront:
>
> 1. `NullBuffer::union(list_array.nulls(), element.nulls())` to get the combined output nulls
> 2. Build `Vec<i64>` (not `Option<i64>`) for the values
> 3. Construct via `Int64Array::new(ScalarBuffer::from(values), combined_nulls)`
>
> This avoids per-row `is_null()` checks in the hot loop and the Option tracking overhead. Not a blocker given the speedup is already decent, but it's a straightforward optimization that could widen the gap further.
>
> ### Signature
>
> `Signature::variadic_any` accepts any number of arguments of any type. Since `array_position` always takes exactly 2, `Signature::new(TypeSignature::Any(2), Volatility::Immutable)` would be more precise. The serde already validates so this is just defense in depth.
>
> ### Unwrap on downcast
>
> `downcast_ref::<GenericListArray<O>>().unwrap()` in `generic_array_position` could panic on invalid input. Should be unreachable given the match guard, but `.ok_or_else(|| DataFusionError::Internal("expected list array".into()))?` would be safer and consistent with patterns elsewhere.
>
> ### Test gap
>
> Consider adding a nested array test like `array_position(array(array(1,2), array(3,4)), array(1,2))`. Spark's unit tests cover this case and it would exercise the `position_fallback` code path.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add support for array_position expression [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3172:
URL: https://github.com/apache/datafusion-comet/pull/3172#issuecomment-4285428890
> > Thanks @andygrove we would need to backport it to datafusion-spark crate.
> > For testing should we test all possible datatypes?
>
> I added ~support~ tests for timestamp_ntz
Also added some nested array columnar tests
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add support for array_position expression [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #3172:
URL: https://github.com/apache/datafusion-comet/pull/3172
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add support for array_position expression [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3172:
URL: https://github.com/apache/datafusion-comet/pull/3172#issuecomment-4016699119
> Just wondering can we reuse DF? the builtin function gets optimized in [apache/datafusion#20532](https://github.com/apache/datafusion/pull/20532)
The DF implementation isn't compatible with Spark though.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org