Skip to content
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

feat(api): day_of_week.iso_index() method #9003

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kaijennissen
Copy link
Contributor

Description of changes

Added a new isoday_of_week method for timestamps.

Issues closed

Resolves #8989

Copy link
Contributor

ACTION NEEDED

Ibis follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message.

Please update your PR title and description to match the specification.

@kaijennissen kaijennissen changed the title isoday_of_week method feat(types): isoday_of_week method Apr 18, 2024
@cpcloud
Copy link
Member

cpcloud commented Apr 18, 2024

We have a DayOfWeek class defined in ibis/expr/types/temporal.py.

It's a bit of an oddball API in that it functions like a namespace instead of top level method like everything else.

Naturally, your first development experience with Ibis is extending a slightly weird bit of the codebase 😂.

Perhaps we can add two new methods there?

class DayOfWeek:
    ...

    def iso_index(self): ...
    def iso_full_name(self): ...

for consistency with the surrounding methods?

@cpcloud
Copy link
Member

cpcloud commented Apr 18, 2024

Here's a usage example:

In [8]: t = ibis.read_parquet("/data/avg_duration_over_time.parquet")

In [9]: t.head()
Out[9]:
┏━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━┓
┃ date       ┃ avg_duration_m ┃
┡━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━┩
│ date       │ float64        │
├────────────┼────────────────┤
│ 2024-04-16 │      57.833333 │
│ 2024-04-15 │      42.833333 │
│ 2024-04-14 │      31.500000 │
│ 2024-04-13 │      53.600000 │
│ 2024-04-12 │      47.400000 │
└────────────┴────────────────┘

In [10]: t.select(
    ...:     "date",
    ...:     weekday=t.date.day_of_week.index(),
    ...:     weekday_name=t.date.day_of_week.full_name(),
    ...: )
Out[10]:
┏━━━━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━┓
┃ date       ┃ weekday ┃ weekday_name ┃
┡━━━━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━┩
│ date       │ int16   │ string       │
├────────────┼─────────┼──────────────┤
│ 2024-04-16 │       1 │ Tuesday      │
│ 2024-04-15 │       0 │ Monday       │
│ 2024-04-14 │       6 │ Sunday       │
│ 2024-04-13 │       5 │ Saturday     │
│ 2024-04-12 │       4 │ Friday       │
│ 2024-04-10 │       2 │ Wednesday    │
│ 2024-04-09 │       1 │ Tuesday      │
│ 2024-04-07 │       6 │ Sunday       │
│ 2024-04-06 │       5 │ Saturday     │
│ 2024-04-05 │       4 │ Friday       │
│ …          │       … │ …            │
└────────────┴─────────┴──────────────┘

@cpcloud
Copy link
Member

cpcloud commented Apr 18, 2024

Alternatively, I can take a crack at this one, if you want to handle iso_year()

@kaijennissen
Copy link
Contributor Author

kaijennissen commented Apr 18, 2024

We have a DayOfWeek class defined in ibis/expr/types/temporal.py.

I missed that.

Alternatively, I can take a crack at this one, if you want to handle iso_year()

I'll give it a try. And iso_yearwith a different PR.

@kaijennissen
Copy link
Contributor Author

Is it possible that there are currently no tests for .day_of_week.index and .day_of_week.full_name? Was looking to place a test nearby but couldn't find them.

@cpcloud
Copy link
Member

cpcloud commented Apr 19, 2024

@kaijennissen You should be able to find the tests that run across backends in ibis/backends/tests/test_temporal.py:

❯ rg test_day_of_week
ibis/backends/bigquery/tests/unit/test_compiler.py
72:def test_day_of_week(case, dtype, snapshot):

ibis/backends/impala/tests/test_client.py
202:def test_day_of_week(con):

ibis/backends/tests/test_temporal.py
1463:def test_day_of_week_scalar(con, date, expected_index, expected_day):
1484:def test_day_of_week_column(backend, alltypes, df):
1529:def test_day_of_week_column_group_by(

@kaijennissen
Copy link
Contributor Author

kaijennissen commented Apr 20, 2024

@cpcloud The sqlite tests pass locally. The exasol as well as the impala containers do not work thus I cannot test this locally. I have to investigate this a little deeper. I have no idea why the docs-test fails.

@cpcloud cpcloud added this to the 9.2 milestone Jun 13, 2024
@cpcloud cpcloud added feature Features or general enhancements ux User experience related issues timestamps Issues related to the timestamp API labels Jul 15, 2024
@cpcloud cpcloud force-pushed the #8989 branch 2 times, most recently from 4655c8a to be291cd Compare July 16, 2024 13:01
@cpcloud cpcloud marked this pull request as ready for review July 16, 2024 13:03
@cpcloud cpcloud changed the title feat(types): isoday_of_week method feat(api,timestamps): day_of_week.iso_index() method Jul 16, 2024
@cpcloud cpcloud changed the title feat(api,timestamps): day_of_week.iso_index() method feat(api): day_of_week.iso_index() method Jul 16, 2024
@cpcloud
Copy link
Member

cpcloud commented Jul 16, 2024

@kaijennissen Are you still interested in this feature? If so, would you mind trying it out for your use case and reporting back?

@cpcloud cpcloud removed this from the 9.2 milestone Jul 19, 2024
@jcrist jcrist self-assigned this Aug 12, 2024
@kaijennissen
Copy link
Contributor Author

@cpcloud Sorry for the late answer. It works for my use case, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Features or general enhancements timestamps Issues related to the timestamp API ux User experience related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possibility to extract isoyear and isoweekday (or weekday) from a timestamp column.
3 participants