test(sqlalchemy): cover AthenaTimestamp.process and simplify AthenaDate isinstance - #744
Merged
laughingman7743 merged 1 commit intoJul 26, 2026
Conversation
…te isinstance Follow-up to #743, which fixed a missing return in AthenaDate.process. AthenaTimestamp.process is the sibling of that method and had no unit test, so nothing guarded its millisecond truncation: strftime("%f") emits six digits and the [:-3] slice narrows them to the three Athena TIMESTAMP supports. Add cases for that truncation, for a whole-second datetime, and for the non-datetime fallback, plus the matching fallback case for AthenaDate. Also drop the redundant datetime from AthenaDate.process's isinstance check: datetime is a subclass of date, so isinstance(value, date) already matches both. The annotation is narrowed to date | Any for the same reason. Swept the whole tree for the #742 bug class (an expression statement whose value is computed and discarded) with an AST walk. No ruff rule catches it - B018 deliberately skips strings so it cannot see a bare f-string - and the only other hits were a PEP 258 attribute docstring and an intentional __getitem__ inside pytest.raises. #742 was the sole real instance.
laughingman7743
marked this pull request as ready for review
July 26, 2026 01:09
laughingman7743
deleted the
fix/athena-timestamp-test-and-date-isinstance
branch
July 26, 2026 01:09
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
1 participant
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.
Follow-up to #743.
WHAT
TestAthenaTimestampcoveringAthenaTimestamp.process: the millisecond truncation, a whole-seconddatetime, and the non-datetimefallback. Add the matching fallback case toTestAthenaDate.AthenaDate.process:isinstance(value, (date, datetime))→isinstance(value, date), and narrow the annotation todate | Any.WHY
#743 fixed a missing
returninAthenaDate.process. Its siblingAthenaTimestamp.processis correct today, but had no test — nothing guarded thestrftime("%f")[:-3]slice that narrows six digits to the three AthenaTIMESTAMPsupports. These are pure-logic tests, so they need no AWS credentials.datetimeis a subclass ofdate, so the tuple in theisinstancecheck was redundant and thedate | datetime | Anyannotation collapsed toAny.Sweep for the same bug class
Since #742 was a computed-then-discarded expression, I checked whether the tree holds others. No ruff rule catches this —
B018deliberately skips strings so it cannot see a bare f-string, and--select ALLon a reduced case reports nothing. An AST walk overpyathena/,tests/, andbenchmarks/for discardedast.Exprstatements returned two hits, both legitimate:pyathena/common.py:33— PEP 258 attribute docstring for theOnPollCallbackaliastests/pyathena/sqlalchemy/test_types.py:76— intentional__getitem__insidepytest.raises(KeyError)#742 was the only real instance.