Repository: astropy/astropy

Issue:
Incorrect result when slicing a row by multiple columns
In the following example:

```
In [1]: from astropy.table import Table

In [2]: t = Table()

In [3]: t['a'] = [1,2,3]

In [4]: t['b'] = [3,4,5]

In [5]: row = t[0]

In [6]: row['a', 'b']
Out[6]: 
<Column name='a' dtype='int64' length=3>
1
2
3
```

I would either expect an error for ``row['a','b']`` or I would expect the same as:

```
In [7]: t['a', 'b'][0]
Out[7]: 
<Row index=0>
  a     b  
int64 int64
----- -----
    1     3
```

Discussion:
astropy-bot[bot]: Hi there @taldcroft :wave: - thanks for the pull request! I'm just a friendly :robot: that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part :smiley:.

I noticed the following issue with this pull request:

* Changelog entry not present (or pull request number missing) and neither the **Affects-dev** nor the **no-changelog-entry-needed** label are set

Would it be possible to fix this? Thanks!

*If there are any issues with this message, please report them [here](https://github.com/astropy/astropy-bot/issues).*
taldcroft: @eteq @adrn - there is a weird failure in test_transform_to_nonscalar_nodata_frame() for appveyor, which should be unrelated to this PR.  Thoughts?
taldcroft: Nevermind, presumably my test fail is related to #7105.
taldcroft: @mhvk - I reworked this a fair bit based on your comments and I think it is better now.  I added rollback of the row update if anything goes wrong, which then removed the need for up-front testing of the column names.  Along the way I discovered that `t[1] = values` was not transactional and could do a halfway update, but now this is and Row.__setitem__ are handled with the same code.  Yay.

There is an API change in the exception that gets raised, as seen in test changes.  Probably OK?
astrofrog: > There is an API change in the exception that gets raised, as seen in test changes. Probably OK?

Yes, but please mention it in the changelog
taldcroft: Just realized with an API change this should not really be in a bug fix release (3.0.1).  What do you think?
mhvk: Can we just put it in 3.0 still? It does fix a bug.
bsipocz: Hmm. We have rc2 out, but I'm inclined that this is still fine as a bugfix to get into the final 3.0 release. I would be happier to do that than 3.0.1, as it would also solve the KeyError API change dilemma.
Any veto for that @eteq or @astrofrog (or anyone else)?


taldcroft: Assuming that others agree we can get this into 3.0, I have updated the change log accordingly (and please review since I added a bunch).  Also changed the milestone and squashed to one commit.
taldcroft: BTW, not to shoot myself in the foot, I'll say that I am somewhat ambivalent about pushing this into 3.0 without a proper RC, esp. since this involves an API change.
taldcroft: But of course it's up to release managers.  :smile:
bsipocz: Ouch. Well, I assumed it'll be fine as the change in the type of exception is rather a minor API change, but your previous comment made me ambivalent, too. 
But, if you think it shouldn't go into 3.0 without RC testing, then it cannot really go into 3.0.1 either for the same API change reason.
astrofrog: Can we not decouple the exception type change from the rest of the fixes?
mhvk: I find it *very* hard to believe the type of exception raised for multiple column names was relied on anywhere, while this does fix a rather annoying bug. Since I'm also not that sure `rc` gets as much testing as a proper release, which means in a way 3.0.0 is the true "release candidate", I'm +1 for going for 3.0.0.
taldcroft: It is not easy to decouple the exception change because of the way the code logic is updated and cleaned up.

I do agree with @mhvk that this is very unlikely to be a problem.  It's just that we did get burned once on a late addition to the release and made a resolution to not do that again.  Hence the ambivalence.  

I *guess* one way to look at this is say the only utility in making rc3 and putting it out for a week is to give downstream codes an extra week to get fixed, or complain.  Very worst case (highly unlikely) is a quick 3.0.1 that addresses a problem.
bsipocz: I'm fine with doing a quick 3.0.1 if needed. Also, many of the affiliates tests against the dev version, we may just check those out to be sure before the backport.
astrofrog: @taldcroft - can you rebase to fix the changelog conflict?

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 3b4300034d..3b5ab48eec 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -478,6 +478,7 @@ astropy.table
 - Allow updating of table by indices through the property ``astropy.table.Table.loc``. [#6831]
 
 - Enable tab-completion for column names with IPython 5 and later. [#7071]
+- Allow getting and setting a table Row using multiple column names. [#7107]
 
 astropy.tests
 ^^^^^^^^^^^^^
@@ -677,6 +678,13 @@ astropy.table
   unicode arrays (dtype ``U``). The ``Column`` class then ensures the
   bytes are automatically converted to string as needed. [#6821]
 
+- When getting a table row using multiple column names, if one of the
+  names is not a valid column name then a ``KeyError`` exception is
+  now raised (previously ``ValueError``).  When setting a table row,
+  if the right hand side is not a sequence with the correct length
+  then a ``ValueError`` is now raised (previously in certain cases
+  a ``TypeError`` was raised). [#7107]
+
 astropy.tests
 ^^^^^^^^^^^^^
 
@@ -779,6 +787,12 @@ astropy.stats
 astropy.table
 ^^^^^^^^^^^^^
 
+- Fix getting a table row when using multiple column names (for example
+  ``t[3]['a', 'b', 'c']``).  Also fix a problem when setting an entire row:
+  if setting one of the right-hand side values failed this could result in
+  a partial update of the referenced parent table before the exception is
+  raised. [#7107]
+
 astropy.tests
 ^^^^^^^^^^^^^
 
diff --git a/astropy/table/row.py b/astropy/table/row.py
index 9a0361847b..1aa2d6c5dd 100644
--- a/astropy/table/row.py
+++ b/astropy/table/row.py
@@ -38,10 +38,20 @@ class Row:
                              .format(index, len(table)))
 
     def __getitem__(self, item):
-        return self._table.columns[item][self._index]
+        if self._table._is_list_or_tuple_of_str(item):
+            cols = [self._table[name] for name in item]
+            out = self._table.__class__(cols, copy=False)[self._index]
+
+        else:
+            out = self._table.columns[item][self._index]
+
+        return out
 
     def __setitem__(self, item, val):
-        self._table.columns[item][self._index] = val
+        if self._table._is_list_or_tuple_of_str(item):
+            self._table._set_row(self._index, colnames=item, vals=val)
+        else:
+            self._table.columns[item][self._index] = val
 
     def _ipython_key_completions_(self):
         return self.colnames
diff --git a/astropy/table/table.py b/astropy/table/table.py
index b2d01999db..8237cdf2f0 100644
--- a/astropy/table/table.py
+++ b/astropy/table/table.py
@@ -1224,12 +1224,7 @@ class Table:
             return self.Row(self, item)
         elif (isinstance(item, np.ndarray) and item.shape == () and item.dtype.kind == 'i'):
             return self.Row(self, item.item())
-        elif (isinstance(item, (tuple, list)) and item and
-              all(isinstance(x, str) for x in item)):
-            bad_names = [x for x in item if x not in self.colnames]
-            if bad_names:
-                raise ValueError('Slice name(s) {0} not valid column name(s)'
-                                 .format(', '.join(bad_names)))
+        elif self._is_list_or_tuple_of_str(item):
             out = self.__class__([self[x] for x in item],
                                  meta=deepcopy(self.meta),
                                  copy_indices=self._copy_indices)
@@ -1323,16 +1318,7 @@ class Table:
                 self.columns[item][:] = value
 
             elif isinstance(item, (int, np.integer)):
-                # Set the corresponding row assuming value is an iterable.
-                if not hasattr(value, '__len__'):
-                    raise TypeError('Right side value must be iterable')
-
-                if len(value) != n_cols:
-                    raise ValueError('Right side value needs {0} elements (one for each column)'
-                                     .format(n_cols))
-
-                for col, val in zip(self.columns.values(), value):
-                    col[item] = val
+                self._set_row(idx=item, colnames=self.colnames, vals=value)
 
             elif (isinstance(item, slice) or
                   isinstance(item, np.ndarray) or
@@ -1441,6 +1427,12 @@ class Table:
     def colnames(self):
         return list(self.columns.keys())
 
+    @staticmethod
+    def _is_list_or_tuple_of_str(names):
+        """Check that ``names`` is a tuple or list of strings"""
+        return (isinstance(names, (tuple, list)) and names and
+                all(isinstance(x, str) for x in names))
+
     def keys(self):
         return list(self.columns.keys())
 
@@ -2165,6 +2157,27 @@ class Table:
 
         self.columns[name].info.name = new_name
 
+    def _set_row(self, idx, colnames, vals):
+        try:
+            assert len(vals) == len(colnames)
+        except Exception:
+            raise ValueError('right hand side must be a sequence of values with '
+                             'the same length as the number of selected columns')
+
+        # Keep track of original values before setting each column so that
+        # setting row can be transactional.
+        orig_vals = []
+        cols = self.columns
+        try:
+            for name, val in zip(colnames, vals):
+                orig_vals.append(cols[name][idx])
+                cols[name][idx] = val
+        except Exception:
+            # If anything went wrong first revert the row update then raise
+            for name, val in zip(colnames, orig_vals[:-1]):
+                cols[name][idx] = val
+            raise
+
     def add_row(self, vals=None, mask=None):
         """Add a new row to the end of the table.
 


Test patch:
diff --git a/astropy/table/tests/test_item_access.py b/astropy/table/tests/test_item_access.py
index d1f2081ed1..5af70551e4 100644
--- a/astropy/table/tests/test_item_access.py
+++ b/astropy/table/tests/test_item_access.py
@@ -220,13 +220,13 @@ class TestTableItems(BaseTestItems):
         """Selecting a column that doesn't exist fails"""
         self.t = table_data.Table(table_data.COLS)
 
-        with pytest.raises(ValueError) as err:
+        with pytest.raises(KeyError) as err:
             self.t[['xxxx']]
-        assert 'Slice name(s) xxxx not valid column name(s)' in str(err)
+        assert "KeyError: 'xxxx'" in str(err)
 
-        with pytest.raises(ValueError) as err:
+        with pytest.raises(KeyError) as err:
             self.t[['xxxx', 'yyyy']]
-        assert 'Slice name(s) xxxx, yyyy not valid column name(s)' in str(err)
+        assert "KeyError: 'xxxx'" in str(err)
 
     def test_np_where(self, table_data):
         """Select rows using output of np.where"""
diff --git a/astropy/table/tests/test_row.py b/astropy/table/tests/test_row.py
index 14bde373ca..8f3549eb7b 100644
--- a/astropy/table/tests/test_row.py
+++ b/astropy/table/tests/test_row.py
@@ -8,6 +8,7 @@ import numpy as np
 
 from ... import table
 from ...table import Row
+from ... import units as u
 from .conftest import MaskedTable
 
 
@@ -200,3 +201,81 @@ class TestRow():
         for ibad in (-5, -4, 3, 4):
             with pytest.raises(IndexError):
                 self.t[ibad]
+
+
+def test_row_tuple_column_slice():
+    """
+    Test getting and setting a row using a tuple or list of column names
+    """
+    t = table.QTable([[1, 2, 3] * u.m,
+                      [10., 20., 30.],
+                      [100., 200., 300.],
+                      ['x', 'y', 'z']], names=['a', 'b', 'c', 'd'])
+    # Get a row for index=1
+    r1 = t[1]
+    # Column slice with tuple of col names
+    r1_abc = r1['a', 'b', 'c']  # Row object for these cols
+    r1_abc_repr = ['<Row index=1>',
+                   '   a       b       c   ',
+                   '   m                   ',
+                   'float64 float64 float64',
+                   '------- ------- -------',
+                   '    2.0    20.0   200.0']
+    assert repr(r1_abc).splitlines() == r1_abc_repr
+
+    # Column slice with list of col names
+    r1_abc = r1[['a', 'b', 'c']]
+    assert repr(r1_abc).splitlines() == r1_abc_repr
+
+    # Make sure setting on a tuple or slice updates parent table and row
+    r1['c'] = 1000
+    r1['a', 'b'] = 1000 * u.cm, 100.
+    assert r1['a'] == 10 * u.m
+    assert r1['b'] == 100
+    assert t['a'][1] == 10 * u.m
+    assert t['b'][1] == 100.
+    assert t['c'][1] == 1000
+
+    # Same but using a list of column names instead of tuple
+    r1[['a', 'b']] = 2000 * u.cm, 200.
+    assert r1['a'] == 20 * u.m
+    assert r1['b'] == 200
+    assert t['a'][1] == 20 * u.m
+    assert t['b'][1] == 200.
+
+    # Set column slice of column slice
+    r1_abc['a', 'c'] = -1 * u.m, -10
+    assert t['a'][1] == -1 * u.m
+    assert t['b'][1] == 200.
+    assert t['c'][1] == -10.
+
+    # Bad column name
+    with pytest.raises(KeyError) as err:
+        t[1]['a', 'not_there']
+    assert "KeyError: 'not_there'" in str(err)
+
+    # Too many values
+    with pytest.raises(ValueError) as err:
+        t[1]['a', 'b'] = 1 * u.m, 2, 3
+    assert 'right hand side must be a sequence' in str(err)
+
+    # Something without a length
+    with pytest.raises(ValueError) as err:
+        t[1]['a', 'b'] = 1
+    assert 'right hand side must be a sequence' in str(err)
+
+
+def test_row_tuple_column_slice_transaction():
+    """
+    Test that setting a row that fails part way through does not
+    change the table at all.
+    """
+    t = table.QTable([[10., 20., 30.],
+                      [1, 2, 3] * u.m], names=['a', 'b'])
+    tc = t.copy()
+
+    # First one succeeds but second fails.
+    with pytest.raises(ValueError) as err:
+        t[1]['a', 'b'] = (-1, -1 * u.s)  # Bad unit
+    assert "'s' (time) and 'm' (length) are not convertible" in str(err)
+    assert t[1] == tc[1]
diff --git a/astropy/table/tests/test_table.py b/astropy/table/tests/test_table.py
index 967862b798..2ad6af96c2 100644
--- a/astropy/table/tests/test_table.py
+++ b/astropy/table/tests/test_table.py
@@ -111,7 +111,7 @@ class TestSetTableColumn(SetupData):
         t = table_types.Table([self.a, self.b])
         with pytest.raises(ValueError):
             t[1] = (20, 21, 22)
-        with pytest.raises(TypeError):
+        with pytest.raises(ValueError):
             t[1] = 0
 
     def test_set_row_fail_2(self, table_types):