FEAT: Add time context in `scope` in execution for pandas backend #2306

LeeTZ · 2020-08-03T12:40:03Z

What is the change

This PR purpose a change to the data structure scope in pandas execution. Time context is added to scope and getting, setting scope methods are changed accordingly.

Existing mechanism

Scope key is the op associated with each expression.
Scope value is a pd.DataFrame or pd.Series that is the result of executing key op.
Set scope kv pair: This is done in multiple places:
- in execute_until_in_scope, new_scope is a combination of scopes of all children(calculating recursively), and scope from pre_execute.
- in window.py window has a similar log of setting scope kv pair.
- in selection.py selction is adding additional_scope to scope by remapping columns to data. See compute_projection_scalar_expr, compute_projection_column_expr etc.
  
  Although this kv is set in multiple places by different code pieces, conceptually, it's just scope[op] = result.
Get scope kv pair:
- This is done in execute_until_in_scope using data = [ new_scope[arg.op()] if hasattr(arg, 'op') else arg for arg in computable_args ] that retrives computed data from scope, and pass on to execute_node implementation.
- Also, there are shortcuts across execute_util_in_scope that return an op without execution if op is already in scope.
Scope miss: If there is a miss in getting scope[op], we will execute op.

Proposed mechanism

Scope key is still op associated with each expression.
Scope value is another key-value map
- value: pd.DataFrame or pd.Series that is the result of executing key op, this is same as the original scope value.
- timecontext: of type TimeContext, the time context associated with the data stored in value

Note that the idea is: data should be determined by both op and timecontext. So the data structure above is conceptually same as making (op, timecontext) as the key for scope. But that may increase the time complexity in querying, so we make timecontext as another key of op. See following getting and setting logic for details.
Set scope kv pair: before setting the value op in scope we need to perform the following check first:
- Test if op is in scope yet
  - No, then put op in scope, set timecontext to be the current timecontext (None if timecontext is not present), set value to be the DataFrame or Series of the actual data.
  - Yes, then get the timecontext stored in scope for op as old_timecontext, and compare it with current timecontext:
    - If curernt timecontext is same as old_timecontext, do nothing.
    - If current timecontext is a subset of old_timecontext, that means we already cache a larger range of data. Do nothing and we will trim data in later execution process.
    - If current timecontext is a superset of old_timecontext, that means we need to update cache. Set value to be the current data and set timecontext to be the current timecontext for op.
    - If current timecontext is neither a subset nor a superset of old_timcontext, If current time context is neither a subset nor a superset of old_timcontext, but they overlap, or not overlap at all. For example, this will happen when there is a window that looks forward, over a window that looks back. So in this case, we should not trust the data stored either, and go on to execute this node. For simplicity, we update the cache in this case as well.
Get scope kv pair: getting will change from scope[op] to get_scope(scope, op, timecontext) in get calls.
Scope miss: If there is a miss in getting scope[op], we will execute op to get the result.

Restrictions

Note that we have an important restriction here: For op with time context, we only use scope to cache leaf table nodes now. We may assume that if we cache a large time range that is the superset of all other time contexts, we could always use this cache and trim data to their associated time context later. But this assumption doesn't hold true for all cases. This is because some ops cannot use the cached results with a different time context to get the correct result. For example, a groupby followed by count, if we use a larger or smaller from the cache, we will probably get an error in the result. Such ops global aggregation, ops whose result is depending on other rows in result Dataframe, cannot use the cached result with different context to optimize calculation. For simplicity, we skip these ops for now and will address these in followups.

How is this change tested

Unit tests are added in ibis/pandas/tests/test_timecontext.py to test if time context comparison works. Also, concrete examples that use this feature like window in mutate and multi-window tests, are added as well. See inline comments for the explanation of each test case.

icexelloss · 2020-08-03T14:53:57Z

ibis/pandas/core.py

@@ -554,3 +610,113 @@ def canonicalize_context(
            f'begin time {begin} must be before or equal' f' to end time {end}'
        )
    return begin, end
+
+
+class TimeContextRelation(enum.Enum):


Perhaps move this to a timecontext module

Make sense, will move this to an upper level (ibis level) so that we could reuse this module for other backends

icexelloss · 2020-08-03T14:54:17Z

ibis/pandas/core.py

+    NONOVERLAP = 3
+
+
+def compare_timecontext(cur_context: TimeContext, old_context: TimeContext):


Move to a timecontext module?

+1, same as above.

icexelloss · 2020-08-03T14:56:06Z

ibis/pandas/core.py

+    if op not in scope:
+        return None
+    # for ops without timecontext
+    if timecontext is None:


For consistency, we should perhaps use an timecontext place holder for the timecontext is None case

Do you mean have an entry of {"value":None, "timecontext":None}?

icexelloss · 2020-08-03T15:02:49Z

ibis/pandas/execution/window.py

@@ -161,14 +161,17 @@ def trim_with_timecontext(data, timecontext: Optional[TimeContext]):
    if not timecontext:
        return data

-    df = data.reset_index()
+    df = data.reset_index(level=1)


Why this change?

OK this is tricky. Here, data is a Series of MultiIndex (int, Timestamp) where int is some index (0,1,2,3 ...) and Timestamp column is our time column. When we filter by time, the index is also filtered so we will have something like:

index time value 2 2010-01-02 2 3 2010-01-03 3

As you can see the index may not start from 0. And later if we put this Series back to generate result. There will be NaN and NaT filling in, since pandas will align rows according to index number.
Therefore, all changes in this function is trying to reindex the int inside the Multiindex after filtering, to guarantee the correctness of our result.

Ok, I think this needs to be explained in the code, otherwise it's kind of confusing.

Also, is it always the case that time is an index here? What if this is not a moving window, e.g.

win = window(group_by='id')

What's the behavior of this code in that case?

Or perhaps

win = window(preceding=interval(days=1), group_by='id', order_by='some_other_time_column')

?

Or

win = window(preceding=10, group_by='id', order_by='time')

In general, I think this function is not very clear about what the expected input and output. It would be helpful to clarify.

icexelloss · 2020-08-03T15:04:40Z

ibis/pandas/execution/window.py

+                data,
+                order_by,
+                group_by=group_by,
+                timecontext=new_timecontext,


Can we rename new_timecontext to adjusted_timecontext?

icexelloss · 2020-08-03T15:06:42Z

ibis/pandas/execution/window.py

-        factory=OrderedDict,
-    )
+    additional_scope = {}
+    for t in operand.op().root_tables():


Why changing the list comprehension to for loop?

There might be better ways but let me explain why, scope_item() now return with a dict {op: {"value":xx, "timecontext":xx}}, how can I turn it into (key, value) like (t, source) in a one-linere we had previously?

I think your code is the same as

toolz.merge( set_scope_item(t, source, adjusted_timecontext) for t in operand.op().root_tables() )

?

icexelloss · 2020-08-03T15:07:53Z

ibis/pandas/core.py

+        return TimeContextRelation.OVERLAP
+
+
+def scope_item(op, result, timecontext):


Can we lift scope related code to its own module so the pyspark backend can also reuse it?

Same as above, will move this to Ibis level

icexelloss · 2020-08-03T15:10:32Z

ibis/pandas/core.py

+        # in result Dataframe, cannot use cached result with different time
+        # context to optimize calculation. For simplicity, we skip these ops
+        # for now.
+        if isinstance(op, ops.TableColumn) or isinstance(op, ops.TableNode):


Can you add a xfail test for projection?

Per discussion offline. I think we should just enable this for all nodes instead of leaf nodes.

icexelloss · 2020-08-03T15:12:53Z

ibis/pandas/core.py

@@ -220,7 +264,7 @@ def execute_with_scope(
            **kwargs,
        ),
        **kwargs,
-    )[op]
+    )[op]['value']


What happens if the value here is for a context that is larger than the one that user requested?

Checked again, this seems unlikely to happen because execute_until_in_scope would return with a dict with op as the only key. This is not reading from the scope. Time context should be already taken care of in execution phases in execute_until_in_scope

I think execute_until_in_scope used to return the scope after execution. Is that still the case? If so, shouldn't the code here be:

result = get_scope_item(scope, op, timecontext)

?

Yes it makes sense, however, execute_until_in_scope is a recursive call and it always returns the executed result of the expr, instead of the whole scope. Before this change, the return value is just {op: computed}. So I think we don't need to call get method again here.

icexelloss · 2020-08-03T15:14:33Z

ibis/pandas/core.py

@@ -330,7 +380,7 @@ def execute_until_in_scope(

    # pass our computed arguments to this node's execute_node implementation
    data = [
-        new_scope[arg.op()] if hasattr(arg, 'op') else arg
+        get_scope(new_scope, arg.op()) if hasattr(arg, 'op') else arg


I think get_scope_item and set_scope_item is better. You basically want a get and set method to get/set item from scope with a op and timecontext

OK I will change these signature

icexelloss

High level looks good. Let's try to decouple timecontext scope and execution modules. I think timecontext and scope module can be reused by other backends as well.

icexelloss · 2020-08-05T17:37:02Z

ibis/pandas/core.py

@@ -343,7 +394,7 @@ def execute_until_in_scope(
        **kwargs,
    )
    computed = post_execute_(op, result, timecontext=timecontext)
-    return {op: computed}
+    return set_scope_item(op, computed, timecontext)


I think the first arg to set_scope_item should be scope

refactored and changed arg accrodingly

icexelloss · 2020-08-05T17:42:16Z

ibis/common/scope.py

+from ibis.timecontext.util import TimeContextRelation, compare_timecontext
+
+
+def set_scope_item(op, result, timecontext: Optional[TimeContext]):


This is not a "set" method. A set method should change the state of object and returns nothing.

This set method would make more sense to me

def set_scope_item(scope, op, timecontext, value): scope[op] = {'value': result, 'timecontext': timecontext}

I first name it as scope_item for the same reason. Now it is refactored and this is addressed.

ibis/pandas/core.py

icexelloss · 2020-08-05T18:51:54Z

ibis/pandas/core.py

+            timecontext,
+        )
+
+    if get_scope_item(scope, op, timecontext) is not None:


Why change the order to be after Literal check here?

Good catch, this is a bug.

icexelloss · 2020-08-05T18:53:34Z

ibis/pandas/core.py

@@ -330,7 +381,7 @@ def execute_until_in_scope(

    # pass our computed arguments to this node's execute_node implementation
    data = [
-        new_scope[arg.op()] if hasattr(arg, 'op') else arg
+        get_scope_item(new_scope, arg.op()) if hasattr(arg, 'op') else arg


Missing timecontext here when calling get_scope_item?

Yes, this is the same bug as the previous comment, op was directly executed and not add to op result in a None in getting time context. Will fix it

ibis/common/scope.py

icexelloss · 2020-08-05T18:58:52Z

ibis/timecontext/util.py

+TIME_COL = 'time'
+
+
+class TimeContextRelation(enum.Enum):


Perhaps call this ibis/expr/timecontext.py?

Sure, renamed the file.

icexelloss · 2020-08-05T19:02:28Z

ibis/timecontext/adjustment.py

+from ibis.expr.typing import TimeContext
+
+
+def adjust_context_asof_join(


We should probably define dispatch methods for extending.

@singledispatch def adjust_timecontext(op, timecontext): raise NotImplementedError() @adjust_timecontext.register(AsOfJoin): def adjust_timecontext_asof_join(op, timecontext): return ...

In order for this to work, I added an extra arg clients in compute_time_context dispatch, we will need to pass clients so that in these dispatch function at expr level, we could run execute on op args.

ibis/timecontext/util.py

icexelloss · 2020-08-05T19:15:01Z

ibis/timecontext/util.py

+
+    Returns
+    -------
+    result : Enum[TimeContextRelation]


I think the type for this is just TimeContextRelation?

Yes, fixed.

icexelloss

Reviewed another around. Left some comments.

jreback · 2020-08-31T19:55:35Z

ibis/expr/scope.py

+
+    `scope` in Scope class is the main cache. It is a dictionary mapping
+    ibis node instances to concrete data, and the time context associate
+    with it(if any).


can you update here

ibis/expr/scope.py

jreback · 2020-09-01T12:59:55Z

ibis/expr/scope.py

+    -------
+    Scope: a new Scope instance with op in it.
+    """
+    item = namedtuple('item', ['value', 'timecontext'])


this should be ScopeItem and created in the module (the definition)

at the top of this module, do
ScopeItem= namedtuple(....)

ibis/expr/timecontext.py

jreback

looks really good. just a small change in the impl of Scope and should be good

jreback · 2020-09-01T17:43:18Z

ibis/expr/scope.py

+    -------
+    Scope: a new Scope instance with op in it.
+    """
+    item = namedtuple('item', ['value', 'timecontext'])


at the top of this module, do
ScopeItem= namedtuple(....)

ibis/expr/scope.py

jreback · 2020-09-04T15:46:54Z

ibis/expr/scope.py

+
+class Scope:
+    def __init__(self, items: Dict[str, ScopeItem] = None):
+        self.items = items or {}


save this as self._items, then you can make a .items() method which return iter(self._items) and then Scope becomes more like a dictionary

ibis/expr/scope.py

jreback · 2020-09-04T15:48:53Z

ibis/expr/scope.py

+            a new Scope instance with items in two scope merged.
+        """
+        result = Scope()
+        for op, v in self._get_items().items():


if you can fix this as above

…dd_time_context_in_scope

jreback

one small additional comment, then lgtm. ping on green.

jreback · 2020-09-04T19:07:33Z

ibis/expr/scope.py

+        """
+        result = Scope()
+        for op in self.items():
+            result._items[op] = self._items[op]


on Scope I would define __getitem__(self, op) and __setitem__(self, op) as public methods (and then use them here)

def __getitem__(self, op): return self._items[op] def __setitem__(self, op, value): self._items[op] = value

jreback · 2020-09-04T19:55:49Z

ibis/expr/scope.py

+        result = Scope()
+
+        for op in self.items():
+            result.__setitem__(op, self.__getitem__(op))


use

result[op] = self[op]

jreback · 2020-09-04T19:56:03Z

ibis/expr/scope.py

+        """
+        result = Scope()
+        for op in self.items():
+            result.__setitem__(op, self.__getitem__(op))


result[op] = self[op]

jreback · 2020-09-04T19:56:33Z

ibis/expr/scope.py

+    def __getitem__(self, op):
+        return self._items[op]
+
+    def __setitem__(self, op, value):


this should return None, type value as Any, op is ScopeItem?

jreback · 2020-09-04T19:56:48Z

ibis/expr/scope.py

+    def items(self):
+        return iter(self._items)
+
+    def __getitem__(self, op):


op is ScopeItem? returns -> Any

What's the diff between __getitem__ and get?

__getitem__ returns a ScopeItem like {'value': 'foo', 'timecontext': None}
get is an api to return the value, which is foo in the case above.

Ok. But be careful this is exposing implementation details

ibis/expr/scope.py

icexelloss

Minor comments

ibis/expr/scope.py

jreback

ping on green

icexelloss

LGTM

jreback · 2020-09-04T21:56:11Z

@LeeTZ if you can add a release note. pls ping when green.

LeeTZ · 2020-09-04T23:17:24Z

Ping @jreback for green. Thank you all for reviewing!

jreback · 2020-09-04T23:20:51Z

awesome @LeeTZ very nice!

LeeTZ added 2 commits July 31, 2020 14:18

add timecontext into scope

b173690

add docs and test for time context comparison

9d1d459

LeeTZ changed the title ~~Add time context in scope in execution for pandas backend~~ FEAT: Add time context in scope in execution for pandas backend Aug 3, 2020

catch keyerror if value and timecontexts are missing

3848f1e

icexelloss reviewed Aug 3, 2020

View reviewed changes

icexelloss suggested changes Aug 3, 2020

View reviewed changes

refactor time context and scope to be higher level modules

b0e6349

icexelloss reviewed Aug 5, 2020

View reviewed changes

ibis/pandas/core.py Show resolved Hide resolved

icexelloss reviewed Aug 5, 2020

View reviewed changes

ibis/common/scope.py Outdated Show resolved Hide resolved

icexelloss reviewed Aug 5, 2020

View reviewed changes

ibis/timecontext/util.py Outdated Show resolved Hide resolved

icexelloss reviewed Aug 5, 2020

View reviewed changes

icexelloss suggested changes Aug 5, 2020

View reviewed changes

LeeTZ added 3 commits August 7, 2020 04:43

add dispatch for context adjustment, refactor time context module

c55cbfa

refactor set_scope_item

79a0735

clean comments

5a079b2

LeeTZ reopened this Aug 27, 2020

LeeTZ added 3 commits August 27, 2020 06:54

fix time context adjustment function for multiple backends

e34167b

remove clients from compute_time_context call

554cb0a

fix docstring

e37353d

jreback suggested changes Sep 1, 2020

View reviewed changes

LeeTZ added 3 commits September 4, 2020 10:05

fix merge scope to return a new value

42993fd

type output Scope

cab66f6

handle overwrite correctly

f1da3bb

jreback suggested changes Sep 4, 2020

View reviewed changes

add item() method for Scope

d706fd1

LeeTZ force-pushed the tiezheng/add_time_context_in_scope branch from d706fd1 to f9e2b46 Compare September 4, 2020 16:03

Merge branch 'master' of github.com:ibis-project/ibis into tiezheng/a…

8b83630

…dd_time_context_in_scope

LeeTZ force-pushed the tiezheng/add_time_context_in_scope branch from f9e2b46 to 8b83630 Compare September 4, 2020 16:11

jreback suggested changes Sep 4, 2020

View reviewed changes

implement get / set item for scope

1e8e867

jreback suggested changes Sep 4, 2020

View reviewed changes

type & fix set/get item in scope

26ed276

icexelloss reviewed Sep 4, 2020

View reviewed changes

ibis/expr/scope.py Outdated Show resolved Hide resolved

icexelloss suggested changes Sep 4, 2020

View reviewed changes

jreback reviewed Sep 4, 2020

View reviewed changes

ibis/expr/scope.py Show resolved Hide resolved

rename get -> get_name in scope

d231039

jreback approved these changes Sep 4, 2020

View reviewed changes

icexelloss approved these changes Sep 4, 2020

View reviewed changes

jreback added this to the Next Feature Release milestone Sep 4, 2020

add release note

76b0c9d

jreback merged commit 816f521 into ibis-project:master Sep 4, 2020

LeeTZ mentioned this pull request Sep 22, 2020

FEAT: use Scope class for scope in pyspark backend #2402

Merged

		NONOVERLAP = 3


		def compare_timecontext(cur_context: TimeContext, old_context: TimeContext):

		return TimeContextRelation.OVERLAP


		def scope_item(op, result, timecontext):

		from ibis.timecontext.util import TimeContextRelation, compare_timecontext


		def set_scope_item(op, result, timecontext: Optional[TimeContext]):

		from ibis.expr.typing import TimeContext


		def adjust_context_asof_join(

FEAT: Add time context in scope in execution for pandas backend #2306

FEAT: Add time context in scope in execution for pandas backend #2306

Conversation

LeeTZ commented Aug 3, 2020

What is the change

Existing mechanism

Proposed mechanism

Restrictions

How is this change tested

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

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 Aug 3, 2020 • edited Loading

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

Choose a reason for hiding this comment

icexelloss Aug 5, 2020 • 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

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

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

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

icexelloss left a comment

Choose a reason for hiding this comment

jreback left a comment

FEAT: Add time context in `scope` in execution for pandas backend #2306

FEAT: Add time context in `scope` in execution for pandas backend #2306

icexelloss Aug 3, 2020 •

edited

Loading

icexelloss Aug 5, 2020 •

edited

Loading