-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: Improve error message for temporal vs non-temporal comparison #25927
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: Improve error message for temporal vs non-temporal comparison #25927
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves the error message when comparing temporal dtypes (Date, Datetime, Duration, Time) with non-temporal values. Previously, the error was generic: "Series of type Date does not have eq operator". The new error is more descriptive: "Invalid comparison between temporal dtype Date and non-temporal value of dtype <class 'int'>. Temporal values may only be compared against compatible temporal dtypes."
Key changes:
- Added a specific error check for temporal vs non-temporal comparisons before falling back to the generic "does not have operator" error
- The check uses
is_temporal()method to identify temporal dtypes and distinguish them from other types
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if f is None: | ||
| other_dtype = getattr(other, "dtype", type(other)) | ||
| if ( | ||
| self.dtype.is_temporal() |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The temporal dtype check should exclude None values. None/null values should be comparable with any dtype, including temporal dtypes. If the FFI function lookup fails for a None comparison, this check would incorrectly raise a TypeError categorizing None as a "non-temporal value". Add and other is not None to the condition on line 834 before checking if the other value is non-temporal.
| self.dtype.is_temporal() | |
| self.dtype.is_temporal() | |
| and other is not None |
| other_dtype = getattr(other, "dtype", type(other)) | ||
| if ( | ||
| self.dtype.is_temporal() | ||
| and not getattr(other_dtype, "is_temporal", lambda: False)() | ||
| ): | ||
| msg = ( | ||
| "Invalid comparison between temporal dtype " | ||
| f"{self.dtype!r} and non-temporal value of dtype {other_dtype!r}. " | ||
| "Temporal values may only be compared against compatible temporal dtypes." | ||
| ) | ||
| raise TypeError(msg) |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new error message should have test coverage to ensure it's raised correctly when comparing temporal dtypes (Date, Datetime, Duration, Time) with non-temporal scalar values (int, str, float, etc.). Consider adding tests that verify the error message is shown for each temporal dtype when compared with various non-temporal types.
|
The CI failures are coming from the delta-lake IO tests:
They are related to Keeping this here for visibility — maintainer guidance appreciated if any |
|
I fixed the issue that causes the tests to fail, update your branch an rerun the github actions and the tests should pass |
658a9d3 to
bdb544a
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25927 +/- ##
==========================================
- Coverage 80.52% 80.51% -0.02%
==========================================
Files 1764 1765 +1
Lines 243008 242842 -166
Branches 3045 3046 +1
==========================================
- Hits 195687 195514 -173
- Misses 46537 46543 +6
- Partials 784 785 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
All CI tests are passing now. The Codecov warning is only from the new temporal vs non-temporal Please let me know if you'd like me to add a small unit test for coverage. |
When comparing temporal dtypes (Date / Datetime / Duration / Time)
against non-temporal values, Polars previously raised:
This error was misleading, because the operator does exist — the
comparison is simply invalid due to incompatible types.
This change improves the error message to explicitly state that a
temporal value is being compared against a non-temporal dtype, e.g.:
This makes the behavior clearer and more consistent with user
expectations when performing mixed-dtype comparisons.
Closes #25729