FEAT: Support context adjustment for udfs for pandas backend #2646

LeeTZ · 2021-02-24T16:37:48Z

Overview

This PR add context adjustment for udfs for pandas backend.
Previously, window context adjustment is tested with built-in aggregations. Context adjustment will fail when user put udf in window aggregations, for example:

@udf.reduction(['double'], 'double')
def my_mean(series):
    return series.mean()

window = ibis.trailing_window(ibis.interval(days=3), order_by='time')
expr = time_table.mutate(v1=my_mean(time_table['value']).over(window))
result = expr.execute(timecontext=context)

result will look like:

                     time   id  value    v1
0 2017-01-05 01:02:03.234  4.0    4.4  1.00
1 2017-01-06 01:02:03.234  5.0    5.5  1.50
2 2017-01-07 01:02:03.234  6.0    6.6  1.65
3 2017-01-08 01:02:03.234  7.0    7.7  2.75
4 2017-01-09 01:02:03.234  8.0    8.8  3.85
5                     NaT  NaN    NaN  4.95
6                     NaT  NaN    NaN  6.05
7                     NaT  NaN    NaN  7.15

Extra rows are added since the result Series of udf is not trimmed by timecontext.
We did implement rules to trim result Series in window execution. However it is based on a fact that the Series contains a time col, so we could extract time info and trim according to time. The result Series of udf doesn't include a time index in current implementation. This PR adds 'time' col (if present) as index for the result of udf execution. To match the result Series of built-in aggregations.

How is this tested

Test is added in ibis/backends/pandas/tests/execution/test_timecontext.py for udf use cases, including unit test for the core methodconstruct_time_context_aware_series, and integration test for udfs on different windows (when there is no groupby nor orderby, both groupby and orderby , only orderby etc.)

jreback

can you add a release note as well

ibis/expr/timecontext.py

jreback · 2021-02-24T16:48:58Z

ibis/expr/timecontext.py

@@ -127,6 +127,40 @@ def canonicalize_context(
    return begin, end


+def construct_multi_index_series(


can we have a more informative name for this. yes its constructing a MI series, but its doing it for time_context.
maybe

construct_time_context_aware_series

Good idea, I changed the name to be construct_time_context_aware_series

icexelloss · 2021-02-24T17:21:02Z

ibis/expr/timecontext.py

+def construct_multi_index_series(
+    series: pd.Series, frame: pd.DataFrame
+) -> pd.Series:
+    """ Construct a pd.MultiIndex of IntIndex and 'time'


Why IntIndex?

Is the IntIndex here presenting the groupby key? What about the case that groupby key is not integer?

I rephrase this a little bit, it meant to add 'time' into the original index, whatever type it is.

ibis/expr/timecontext.py

icexelloss · 2021-02-24T17:26:42Z

ibis/expr/timecontext.py

+    if TIME_COL == frame.index.name:
+        time_index = frame.index
+    elif TIME_COL in frame:
+        time_index = frame.set_index(TIME_COL).index


Probably question for Jeff: Is this the best way to construct an index from existing column?

no ,do this
pd.Index(frame[TIME_COL])

icexelloss · 2021-02-24T17:27:35Z

ibis/expr/timecontext.py

+
+    Parameters
+    ----------
+    series: pd.Series


Can you describe what is the series and what is the frame here?

Same as comments above, added description, as well as examples.

icexelloss · 2021-02-24T17:28:49Z

ibis/expr/timecontext.py

@@ -127,6 +127,40 @@ def canonicalize_context(
    return begin, end


+def construct_multi_index_series(


It also feels to me what the function is an implementation of the pandas backend and therefore, probably don't belong in ibis/expr/timecontext.py module. Is there a place you can put this in the pandas module?

I thought about this. This could be a util function in general, also putting this in pandas/execution/timecontext.py seems to cause circular dependency. I could revisit.

Why would it have a circular dependency?

icexelloss · 2021-02-24T17:30:10Z

ibis/backends/pandas/execution/window.py

@@ -338,6 +341,8 @@ def execute_window_op(
    )
    series = post_process(result, data, ordering_keys, grouping_keys)

+    if timecontext:
+        series = construct_multi_index_series(series, data)


Are the series and data matching in row ordering here?

i.e. I think the series is ordered in group_by ordering, and the data is ordered in original ordering (probably ordered by time).

It's a good point. I made some modifications here. Time index should be injected to series prior to groupby. I add logics in aggcontext.py for this, row order is well tracked in agg phase.

icexelloss

Left some comments.

LeeTZ · 2021-02-25T16:29:22Z

Update with a change in window.py and aggcontext.py to address row ordering for the series and parent DataFrame, when groupby is defined.
Also added more tests, including unit test for onstruct_time_context_aware_series, and integration test for udfs when there is no groupby nor orderby, both groupby and orderby , and only orderby
A relase note is also added.

ibis/backends/pandas/execution/window.py

ibis/backends/pandas/tests/execution/conftest.py

icexelloss · 2021-02-25T17:28:31Z

ibis/backends/pandas/execution/window.py

@@ -53,13 +62,18 @@ def _post_process_group_by(
    parent: pd.DataFrame,
    order_by: List[str],
    group_by: List[str],
+    **kwargs,


Should this be timecontext instead of kwargs?

It could be, but timecontext is currently not used in this function so I didn't make it as an explicit param

I think it's a good pattern to specify all param explicitly even some of them are not used. kwargs brings flexibility which I don't think you need here

(Explicitness over implicitness)

agree with @icexelloss here, we don't want kwargs at all.

thx, I removed kwargs

ibis/expr/timecontext.py

icexelloss · 2021-02-25T21:03:49Z

ibis/backends/pandas/tests/execution/test_timecontext.py

+    return series.mean()
+
+
+def test_context_adjustment_window_udf_nogroupby_noorderby(


Can you move these to the backend test and test this with pyspark backend as well?

icexelloss · 2021-02-25T21:06:32Z

ibis/expr/timecontext.py

+        1 2017-01-03 01:02:03.234   2    2.2
+        2 2017-01-04 01:02:03.234   3    3.3
+
+        For a series of:


Can you add some index here for better readability ?

For a series:
...
The result series will be

For a series:
...
The result will be
...

Good point, combining with Jeff's comment I update this to be runnable code / ipython output

jreback · 2021-02-25T22:15:02Z

ibis/backends/pandas/execution/window.py

@@ -53,13 +62,18 @@ def _post_process_group_by(
    parent: pd.DataFrame,
    order_by: List[str],
    group_by: List[str],
+    **kwargs,


agree with @icexelloss here, we don't want kwargs at all.

ibis/backends/pandas/tests/execution/test_timecontext.py

jreback · 2021-02-25T22:16:43Z

ibis/expr/timecontext.py

+    Examples:
+        Assume frame is:
+                             time  id  value
+        0 2017-01-02 01:02:03.234   1    1.1


can you actually programatically construct this, then show it.

also use

Examples -----------

IOW do this in ipython and basically copy it here (or alternatly do it in a python terminal and use >>>

idea is that this code can be copy pasted to a terminal for execution directly; so include imports too.

Good point thx

jreback · 2021-02-25T22:18:06Z

ibis/expr/timecontext.py

+        Name: value, dtype: float64
+
+        The result series will be:
+                             time


move all examples after Parameters

jreback · 2021-02-25T22:18:33Z

ibis/expr/timecontext.py

+    if TIME_COL == frame.index.name:
+        time_index = frame.index
+    elif TIME_COL in frame:
+        time_index = frame.set_index(TIME_COL).index


no ,do this
pd.Index(frame[TIME_COL])

…is into tiezheng/udf_context_adjustment

ibis/backends/pandas/tests/execution/test_timecontext.py

ibis/backends/pandas/aggcontext.py

… ibis config

LeeTZ · 2021-03-09T15:49:47Z

Move tests for timecontext for udfs to be under ibis/backends/test to test for all backends that support this feature (pandas & pyspark in this case). Tests are parameterized and use alltypes as the data source. Note that in order to make this work, I also move TIME_COL from a constant in timecontext.py to be an ibis config that is configurable on test runs.

icexelloss · 2021-03-09T16:17:03Z

ibis/backends/pandas/execution/generic.py

@@ -920,6 +920,7 @@ def execute_database_table_client(
    df = client.dictionary[op.name]
    if timecontext:
        begin, end = timecontext
+        TIME_COL = get_time_col()


This looks funky, why are we setting a constant here?

Probably I should make this not capitalized to avoid misunderstading here. TIME_COL is not a constant anymore, this just means, read from config for the name of time col and assign the value to a variable, for later execution.

ok yeah, upper case is confusing because it looks like you are reassigning value to a constant.

…is into tiezheng/udf_context_adjustment

jreback · 2021-03-09T22:26:16Z

ibis/backends/pandas/execution/window.py

-    # indexes are retturned.
-    if TIME_COL not in df:
+    # indexes are returned.
+    TIME_COL = get_time_col()


same comment here (as you describe above)

ibis/backends/pandas/tests/execution/test_timecontext.py

ibis/backends/tests/test_timecontext.py

jreback · 2021-03-09T22:30:10Z

ibis/expr/timecontext.py

+    pd.Series
+
+    Examples
+    -----------


underline should be the same length

jreback · 2021-03-09T22:30:48Z

ibis/expr/timecontext.py

+    Name: value, dtype: float64
+    The result is unchanged for a series already has 'time' as its index.
+    """
+    TIME_COL = get_time_col()


same comment as above (make this lowercase)

…is into HEAD

icexelloss

LGTM

icexelloss · 2021-03-10T18:08:12Z

ibis/__init__.py

@@ -33,6 +33,13 @@
    validator=ibis.config.is_bool,
 )
 ibis.config.register_option('default_backend', None)
+ibis.config.register_option(
+    'time_col',


Can be a follow up. We should properly namespace this to be
context_adjustment.time_col

jreback · 2021-03-10T20:23:11Z

thanks @LeeTZ very nice!

context adjustment for udfs

b1d18da

jreback added the expressions Issues or PRs related to the expression API label Feb 24, 2021

jreback suggested changes Feb 24, 2021

View reviewed changes

icexelloss reviewed Feb 24, 2021

View reviewed changes

ibis/expr/timecontext.py Show resolved Hide resolved

icexelloss reviewed Feb 24, 2021

View reviewed changes

ibis/expr/timecontext.py Show resolved Hide resolved

icexelloss reviewed Feb 24, 2021

View reviewed changes

icexelloss suggested changes Feb 24, 2021

View reviewed changes

LeeTZ added 3 commits February 25, 2021 11:02

add tests for groupby, orderby

8046f06

add release note

9cad131

Merge branch 'master' into tiezheng/udf_context_adjustment

08261f5

icexelloss reviewed Feb 25, 2021

View reviewed changes

ibis/backends/pandas/execution/window.py Show resolved Hide resolved

icexelloss reviewed Feb 25, 2021

View reviewed changes

ibis/backends/pandas/execution/window.py Outdated Show resolved Hide resolved

icexelloss reviewed Feb 25, 2021

View reviewed changes

ibis/backends/pandas/tests/execution/conftest.py Show resolved Hide resolved

icexelloss reviewed Feb 25, 2021

View reviewed changes

ibis/expr/timecontext.py Show resolved Hide resolved

comments, test for trim_with_context

5fc10a0

icexelloss reviewed Feb 25, 2021

View reviewed changes

jreback added this to the Next release milestone Feb 25, 2021

jreback suggested changes Feb 25, 2021

View reviewed changes

icexelloss added the pandas The pandas backend label Feb 26, 2021

LeeTZ force-pushed the tiezheng/udf_context_adjustment branch from 2b971fd to 85d05a2 Compare February 26, 2021 19:51

Merge branch 'tiezheng/udf_context_adjustment' of github.com:LeeTZ/ib…

3dc41d7

…is into tiezheng/udf_context_adjustment

LeeTZ force-pushed the tiezheng/udf_context_adjustment branch from 85d05a2 to 3dc41d7 Compare March 4, 2021 15:33

extend tests for pyspark backend

71723e4

Merge branch 'master' into tiezheng/udf_context_adjustment

2a41b30

icexelloss reviewed Mar 4, 2021

View reviewed changes

ibis/backends/pandas/tests/execution/test_timecontext.py Outdated Show resolved Hide resolved

icexelloss reviewed Mar 4, 2021

View reviewed changes

ibis/backends/pandas/aggcontext.py Outdated Show resolved Hide resolved

extend timecontext test for all backends with alltypes, add 'time' to…

3ce1bef

… ibis config

icexelloss reviewed Mar 9, 2021

View reviewed changes

Merge branch 'tiezheng/udf_context_adjustment' of github.com:LeeTZ/ib…

b7c0521

…is into tiezheng/udf_context_adjustment

LeeTZ force-pushed the tiezheng/udf_context_adjustment branch from 0ad5eeb to b7c0521 Compare March 9, 2021 16:19

jreback reviewed Mar 9, 2021

View reviewed changes

LeeTZ force-pushed the tiezheng/udf_context_adjustment branch 4 times, most recently from a7cf598 to b7c0521 Compare March 10, 2021 16:40

LeeTZ added 2 commits March 10, 2021 11:41

Merge branch 'master' into tiezheng/udf_context_adjustment

5ee762a

lowercase TIME_COL, fix style

2d1f2c5

LeeTZ force-pushed the tiezheng/udf_context_adjustment branch from 5ee762a to b7c0521 Compare March 10, 2021 17:00

LeeTZ added 2 commits March 10, 2021 12:01

Merge branch 'master' into tiezheng/udf_context_adjustment

e77c8ff

Merge branch 'tiezheng/udf_context_adjustment' of github.com:LeeTZ/ib…

516311e

…is into HEAD

LeeTZ force-pushed the tiezheng/udf_context_adjustment branch from e77c8ff to b7c0521 Compare March 10, 2021 17:03

icexelloss approved these changes Mar 10, 2021

View reviewed changes

blank lines

44ea049

icexelloss reviewed Mar 10, 2021

View reviewed changes

jreback approved these changes Mar 10, 2021

View reviewed changes

jreback merged commit 3e250a8 into ibis-project:master Mar 10, 2021

LeeTZ mentioned this pull request Mar 11, 2021

ENH: Namespace time_col config, fix type check for trim_with_timecontext for pandas window execution. #2680

Merged

cpcloud removed this from the Next release milestone Jan 7, 2022

		@@ -127,6 +127,40 @@ def canonicalize_context(
		return begin, end


		def construct_multi_index_series(

		return series.mean()


		def test_context_adjustment_window_udf_nogroupby_noorderby(

FEAT: Support context adjustment for udfs for pandas backend #2646

FEAT: Support context adjustment for udfs for pandas backend #2646

Conversation

LeeTZ commented Feb 24, 2021 • edited Loading

Overview

How is this tested

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icexelloss Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icexelloss left a comment

Choose a reason for hiding this comment

LeeTZ commented Feb 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icexelloss Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeeTZ commented Mar 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icexelloss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Mar 10, 2021

LeeTZ commented Feb 24, 2021 •

edited

Loading

icexelloss Feb 24, 2021 •

edited

Loading

icexelloss Feb 25, 2021 •

edited

Loading