BUG: Fix time context trimming error for multi column udfs in pandas backend #2712

LeeTZ · 2021-03-26T12:42:54Z

Overview

When specifying time context, multi-column udf execution for pandas backend may end up with an exception. This PR aims to fix the issue.

Example

For the following example:

@analytic(
    input_type=[dt.double, dt.double],
    output_type=dt.Struct(['demean', 'demean_weight'], [dt.double, dt.double]),
)
def demean_struct(v, w):
    return v - v.mean(), w - w.mean()

w = ibis.window(preceding=None, following=None)
result = expr.mutate(
    demean_struct(expr['double_col'], expr['int_col'])
    .over(w)
    .destructure()
).execute(timecontext=context)

To compute a multi column udf over a table, the execution may fail with exception:

ibis/backends/pandas/execution/window.py:375: in execute_window_op
    computed = trim_window_result(computed, timecontext)
ibis/backends/pandas/execution/window.py:219: in trim_window_result
    name = data.name if data.name else 0

self =                             demean  demean_weight
   timestamp_col                                 
0  2009-01-05 00:4...5.25            2.5
58 2009-01-10 01:38:04.330   35.35            3.5
59 2009-01-10 01:39:04.410   45.45            4.5
name = 'name'

    def __getattr__(self, name: str):
        """After regular attribute access, try looking up the name
        This allows simpler access to columns for interactive use.
        """
    
        # Note: obj.x will always call obj.__getattribute__('x') prior to
        # calling obj.__getattr__('x').
    
        if (
            name in self._internal_names_set
            or name in self._metadata
            or name in self._accessors
        ):
            return object.__getattribute__(self, name)
        else:
            if self._info_axis._can_hold_identifiers_and_holds_name(name):
                return self[name]
>           return object.__getattribute__(self, name)
E           AttributeError: 'DataFrame' object has no attribute 'name'

The reason of failing is that, in Window execution for pandas backend, we assume the computed result is always a pd.Series. However in such cases of multi-column udf, the result of op AnalyticVectorizedUDF is a pd.DataFrame.

For the fix, we add logic to deal with both pd.Series and pd.DataFrame in trimming the computed result for a pandas WindowOp

Test

Test case to cover this multi-column udf case is added in ibis/backends/tests/test_timecontext.py

jreback

can you add this PR number to the prior release note for changes in time context

jreback · 2021-03-26T13:00:08Z

ibis/backends/pandas/execution/window.py


    """
    # noop if timecontext is None
    if not timecontext:
        return data
+    assert isinstance(data, pd.Series) or isinstance(


assert(isinstance, (pd.Series, pd.DataFrame))

jreback · 2021-03-26T13:01:24Z

ibis/backends/pandas/execution/window.py

+        name = data.name if data.name else 0
+        index_columns = list(subset.columns.difference([name]))
+    else:
+        name = data.columns.tolist()


you don't need the .tolist() here just pass name directly to .difference

Oops forgot to remove this, +1

icexelloss · 2021-03-26T13:42:28Z

ibis/backends/tests/test_timecontext.py

+    input_type=[dt.double, dt.double],
+    output_type=dt.Struct(['demean', 'demean_weight'], [dt.double, dt.double]),
+)
+def demean_struct(v, w):


Can we import these UDFs instead of duplicating

Yeah, I will just import from vectorized_udf tests

icexelloss · 2021-03-26T16:45:19Z

ibis/backends/pandas/execution/window.py

@@ -361,15 +371,16 @@ def execute_window_op(
        clients=clients,
        **kwargs,
    )
-    series = post_process(
+    computed = post_process(


calling this window_result is a little better I think

Also fine to reuse result as well

Sure, I will keep using result

ibis/backends/pandas/execution/window.py

icexelloss · 2021-03-26T18:05:58Z

ibis/backends/pandas/execution/window.py

@@ -179,26 +179,32 @@ def get_aggcontext_window(
    return aggcontext


-def trim_window_series(data, timecontext: Optional[TimeContext]):
+def trim_window_result(data, timecontext: Optional[TimeContext]):


Can you annotate the type of "data"?

Yeah that would be helpful, added

ibis/backends/pandas/execution/window.py

icexelloss · 2021-03-26T18:08:31Z

ibis/backends/pandas/execution/window.py

+        index_columns = list(subset.columns.difference([name]))
+    else:
+        name = data.columns
+        index_columns = list(subset.columns.difference(name))

    # set the correct index for return Seires


Please fix comment

Series -> Series/DataFrame

icexelloss · 2021-03-26T18:10:50Z

Minor comments. Otherwise LGTM.

…bis into tiezheng/multi-col-udf-nogroupby

icexelloss

LGTM

jreback · 2021-03-26T18:36:23Z

thanks @LeeTZ ping on green.

Fix exception for multi-col udf time context trimming

c8b9a11

LeeTZ changed the title ~~BUG: Fix time context trimming error fro multi column udfs~~ BUG: Fix time context trimming error for multi column udfs in pandas backend Mar 26, 2021

jreback added the udf Issues related to user-defined functions label Mar 26, 2021

jreback added this to the Next release milestone Mar 26, 2021

jreback suggested changes Mar 26, 2021

View reviewed changes

icexelloss reviewed Mar 26, 2021

View reviewed changes

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

LeeTZ added 3 commits March 26, 2021 13:58

refactor test to import udf

3c78d54

add release note

42fafe1

Merge branch 'master' into tiezheng/multi-col-udf-nogroupby

49275cb

icexelloss added context adjustment pandas The pandas backend labels Mar 26, 2021

icexelloss reviewed Mar 26, 2021

View reviewed changes

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

icexelloss reviewed Mar 26, 2021

View reviewed changes

LeeTZ added 3 commits March 26, 2021 14:13

fix comment

a4099dd

Merge branch 'tiezheng/multi-col-udf-nogroupby' of github.com:LeeTZ/i…

263355b

…bis into tiezheng/multi-col-udf-nogroupby

fix comment

4b05a71

icexelloss approved these changes Mar 26, 2021

View reviewed changes

jreback approved these changes Mar 26, 2021

View reviewed changes

jreback merged commit f6910dc into ibis-project:master Mar 26, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix time context trimming error for multi column udfs in pandas backend #2712

BUG: Fix time context trimming error for multi column udfs in pandas backend #2712

LeeTZ commented Mar 26, 2021

jreback left a comment

jreback Mar 26, 2021

LeeTZ Mar 26, 2021

jreback Mar 26, 2021

LeeTZ Mar 26, 2021

icexelloss Mar 26, 2021

LeeTZ Mar 26, 2021

icexelloss Mar 26, 2021

LeeTZ Mar 26, 2021

icexelloss Mar 26, 2021

LeeTZ Mar 26, 2021

icexelloss Mar 26, 2021

icexelloss Mar 26, 2021

LeeTZ Mar 26, 2021

icexelloss commented Mar 26, 2021

icexelloss left a comment

jreback commented Mar 26, 2021

BUG: Fix time context trimming error for multi column udfs in pandas backend #2712

BUG: Fix time context trimming error for multi column udfs in pandas backend #2712

Conversation

LeeTZ commented Mar 26, 2021

Overview

Example

Test

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icexelloss commented Mar 26, 2021

icexelloss left a comment

Choose a reason for hiding this comment

jreback commented Mar 26, 2021