Repository: pandas-dev/pandas
Pull request: Prevent Unlimited Agg Recursion with Duplicate Col Names
- [X] closes #21063
- [X] tests added / passed
- [X] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

The `_gotitem` implementation for `DataFrame` seems a little strange so there may be a more comprehensive approach, but this should prevent the issue for the time being

Didn't add whatsnew yet since none existed for 0.23.1. Happy to add if we are OK with this fix

Problem statement:
segfault in DataFrame.apply with duplicate column names and multiple aggfuncs
```python
In [1]: import pandas as pd

In [2]: df = pd.DataFrame([['a', 'b'], ['c', 'd']], columns=['a', 'a'])

In [3]: df.agg(['min'])
Fatal Python error: Cannot recover from stack overflow.
```

This was present at least in 0.22.0. Haven't looked further.

Hints from issue discussion:

Commit message: Added type annotations to internal method

Patch to review:
diff --git a/pandas/core/base.py b/pandas/core/base.py
index fa78c89ed4ee7..aa051c6f5eaef 100644
--- a/pandas/core/base.py
+++ b/pandas/core/base.py
@@ -590,9 +590,10 @@ def _aggregate_multiple_funcs(self, arg, _level, _axis):
 
         # multiples
         else:
-            for col in obj:
+            for index, col in enumerate(obj):
                 try:
-                    colg = self._gotitem(col, ndim=1, subset=obj[col])
+                    colg = self._gotitem(col, ndim=1,
+                                         subset=obj.iloc[:, index])
                     results.append(colg.aggregate(arg))
                     keys.append(col)
                 except (TypeError, DataError):
@@ -675,7 +676,6 @@ def _gotitem(self, key, ndim, subset=None):
         subset : object, default None
             subset to act on
         """
-
         # create a new object to prevent aliasing
         if subset is None:
             subset = self.obj
diff --git a/pandas/core/frame.py b/pandas/core/frame.py
index dccc840f5affd..30af23407308b 100644
--- a/pandas/core/frame.py
+++ b/pandas/core/frame.py
@@ -5731,7 +5731,12 @@ def diff(self, periods=1, axis=0):
     # ----------------------------------------------------------------------
     # Function application
 
-    def _gotitem(self, key, ndim, subset=None):
+    def _gotitem(self,
+                 key,           # type: Union[str, List[str]]
+                 ndim,          # type: int
+                 subset=None    # type: Union[Series, DataFrame, None]
+                 ):
+        # type: (...) -> Union[Series, DataFrame]
         """
         sub-classes to define
         return a sliced object
@@ -5746,9 +5751,11 @@ def _gotitem(self, key, ndim, subset=None):
         """
         if subset is None:
             subset = self
+        elif subset.ndim == 1:
+            subset = self._constructor(subset)
 
         # TODO: _shallow_copy(subset)?
-        return self[key]
+        return subset[key]
 
     _agg_doc = dedent("""
     The aggregation operations are always performed over an axis, either the
diff --git a/pandas/tests/frame/test_apply.py b/pandas/tests/frame/test_apply.py
index ac46f02d00773..dfb2961befe35 100644
--- a/pandas/tests/frame/test_apply.py
+++ b/pandas/tests/frame/test_apply.py
@@ -554,6 +554,14 @@ def test_apply_non_numpy_dtype(self):
         result = df.apply(lambda x: x)
         assert_frame_equal(result, df)
 
+    def test_apply_dup_names_multi_agg(self):
+        # GH 21063
+        df = pd.DataFrame([[0, 1], [2, 3]], columns=['a', 'a'])
+        expected = pd.DataFrame([[0, 1]], columns=['a', 'a'], index=['min'])
+        result = df.agg(['min'])
+
+        tm.assert_frame_equal(result, expected)
+
 
 class TestInferOutputShape(object):
     # the user has supplied an opaque UDF where


Code review comments:
diff_hunk: @@ -5746,9 +5751,11 @@ def _gotitem(self, key, ndim, subset=None):
         """
         if subset is None:
             subset = self
+        elif subset.ndim == 1:
path: pandas/core/frame.py
text: @user1:
is this line actually hit in tests? return self._constructor here doesn't make sense

@author:
Yes this is actually hit by the test case and code that was in place. A Series is a valid value for the subset parameter so I’m forcing it to a DataFrame or else the subsequent slice would fail. All for a better way if you think there’s one

@user1:
what operation actually hits this code?

@author:
This line (which was changed to prevent the unlimited calls):

https://github.com/WillAyd/pandas/blob/4a24f734047d387ce242c36dba16eb69388a3ca1/pandas/core/base.py#L595

This would pass a Series before (unless column names were duplicated), though it never raised an error because of the code in `_got_item`. It would accept the Series and even use it in a condition, but would always just return a subset of itself...

Definitely convoluted - I think it was inadvertent before the way `_got_item` was implemented by DataFrame

@user1:
no the issue is why you are wrapping it with ``self._constructor`` which is a ``DataFrame`` here, then you are selecting it out again, just do

``return subset`` i think is enough?

@author:
Hmm you're probably right with that - I suppose could just return immediately if `ndim == 1`. Will try locally and push if it works