Repository: astropy/astropy

Issue:
Column names not preserved when initializing Table from a list of Rows
The following code:

```python
import numpy as np
from astropy.table import Table

t1 = Table()
t1['a'] = [1,2,3]
t1['b'] = [2.,3.,4.]

print(t1)

rows = []
for row in t1:
    rows.append(row)

t2 = Table(rows=rows)

print(t2)
```

illustrates that column names are lost:

```
 a   b 
--- ---
  1 2.0
  2 3.0
  3 4.0
col0 col1
---- ----
   1  2.0
   2  3.0
   3  4.0
```

Of course, this isn't the recommended way of getting a Table subset, but I think that it's nevertheless a bug?

Discussion:
github-actions[bot]: Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  - [ ] Do the proposed changes actually accomplish desired goals?
  - [ ] Do the proposed changes follow the [Astropy coding guidelines](https://docs.astropy.org/en/latest/development/codeguide.html)?
  - [ ] Are tests added/updated as required? If so, do they follow the [Astropy testing guidelines](https://docs.astropy.org/en/latest/development/testguide.html)?
  - [ ] Are docs added/updated as required? If so, do they follow the [Astropy documentation guidelines](https://docs.astropy.org/en/latest/development/docguide.html#astropy-documentation-rules-and-guidelines)?
  - [ ] Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for [rebase](https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#rebase-if-necessary) and [squash](https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#squash-if-necessary).
  - [ ] Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the [bot](https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#pre-commit).
  - [ ] Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  - [ ] Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  - [ ] Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  - [ ] At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) *before* merge.
github-actions[bot]: 👋 Thank you for your draft pull request! Do you know that you can use `[ci skip]` or `[skip ci]` in your commit messages to skip running continuous integration tests until you are ready?
neutrinoceros: There's one failure I didn't catch locally. I'll get back to fix it in the morning, most likely.
hamogu: We should backport.
hamogu: Since I don't review many PRs, I don't know what I need tp do about that though. Set the backport label?
pllim: Change the milestone and set the backport label, yes. I already did it here. Thanks for the feedback!
taldcroft: @neutrinoceros - I did a little digging and noticed a solution that has a much smaller footprint, in fact it reduces the line count. Basically this relies on `Row` being mostly dict-like, though sadly `list(row)` gives the values not the keys. (If we could go back 11 years...).

This also fixes some old inelegant code for getting the column order which was written way before Python had an ordered dict. It's very easy now to get the column order as simply the order in which they appear in successive rows. The previous alphabetical sort was not very nice.

One slight unfortunate thing about this solution is that it could change the column ordering in code so it would count as an API change for the unusual circumstance where the first row does not have all the columns. So I think this would need to wait for the next release, but the original issue is 6 years old so it can wait a little longer.
```
diff --git a/astropy/table/table.py b/astropy/table/table.py
index ac5a06d791..65631d9b0a 100644
--- a/astropy/table/table.py
+++ b/astropy/table/table.py
@@ -205,9 +205,9 @@ def _get_names_from_list_of_dict(rows):
     if rows is None:
         return None
 
-    names = set()
+    names = dict()
     for row in rows:
-        if not isinstance(row, Mapping):
+        if not isinstance(row, (Mapping, Row)):
             return None
         names.update(row)
     return list(names)
@@ -1169,17 +1169,7 @@ class Table:
         MISSING = object()
 
         # Gather column names that exist in the input `data`.
-        names_from_data = set()
-        for row in data:
-            names_from_data.update(row)
-
-        if set(data[0].keys()) == names_from_data:
-            names_from_data = list(data[0].keys())
-        else:
-            names_from_data = sorted(names_from_data)
-
-        # Note: if set(data[0].keys()) != names_from_data, this will give an
-        # exception later, so NO need to catch here.
+        names_from_data = _get_names_from_list_of_dict(data)
 
         # Convert list of dict into dict of list (cols), keep track of missing
         # indexes and put in MISSING placeholders in the `cols` lists.
```

neutrinoceros: @taldcroft thank you so much, I agree your solution is much more elegant and it still passes the test, so I went ahead and replaced mine with it (with you as a "co-author") !
taldcroft: @neutrinoceros - the sphinx doctests are failing which highlights the API change I mentioned. So there are a couple of actions:
- Update the sphinx narrative docs to pass.
- Add a `15735.api.rst` file which describes the API change for the case of `list[Mapping | Row]` input data where the first row does not have all the columns.
- Add a unit test to catch this case, this update should have been failing unit tests originally!

I've changed the milestone to 6.1 and added the `API` label.
neutrinoceros: Thanks @taldcroft , I think I've addressed all your points !
saimn: Hmm API changes should go in 7.0 (with a deprecation warning if possible), not 6.1...
pllim: As per Simon's recommendation, I moved milestone to 7.0, added label to request What's New entry, and turned this into draft so we don't merge before v6.1.
pllim: Re: https://github.com/astropy/astropy/pull/15735#issuecomment-1863471443

@saimn , I have not been super careful about API change and v6.1. Do we have to revert anything in https://github.com/astropy/astropy/pulls?q=is%3Apr+is%3Amerged+label%3A%22API+change%22+milestone%3Av6.1.0
hamogu: I think pushing this to 7.0 is going too far. See https://github.com/astropy/astropy/pull/15735#discussion_r1427881233
This was always intended to work -in my view  that fact that it did not is a bug that's fixed here. In that sense it's similar to https://github.com/astropy/astropy/pull/15774 - it breaks stuff if you relied on undocumented, uncodified behavior that the was not intended. In the same vain, I don't think a what'-s new is justified for this bug fix.

@taldcroft ?
neutrinoceros: >  it breaks stuff if you relied on undocumented, uncodified behavior that the was not intended.

@hamogu  not exactly: the behaviour that's changed here *was* documented, even though it wasn't ideal (and it's a side effect from @taldcroft's patch that I adopted, not it's primary goal)
In that sense, I agree that it makes sense to push it to 6.1 in this kind of minor breakage was acceptable (it sounds like they're not) or 7.0 otherwise.

My own patch was backward compatible, albeit a lot less elegant than @taldcroft , so I think I can propose a way forward that should be satisfactory to all parties:
- reinstate my own backward-compatible patch that *just* fixes the targeted bug (including a regression test) and move back this PR to 6.0.1
- open a separate PR with @taldcroft's cleanup patch, aimed at 7.0

@saimn I am not sure how to go about adding a deprecation warning in this case: ideally users should have some sort of control to opt-in the new behaviour before it becomes the default, so they're able to *dodge* the warning once they are aware of it. In this case in do not see how to provide such an interface without adding a flag to `Table.__init__`'s signature, but the flag itself would then need to be deprecated too. All in all, is it really worth it ? I think there's a case to be had that a clean (documented !)  break in 7.0 would help a lot with maintainability.
saimn: @hamogu - From what I understand, and @neutrinoceros confirms it above, there is both a bugfix (supporting list of Row) and an API change (ordering of the columns, which affects also instantiating from a list of dict). If the second part can be avoided then the bugfix can even go to 6.0.1, and maybe the API change can be delayed to 7.0, with a warning if possible.

@neutrinoceros - Not sure for the warning, it also depends if it makes sense to have an option to keep the old behavior. One option is to issue the warning when columns are sorted, stating that the default behavior will change in 7.0, and people can filter the warning if they want. Or add an option to choose the sort behavior which would allow to force the old behavior if it make sense to do so ?
neutrinoceros: > Or add an option to choose the sort behavior which would allow to force the old behavior if it make sense to do so ?

What's great about @taldcroft's patch is that it decreases complexity. If we were to support multiple behaviours then complexity would go back up again, and I'm honestly not sure it's worth it.
taldcroft: This is exactly the sort of situation I was thinking about in discussions for APE-21 which resulted in the statement about being pragmatic. **Historically, API changes like this have been common in minor releases**. Although we justified them by pointing to the LTS, in practice the project decided that very few people were actually using the LTS.

Breakage could only occur for a pretty unusual situation:
1. Initializing a table from a list of dict where the first row does not contain all of the columns.
2. Downstream code that depends on the column ordering.

The pragmatic part is accepting that this situation can occur but it is unlikely. Not only does (1) need to be true, but it needs to always be true for all input data sets. Basically the current code is somewhat randomly changing the column ordering based on an arbitrary test.

 It is true that we could test for situation (1), issue a warning and sort the columns alphabetically. But then users in that situation need to modify their code anyway to suppress the warning. And if they want to permanently keep alphabetical ordering they have to add code to do that sorting so it still works after 7.0, and then they are left with orphaned code to suppress the warning. Users that want the more natural ordering now would be out of luck unless we add some new configuration option. 

This is just a LOT of complexity for a very little gain.
hamogu: Like #15774 this sounds like space-bar heating: https://xkcd.com/1172/. I don't think we have to backport, but I suggest a 6.1 milestone. We also don't have to do the extra work of breaking this up into two PRs.
taldcroft: In my day job (Chandra Flight Director dealing directly with NASA) I deal with process control authority, A LOT. In this case, there is a delegation of authority:
- CoCo has ultimate authority for the release contents.
- CoCo does not have time to deal with each PR, so they delegate to the release managers.
- Release managers do not have time or (in many cases) the domain knowledge to assess each PR, so they delegate to the package maintainers.
- Package maintainers normally have authority to make a decision about the impact of an API change and the target milestone.

Only in unusual circumstances would the release managers or the CoCo override the recommendation of the package maintainer(s). Of course a healthy discussion is always encouraged!

saimn: Not sure I understand where this discussion is going but, sure in the end it's up to the maintainers to decide. 
But on the other side the change of philosophy of the release cycle is recent and it's easy to forget (e.g. #14977) that we decided to avoid breaking changes in minor versions. It's sometimes difficult to avoid, or sometimes justified to break something in order to fix a bug, fine. 
Here the first version was backward compatible, and creating a Table from a list of dict is pretty common I guess, so it seems reasonable to ask if we can find another way to proceed. 
neutrinoceros: It's not up to me to decide and ultimately I'm happy to re-iterate as long as it takes to get everyone happy but I think that going with the patch as is and aiming it as 6.1 is a reasonable way forward and indeed falls into the "pragmatic" kind of low impact breakage that APE 21 defines as acceptable for a minor release.
taldcroft: @saimn - agreed to try avoiding API changes, I'm definitely 100% behind not breaking code. That said, in this case I believe the complexity that is needed to maintain perfect back-compatibility outweighs the potential impact. Creating Tables from list of dict is common, but having both (1) and (2) from above is not.
neutrinoceros: rebased to resolve a merge conflict
neutrinoceros: rebased again to resolve conflicts. This time I also squashed my commits to minimize the number of conflicts I need to resolve in a single rebase.
github-actions[bot]:  Hi humans :wave: - this pull request hasn't had any new commits for approximately 4 months. **I plan to close this in 30 days if the pull request doesn't have any new commits by then.**

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add **keep-open** label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

*If you believe I commented on this pull request incorrectly, please report this [here](https://github.com/pllim/action-astropy-stalebot/issues).*
neutrinoceros: rebased again to repel the bot.
taldcroft: @neutrinoceros - getting really close but RTD is failing, ugh. Apart from that I think this is approvable.
neutrinoceros: Thanks ! I included suggestions from @pllim and rebased again 🤞🏻 
pllim: I restarted devdeps and mpldev, in case failures were transient. 🤞 
neutrinoceros: @taldcroft last try before feature freeze ? 🤞🏻 
taldcroft: Sorry, I'll have a look shortly.

Patch:
diff --git a/astropy/table/table.py b/astropy/table/table.py
index 8895bd33a0..a5338f1024 100644
--- a/astropy/table/table.py
+++ b/astropy/table/table.py
@@ -209,9 +209,9 @@ def _get_names_from_list_of_dict(rows):
     if rows is None:
         return None
 
-    names = set()
+    names = {}
     for row in rows:
-        if not isinstance(row, Mapping):
+        if not isinstance(row, (Mapping, Row)):
             return None
         names.update(row)
     return list(names)
@@ -1189,17 +1189,7 @@ class Table:
         MISSING = object()
 
         # Gather column names that exist in the input `data`.
-        names_from_data = set()
-        for row in data:
-            names_from_data.update(row)
-
-        if set(data[0].keys()) == names_from_data:
-            names_from_data = list(data[0].keys())
-        else:
-            names_from_data = sorted(names_from_data)
-
-        # Note: if set(data[0].keys()) != names_from_data, this will give an
-        # exception later, so NO need to catch here.
+        names_from_data = _get_names_from_list_of_dict(data)
 
         # Convert list of dict into dict of list (cols), keep track of missing
         # indexes and put in MISSING placeholders in the `cols` lists.
diff --git a/docs/changes/table/15735.bugfix.rst b/docs/changes/table/15735.bugfix.rst
new file mode 100644
index 0000000000..5c83d2f273
--- /dev/null
+++ b/docs/changes/table/15735.bugfix.rst
@@ -0,0 +1,1 @@
+Fix a bug where column names would be lost when instantiating ``Table`` from a list of ``Row`` objects.
diff --git a/docs/table/construct_table.rst b/docs/table/construct_table.rst
index fcf238b4d5..04baa5bae7 100644
--- a/docs/table/construct_table.rst
+++ b/docs/table/construct_table.rst
@@ -258,26 +258,17 @@ You can specify the column order with the ``names`` argument::
    10   5
    20  15
 
-If ``names`` are not provided then column ordering will be determined by the
-first :class:`dict` if it contains values for all the columns, or by sorting
-the column names alphabetically if it doesn't::
+If ``names`` are not provided then column ordering will be determined by
+order in which they appear as the :class:`list` of :class:`dict` is iterated over.
 
-  >>> data = [{'b': 10, 'c': 7, 'a': 5},
+  >>> data = [{'b': 10, 'c': 7, },
   ...         {'a': 15, 'c': 35, 'b': 20}]
   >>> t = Table(rows=data)
   >>> print(t)
    b   c   a
   --- --- ---
-   10   7   5
+   10   7  --
    20  35  15
-  >>> data = [{'b': 10, 'c': 7, },
-  ...         {'a': 15, 'c': 35, 'b': 20}]
-  >>> t = Table(rows=data)
-  >>> print(t)
-   a   b   c
-  --- --- ---
-   --  10   7
-   15  20  35
 
 **Single row**
 
diff --git a/docs/whatsnew/7.0.rst b/docs/whatsnew/7.0.rst
index 1a4c47bf9c..5348992eaf 100644
--- a/docs/whatsnew/7.0.rst
+++ b/docs/whatsnew/7.0.rst
@@ -15,6 +15,7 @@ In particular, this release includes:
 * :ref:`whatsnew-7.0-table-masked-quantity`
 * :ref:`whatsnew-7.0-table-frames`
 * :ref:`whatsnew-7.0-table-notebook-backends`
+* :ref:`whatsnew-7.0-table-init-columns-order`
 * :ref:`whatsnew_7_0_quantity_to_string_formatter`
 * :ref:`whatsnew_7_0_numpy_constructors_like_quantity`
 * :ref:`whatsnew_7_0_ecsv_meta_default_dict`
@@ -77,6 +78,43 @@ backends for rendering Astropy tables in Jupyter notebooks.
    :width: 450px
    :alt: Animated DataGrid usage example from ipydatagrid
 
+.. _whatsnew-7.0-table-init-columns-order:
+
+Ordering of table columns constructed from rows
+===============================================
+
+The column order in a `~astropy.table.Table` constructed from a `list` or rows (`dict`
+or `~astropy.table.Row`) may change using ``astropy`` version ``7.0`` *if the
+first row has missing values*.
+
+Before ``7.0``, the column ordering was determined from the first row if it contained
+values for all the columns, or by sorting the final column names alphabetically if it
+did not. Starting with ``7.0``, columns are always added in the order they appear
+when iterating over  the `list` of rows.
+
+For example, create a table as shown below:
+
+    >>> from astropy.table import Table
+    >>> data = [{'b': 10, 'c': 7, },
+    ...         {'a': 15, 'c': 35, 'b': 20}]
+    >>> t = Table(data)  # or Table(rows=data), which is equivalent
+
+
+Before ``7.0`` the table would look like this::
+
+     a   b   c
+    --- --- ---
+     --  10   7
+     15  20  35
+
+
+Starting with ``7.0`` the table would instead look like this::
+
+     b   c   a
+    --- --- ---
+     10   7  --
+     20  35  15
+
 .. _whatsnew_7_0_quantity_to_string_formatter:
 
 ``Quantity.to_string`` supports ``formatter`` for formatting
@@ -317,6 +355,7 @@ now supports version 1.5 of the VOTable standard.  The main new feature is that
 At this writing, version 1.5 is a proposed standard, but it is expected to be approved as an
 official recommendation soon.
 
+
 Full change log
 ===============
 


Test patch:
diff --git a/astropy/table/tests/test_init_table.py b/astropy/table/tests/test_init_table.py
index 413fdf820a..c4aa058df2 100644
--- a/astropy/table/tests/test_init_table.py
+++ b/astropy/table/tests/test_init_table.py
@@ -197,6 +197,7 @@ class TestInitFromListOfDicts(BaseInitFromListLike):
     def _setup(self, table_type):
         self.data = [{"a": 1, "b": 2, "c": 3}, {"a": 3, "b": 4, "c": 5}]
         self.data_ragged = [{"a": 1, "b": 2}, {"a": 2, "c": 4}]
+        self.data_acb = [{"a": 2, "c": 4}, {"a": 1, "b": 2}]
 
     def test_names(self, table_type):
         self._setup(table_type)
@@ -208,6 +209,14 @@ class TestInitFromListOfDicts(BaseInitFromListLike):
         t = table_type(self.data, names=("c", "b", "a"))
         assert t.colnames == ["c", "b", "a"]
 
+    def test_rows_without_names_args(self, table_type):
+        # see https://github.com/astropy/astropy/pull/15735
+        self._setup(table_type)
+        t1 = table_type(rows=self.data)
+        assert t1.colnames == ["a", "b", "c"]
+        t2 = table_type(rows=self.data_acb)
+        assert t2.colnames == ["a", "c", "b"]
+
     def test_missing_data_init_from_dict(self, table_type):
         self._setup(table_type)
         dat = self.data_ragged
@@ -252,6 +261,7 @@ class TestInitFromListOfMapping(TestInitFromListOfDicts):
     def _setup(self, table_type):
         self.data = [DictLike(a=1, b=2, c=3), DictLike(a=3, b=4, c=5)]
         self.data_ragged = [DictLike(a=1, b=2), DictLike(a=2, c=4)]
+        self.data_acb = [DictLike(a=2, c=4), DictLike(a=1, b=2)]
         # Make sure data rows are not a dict subclass
         assert not isinstance(self.data[0], dict)
 
diff --git a/astropy/table/tests/test_table.py b/astropy/table/tests/test_table.py
index 3c34f65b7a..fe883df593 100644
--- a/astropy/table/tests/test_table.py
+++ b/astropy/table/tests/test_table.py
@@ -1784,6 +1784,17 @@ def test_rows_equal():
     )
 
 
+def test_table_from_rows():
+    # see https://github.com/astropy/astropy/issues/5923
+    t1 = Table()
+    t1["a"] = [1, 2, 3]
+    t1["b"] = [2.0, 3.0, 4.0]
+
+    rows = [row for row in t1]  # noqa: C416
+    t2 = Table(rows=rows)
+    assert_array_equal(t2.colnames, t1.colnames)
+
+
 def test_equality_masked():
     t = table.Table.read(
         [