feat: comparison ops for list and struct types#6104
Merged
universalmind303 merged 2 commits intoEventual-Inc:mainfrom Feb 3, 2026
Merged
feat: comparison ops for list and struct types#6104universalmind303 merged 2 commits intoEventual-Inc:mainfrom
universalmind303 merged 2 commits intoEventual-Inc:mainfrom
Conversation
Contributor
Greptile OverviewGreptile SummaryThis PR implements comparison operations ( Key Changes:
Minor Issue:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Series
participant ListArray
participant RowFastPath
participant ElementWise
participant RowConverter
User->>Series: compare(lhs, rhs, op)
Series->>Series: Check if List/FixedSizeList/Struct type
alt List/FixedSizeList/Struct
Series->>ListArray: compare_list_like_array(lhs, rhs, op)
ListArray->>ListArray: comparison_broadcast_info()
ListArray->>RowFastPath: row_fast_path()?
alt No nulls + No floats + Supported type
RowFastPath->>RowFastPath: type_contains_float() → false
RowFastPath->>RowFastPath: array_has_any_nulls() → false
RowFastPath->>RowConverter: convert_columns()
RowConverter-->>RowFastPath: row representations
RowFastPath->>RowFastPath: Compare rows using cmp()
RowFastPath-->>ListArray: Some(BooleanArray)
ListArray-->>Series: BooleanArray
else Contains nulls/floats or unsupported
RowFastPath-->>ListArray: None (fallback)
ListArray->>ElementWise: compare_list_like_value() per element
loop For each array index
ElementWise->>ElementWise: Get element at index
ElementWise->>Series: compare_scalar_series()
Series-->>ElementWise: Option<bool>
end
ElementWise-->>ListArray: BooleanArray
ListArray-->>Series: BooleanArray
end
else Primitive types
Series->>Series: Use existing comparison logic
end
Series-->>User: BooleanArray result
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+48
to
+59
| pub fn is_comparison(&self) -> bool { | ||
| matches!( | ||
| self, | ||
| Self::Eq | ||
| | Self::EqNullSafe | ||
| | Self::NotEq | ||
| | Self::Lt | ||
| | Self::LtEq | ||
| | Self::Gt | ||
| | Self::GtEq | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
Check that is_comparison() behavior change doesn't break existing code. Old version included logical operators (And/Or/Xor), new version correctly excludes them
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/daft-core/src/operator.rs
Line: 48:59
Comment:
Check that `is_comparison()` behavior change doesn't break existing code. Old version included logical operators (`And`/`Or`/`Xor`), new version correctly excludes them
How can I resolve this? If you propose a fix, please make it concise.
aaron-ang
force-pushed
the
cmp-list-struct
branch
from
ca92074 to
d694e63
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6104 +/- ##
==========================================
- Coverage 43.34% 43.26% -0.09%
==========================================
Files 917 921 +4
Lines 112995 114222 +1227
==========================================
+ Hits 48978 49417 +439
- Misses 64017 64805 +788
🚀 New features to boost your workflow:
|
aaron-ang
force-pushed
the
cmp-list-struct
branch
from
d694e63 to
4cf4b23
Compare
| } | ||
|
|
||
| #[test] | ||
| fn compare_list_arrays_basic() -> DaftResult<()> { |
Member
There was a problem hiding this comment.
it'd be nice to have a few meor test cases here. maybe we could use rstest to create some parametrized tests to cover a larger area. We have other parts of the codebse that use parametrized rstests.
Member
universalmind303
left a comment
universalmind303
left a comment
There was a problem hiding this comment.
Overall this looks great! good work @aaron-ang
two things i'd like to see before we merge this in.
- a few more rust based unit tests
- python unit tests testing the compare ops using series & dataframes.
universalmind303
approved these changes
Feb 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes Made
Operatorenum todaft-coreto reuse comparison ops.arrow-row'sRowConverterif specific conditions are met, falling back to element-wise comparison.Related Issues
Closes #3510.