Repository navigation
fix: name the missing column in column-not-found errors and test on Python 3.14 - #306
Conversation
…ython 3.14 Column-name lookups in `column()` and `update_table` let `list.index()`'s `ValueError` escape, so the message was whatever CPython produced. Python 3.14 changed it from `'x' is not in list` to `list.index(x): x not in list`, which drops the column name. Both sites now raise their own `ValueError` naming the missing column and listing the available ones. `column()` now matches only top-level field names, so a name that exists only as a nested struct field gets the same error instead of a second bare `list.index` failure. Python 3.14 is already allowed by `requires-python`, so add it to the CI test matrix. Fixes substrait-io#304
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 7 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check The description references issue #304, and the implementation addresses its requested error handling and Python 3.14 CI coverage.Out of Scope Changes check The changes are limited to column validation, related tests, and adding Python 3.14 to the test matrix. They align with the stated objectives. Linked Issues check Issue #304requires explicit missing-column errors at thecolumn()andupdate_tablelookup sites, with the missing name and available names, plus Python 3.14 in CI. The reviewed changes implement …Out of Scope Changes check The changes stay within issue #304. The source changes implement the required error behavior, the workflow change tests the supported Python version, and the test changes verify the new errors and upd…Title check The title clearly summarizes the missing-column error change and Python 3.14 CI coverage. It uses a valid Conventional Commit format. Description check The description provides a clear rationale, explains the affected lookup behavior, documents related fixes, and identifies the linked issue. It satisfies the available template requirements.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 7 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
@coderabbitai resume |
|
…t-found # Conflicts: # tests/builders/plan/test_lateral_join.py
andrew-coleman
left a comment
andrew-coleman
left a comment
There was a problem hiding this comment.
Two of the lookup changes break cases that work on main. If a struct-valued column with a short names list, or any map column, is followed by another column, every by-name column() lookup now raises IndexError. Details are inline at extended_expression.py:585 and :588. The other comments are smaller gaps in the same lookups, and the update_table key handling and the out-of-range indexes in column() were already there before this PR.
A few more that couldn't go inline, because the lines aren't in the diff:
rename / drop. DataFrame.rename (frame.py:252) and drop (frame.py:271) call col(n) for every name in infer_plan_schema(...).names, and that list includes nested field names. Both already failed on main for any frame with a struct column. With this PR, the error points at a field the user never mentioned: df.drop("b") raises column 'x' not found; available columns: ['a', 's', 'b']. The unknown-name checks also accept nested names, so drop("x") passes validation. The top_level_names helper suggested on frame.py:1035 fixes both:
names = top_level_names(infer_plan_schema(bound, registry=registry))CONTRIBUTING.md:78 still says CI tests "across Python 3.10–3.13". With 3.14 added to test.yml, it should say 3.10–3.14.
PR description. It ends with 🤖 Generated with AI. The description becomes the squash-merge commit body, and CONTRIBUTING.md:114 asks to keep tool-attribution lines out of it, so that line should be dropped before merging.
| @@ -585,8 +585,12 @@ def resolve( | |||
| flat_indices = [0] + list(itertools.accumulate(lengths))[:-1] | |||
There was a problem hiding this comment.
Map columns now break lookups even in a well-formed schema. type_num_names counts a map as key names plus value names, so map<string, i64> counts as 2. The NamedStruct docs say only struct fields carry names, so it should count as 1. With names=["a", "m", "b"] and types [i64, map<string, i64>, i64], the offsets come out as [0, 1, 3] and every lookup raises IndexError. On main, a worked and m returned the wrong output_names. The count was already wrong, but this PR makes it fatal, and the guard suggested on line 588 doesn't cover it (b would be reported missing). The fix belongs at src/substrait/utils/__init__.py:22:
elif kind == "map":
# The map field's own name, plus any struct field names in its key/value.
return type_num_names(typ.map.key) + type_num_names(typ.map.value) - 1There was a problem hiding this comment.
Fixed in b8f16f5 with your type_num_names change: a map now counts as its own name plus any struct field names in its key/value. test_column_by_name_after_a_map_column covers lookups before and after a map column, and test_type_num_names_map covers the count.
| if isinstance(field, str): | ||
| column_index = list(base_schema.names).index(field) | ||
| field_index = flat_indices.index(column_index) | ||
| top_level = [base_schema.names[i] for i in flat_indices] |
There was a problem hiding this comment.
This line reads names at every top-level offset before it checks for a match. If the names list is shorter than the depth-first count, every lookup now fails with a bare IndexError, even for names that exist. For example, read_named_table("t", {"a": i64, "s": struct([i64(), i64()]), "b": i64}).select(col("a")) builds on main but raises here. The dict path producing only ["a", "s", "b"] is a separate bug, but main still resolved a and s. Skipping offsets past the end of the list keeps the new error:
| top_level = [base_schema.names[i] for i in flat_indices] | |
| names = base_schema.names | |
| top_level = [names[i] for i in flat_indices if i < len(names)] |
There was a problem hiding this comment.
Fixed in b8f16f5. column() now uses the new top_level_names helper, which skips offsets past the end of the names list. test_select_by_name_with_a_short_names_list covers the dict-schema case.
| else: | ||
| field_index = field |
There was a problem hiding this comment.
Also pre-existing: lookups by position have the same gap. column(-1) silently emits struct_field.field = -1 with empty output_names, and column(2) on a two-column schema raises a bare IndexError.
| else: | |
| field_index = field | |
| else: | |
| num_columns = len(base_schema.struct.types) | |
| if isinstance(field, bool) or not 0 <= field < num_columns: | |
| raise ValueError( | |
| f"column {field!r} out of range; schema has {num_columns} columns" | |
| ) | |
| field_index = field |
There was a problem hiding this comment.
Applied in b8f16f5: negative, out-of-range and bool ordinals now raise ValueError. Covered by test_column_by_ordinal_out_of_range_raises.
| @@ -1035,6 +1035,11 @@ def update_table( | |||
| names_list = list(ns.names) | |||
There was a problem hiding this comment.
update_table still matches names against the full depth-first list, while column() now matches top-level names only. Take names ["a", "s", "x", "y", "b"], where s is a struct of x and y. {"b": ...} emits column_target=4 when it should be 2. {"x": ...} passes the new check and emits column_target=2, which silently updates b. I suggest a small helper in substrait/utils, next to type_num_names. rename and drop can use it too (see the review body).
def top_level_names(ns: stp.NamedStruct) -> list[str]:
"""The names of ``ns``'s top-level fields, skipping the nested struct field
names that ``ns.names`` interleaves in depth-first order."""
names, offset = [], 0
for typ in ns.struct.types:
if offset < len(ns.names):
names.append(ns.names[offset])
offset += type_num_names(typ)
return names| names_list = list(ns.names) | |
| names_list = top_level_names(ns) |
The helper also needs adding to the from substrait.utils import ... line.
There was a problem hiding this comment.
Added top_level_names to substrait/utils as suggested, and used it in update_table, rename and drop (b8f16f5). test_update_table_targets_top_level_columns checks that b targets 2, and test_drop_and_rename_over_a_struct_column covers rename/drop over a struct column.
| if isinstance(target, str) and target not in names_list: | ||
| raise ValueError( | ||
| f"update_table got unknown column {target!r}; " | ||
| f"available columns: {names_list}" | ||
| ) | ||
| index = target if isinstance(target, int) else names_list.index(target) |
There was a problem hiding this comment.
This predates the PR, but the new guard only covers str keys. Keys like 1.0 or ("a",) still reach names_list.index() and produce the raw list.index(x): x not in list message this PR is replacing. int keys have no range check, so 99, -1 and True silently emit column_target values of 99, -1 and 1. Giving each accepted key type its own branch, with everything else falling through to the error, covers both cases:
| if isinstance(target, str) and target not in names_list: | |
| raise ValueError( | |
| f"update_table got unknown column {target!r}; " | |
| f"available columns: {names_list}" | |
| ) | |
| index = target if isinstance(target, int) else names_list.index(target) | |
| if isinstance(target, str) and target in names_list: | |
| index = names_list.index(target) | |
| elif ( | |
| isinstance(target, int) | |
| and not isinstance(target, bool) | |
| and 0 <= target < len(ns.struct.types) | |
| ): | |
| index = target | |
| else: | |
| raise ValueError( | |
| f"update_table got unknown column {target!r}; " | |
| f"available columns: {names_list}" | |
| ) |
There was a problem hiding this comment.
Applied in b8f16f5: each accepted key type now has its own branch, and anything else raises the update_table got unknown column error. test_update_table_rejects_nested_and_out_of_range_targets covers "x", 3, -1, True and 1.0.
| match=rf"column '{name}' not found; available columns: " | ||
| r"\['order_id', 'shop_details', 'order_total'\]", | ||
| ): | ||
| column(name)(nested_named_struct, None) |
There was a problem hiding this comment.
On an empty schema flat_indices is still [0], so column("x") now raises IndexError where main raised ValueError. The guard suggested at extended_expression.py:588 fixes this, and a test would keep it fixed:
| column(name)(nested_named_struct, None) | |
| column(name)(nested_named_struct, None) | |
| def test_column_unknown_name_on_empty_schema_raises(): | |
| empty = stt.NamedStruct(names=[], struct=stt.Type.Struct(types=[])) | |
| with pytest.raises(ValueError, match=r"column 'x' not found"): | |
| column("x")(empty, None) |
There was a problem hiding this comment.
Added this test in b8f16f5. It passes now that top_level_names skips past the end of the names list.
Address review feedback on the column-not-found change. `type_num_names` counted a map as its key names plus its value names, so `map<string, i64>` took two slots. Per the `NamedStruct` docs only struct fields carry names, so it now counts the map's own name plus any struct field names in its key and value. Without this, every by-name lookup after a map column failed. Add `top_level_names` and use it in `column()`, `update_table`, `rename` and `drop`. It also tolerates a names list shorter than the depth-first count, so lookups on such schemas and on an empty schema raise the new `ValueError` rather than `IndexError`. `update_table` previously matched names against the depth-first list, so a nested field name silently targeted the wrong column and a later top-level name got the wrong ordinal. Ordinal lookups are now range-checked: `column()` and `update_table` reject negative, out-of-range and `bool` indexes, and `update_table` rejects keys of any other type.
|
Thanks @andrew-coleman, all addressed in b8f16f5:
One thing I left out of this PR because it is already broken on |
|
Filed the |
2 participants
Column-name lookups in
column()andupdate_tableletlist.index()'sValueErrorescape, so the message was whatever CPython produced. Python 3.14 changed it from'x' is not in listtolist.index(x): x not in list, which drops the column name. Lookups now raise their ownValueErrornaming the missing column and listing the available ones.Name lookups in
column(),update_table,renameanddropnow match only top-level fields, via a newtop_level_nameshelper. Previouslyupdate_tablematched against the depth-first names list, so a nested field name silently targeted the wrong column and later top-level names got the wrong ordinal, andrename/dropfailed on any frame with a struct column. The helper also tolerates a names list shorter than the depth-first count, as the dict schema form produces.type_num_namescountedmap<string, i64>as two names. Per theNamedStructdocs only struct fields carry names, so a map now counts as its own name plus any struct field names in its key and value. This fixes by-name lookups after a map column.Ordinal lookups are range-checked:
column()andupdate_tablereject negative, out-of-range andboolindexes instead of emitting them or raisingIndexError.Python 3.14 is already allowed by
requires-python, so add it to the CI test matrix.Fixes #304