BUG: Fix analytic ops over ungrouped and unordered windows on Pandas backend #2376

timothydijamco · 2020-09-11T02:26:41Z

Overview

Using analytic ops (like lead, lag, first, last, or analytic UDFs) over a window where no group_by and no order_by are defined produces an incorrect result.

Example

Analytic op lag over an ungrouped+unordered window

import pandas as pd
import ibis

client = ibis.pandas.connect({'df': pd.DataFrame({'id': [1, 2, 3], 'value_col': [16, 92, 51]})})
table = client.table('df')

window = ibis.window()
expr = table.value_col.lag().over(window)  # lag operation over window
df = expr.execute()

df.to_markdown()

Before this PR

The result of the analytic op is a Series. That Series result is strangely copied n times, where n is the number of rows of the original column.

	0
0	0 NaN
	1 16.0
	2 92.0
	Name: value_col, dtype: float64
1	0 NaN
	1 16.0
	2 92.0
	Name: value_col, dtype: float64
2	0 NaN
	1 16 .0
	2 92.0
	Name: value_col, dtype: float64

After this PR

The Series result of the analytic op is treated as the end result of using the analytic op over the window.

	value_col
0	nan
1	16
2	92

As additional verification that this is the correct result: the above table is what we get when we use the exact same code but with window = ibis.window(order_by=[table.id]) (both before and after this PR).

Testing

New tests in test_window.py to exercise the four kinds of windows:

Window with group_by and order_by
Window with group_by only
Window with order_by only
Window with neither group_by nor order_by

New test in test_vectorized_udf.py to test using an analytic op and adding the result to a table using mutate. This implicitly executes the analytic op over a window. (Also a few additional general UDF tests for completeness)

…backend

timothydijamco · 2020-09-11T21:12:40Z

Going to need to investigate a bit into why the tests are failing on the Spark backend

timothydijamco · 2020-09-14T15:02:53Z

There are some differences in how backends view a window spec like this:

ibis.window(order_by=order_by)

Backends that treat this as a completely unbounded window (applies the function over ALL rows in the table):

Pandas
PySpark
Postgres
MySQL
SQLite
Clickhouse

Backends that treats this as an expanding window (apply the function over all rows up until the current row):

Spark
Impala

This is sort of orthogonal to this PR (this is hit by a test I added in this PR for completeness but not for testing the change being done) so this could be addressed in a separate PR.

But to confirm, behavior across backends should be consistent regardless of how the backend itself treats windows in its non-Ibis API? If not, (if we want to accept the difference) I can just change the tests here to reflect the difference.

ibis/tests/all/test_vectorized_udf.py

icexelloss · 2020-09-14T15:19:56Z

Can you please add a release note?

icexelloss

Looks pretty good. Some minor comments.

timothydijamco · 2020-09-14T15:42:39Z

About the behavior difference I mentioned above: Created #2378 to track making ungrouped, ordered windows consistent across backends. I will x-fail that test for Impala and Spark for this PR.

timothydijamco · 2020-09-14T17:44:15Z

Still have a couple of tests I'm still troubleshooting: lag and lead over empty window spec on Spark backend leads to results that don't match other backends

I need to confirm this is a problem with the Spark backend and not something that should be changed with the test (strangely, Spark inherits from Impala for this, yet Impala seems to pass for these tests)

…a for Impala backend

timothydijamco · 2020-09-14T21:07:49Z

I think I know the reason behind the failing tests on the Spark backend, but I haven't been able to figure out why the Impala backend doesn't fail in the same way, given that the Spark backend inherits from the Impala backend. I'm going to stop investigating further since this is orthogonal to the main part of the PR anyways. I opened #2381 to look into this further

icexelloss

LGTM. +1

jreback · 2020-09-15T19:44:47Z

ibis/pandas/execution/window.py

@@ -30,12 +30,19 @@
 from ibis.pandas.execution import util


-def _post_process_empty(scalar, parent, order_by, group_by):
+def _post_process_empty(result, parent, order_by, group_by):


can you type these args (and return type of this function)

jreback · 2020-09-15T19:45:53Z

ibis/pandas/execution/window.py

-    result.index = index
-    return result
+    if isinstance(result, pd.Series):
+        # `result` is a Series e.g. when an analytic operation is being


can you elabort when this happens (e.g. what casuses this result to be a Series vs a scalar). Is this better handled in a dispatch rule though? e.g. to differentiate these cases.

I'll make the comments a little clearer

I think if all the _post_process_xxxx functions could be made into a dispatch rule that would be pretty nice, but some of them exactly share the same types of input parameters

In terms of a dispatch rule for just _post_process_empty, I don't think it'd be any clearer because I think the dispatch rule for the case that result is a scalar would have to be @_post_process_empty.register(Any), which doesn't clearly indicate that that is the scalar case

OK this ^ has been done

ibis/tests/all/test_vectorized_udf.py

jreback · 2020-09-15T19:49:42Z

ibis/tests/all/test_window.py

@@ -323,25 +329,120 @@ def test_bounded_preceding_windows(backend, alltypes, df, con, window_fn):
        ),
    ],
 )
+@pytest.mark.parametrize(
+    ('ordered'), [param(True, id='orderered'), param(False, id='unordered')],


are these ids different for a reason? why are you using param here?

I think this actually doesn't need param or ids. I'll change it to just [True, False]

Actually this makes the test output a bit nicer, are you OK with keeping this as is?

# Without ids XFAIL ibis/tests/all/test_window.py::test_unbounded_window_grouped[Pandas-True-mean]

vs

# With ids XFAIL ibis/tests/all/test_window.py::test_unbounded_window_grouped[Pandas-orderered-mean]

ibis/tests/all/test_window.py

jreback · 2020-09-16T00:26:47Z

thanks @timothydijamco

Timothy Dijamco added 3 commits September 11, 2020 01:57

BUG: Fix analytic ops over ungrouped and unordered windows on Pandas …

c7e542c

…backend

Skip and xfail tests as needed

7374f98

Not sure why I xfailed SQLite

3745c31

jreback added pandas The pandas backend window functions Issues or PRs related to window functions labels Sep 12, 2020

jreback added this to the Next Bugfix Release milestone Sep 12, 2020

icexelloss reviewed Sep 14, 2020

View reviewed changes

ibis/tests/all/test_vectorized_udf.py Show resolved Hide resolved

icexelloss suggested changes Sep 14, 2020

View reviewed changes

timothydijamco mentioned this pull request Sep 14, 2020

feat: make behavior of ibis.window(order_by=xxxx) consistent across backends #2378

Closed

Timothy Dijamco added 3 commits September 14, 2020 17:38

Xfail ungrouped, ordered window tests on Spark and Impala backends

7b38995

Add release note

4407906

Merge branch 'master' into fix-pandas-analytic-ops-window

b2b25ee

Timothy Dijamco added 2 commits September 14, 2020 20:06

Temporarily add an assert to check whether test actually receives dat…

3ca2c5c

…a for Impala backend

Remove temporary assert

5c497b4

Xfail empty window spec test on Spark backend

04d9cbe

icexelloss approved these changes Sep 15, 2020

View reviewed changes

jreback suggested changes Sep 15, 2020

View reviewed changes

Typing and comment improvements

ffc9245

timothydijamco requested a review from jreback September 15, 2020 22:50

jreback approved these changes Sep 15, 2020

View reviewed changes

Empty

926119c

jreback merged commit 21565e3 into ibis-project:master Sep 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix analytic ops over ungrouped and unordered windows on Pandas backend #2376

BUG: Fix analytic ops over ungrouped and unordered windows on Pandas backend #2376

timothydijamco commented Sep 11, 2020

timothydijamco commented Sep 11, 2020

timothydijamco commented Sep 14, 2020

icexelloss commented Sep 14, 2020

icexelloss left a comment

timothydijamco commented Sep 14, 2020 •

edited

Loading

timothydijamco commented Sep 14, 2020

timothydijamco commented Sep 14, 2020

icexelloss left a comment

jreback Sep 15, 2020

timothydijamco Sep 15, 2020

jreback Sep 15, 2020

timothydijamco Sep 15, 2020

timothydijamco Sep 15, 2020

jreback Sep 15, 2020

timothydijamco Sep 15, 2020

timothydijamco Sep 15, 2020

jreback commented Sep 16, 2020

BUG: Fix analytic ops over ungrouped and unordered windows on Pandas backend #2376

BUG: Fix analytic ops over ungrouped and unordered windows on Pandas backend #2376

Conversation

timothydijamco commented Sep 11, 2020

Overview

Example

Before this PR

After this PR

Testing

timothydijamco commented Sep 11, 2020

timothydijamco commented Sep 14, 2020

icexelloss commented Sep 14, 2020

icexelloss left a comment

Choose a reason for hiding this comment

timothydijamco commented Sep 14, 2020 • edited Loading

timothydijamco commented Sep 14, 2020

timothydijamco commented Sep 14, 2020

icexelloss 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

jreback commented Sep 16, 2020

timothydijamco commented Sep 14, 2020 •

edited

Loading