Add an ARRAY<string> column to one_row_complex through its definition only - #877
laughingman7743 wants to merge 1 commit into
Conversation
| pa.struct([("a", pa.int32()), ("b", pa.int32())]), | ||
| ), | ||
| Column("col_decimal", "DECIMAL(10,1)", pa.decimal128(10, 1)), | ||
| Column("col_array_string", "ARRAY<string>", pa.list_(pa.string())), |
There was a problem hiding this comment.
Self-review round one (implementation behavior): CLEAN
Base 3202ab5 (#876 head, the stacked base), head bcbf874. The full diff is 2 lines.
- Generation:
Table.data_filewrites["a", "b"]aslist<string>and the new round-trip check accepts it. The DDL gainscol_array_string ARRAY<string>. - Derived tests: all 19 whole-row tests pick the column up through
Selection. The PolarsCursor tests leave it out throughCOMPLEX_FAMILIES. - Other readers of the table:
- The tests that select explicit
one_row_complexcolumns are unaffected. meta.reflect()over the schema (test_get_table_names/test_get_view_names) reflects the new array type.- The Glue-vs-Athena and throttled-metadata comparisons list the table with the new column on both sides.
- The tests that select explicit
- AWS results: 104 passed for the derived-expectation, reflection, and
one_row_complexcolumn tests, and 43 passed for the Glue, throttling, and table-metadata tests.
| [(1, 2), (3, 4)], | ||
| {"a": 1, "b": 2}, | ||
| Decimal("0.1"), | ||
| ["a", "b"], |
There was a problem hiding this comment.
Self-review round two (claims, callers, AWS operations): CLEAN
Claims checked against the AWS run on this head
- Each row of the description's table is a derived expectation that passed on AWS:
["a", "b"]for the Python-object cursors and SQLAlchemy"[a, b]"for the CSV pathspa.list_(pa.field("array_element", pa.string()))/pl.List(pl.String)for Arrow UNLOADobjectdtype for pandas UNLOAD
- "Two lines, both in the table definition": the diff touches only
tests/pyathena/tables.py. - "Four places before": the gzipped Hive text row, the Jinja DDL, the hand-written expectations in about 20 tests, and
len(one_row_complex.c) == 16. All four existed before Define the shared test tables in Python and generate their data #874.
Operational effect: none beyond one more column in one Parquet file. The number of setup statements and uploads is unchanged.
CI scope: no SQLAlchemy or Spark path changes, so on Ready the PyAthena suite runs without Spark.
| pa.struct([("a", pa.int32()), ("b", pa.int32())]), | ||
| ), | ||
| Column("col_decimal", "DECIMAL(10,1)", pa.decimal128(10, 1)), | ||
| Column("col_array_string", "ARRAY<string>", pa.list_(pa.string())), |
There was a problem hiding this comment.
Independent review (relayed Codex result): CLEAN
Reviewer: Codex CLI 0.157.1 (codex exec -s read-only), model gpt-6-astra, session 01a0e7b6-1295-7331-87c3-e21bcaa43fd8.
Scope: base 3202ab5, head bcbf874. The review ran on a detached snapshot without .env, and the prompt left out the PR framing. The snapshot was still clean at the head afterwards. This was a static review only.
Covered, as reported:
- DDL, Parquet generation, and the Arrow round-trip guard.
- All consumers under
tests/, including metadata, Glue, reflection, and schema listings, plus assertions that depend on column count or position. - The derived rows, descriptions, pandas dtypes, Arrow schemas, Polars dtypes, and SQLAlchemy array element types.
- The default, S3FS, pandas, Arrow, and Polars converter and result-set paths, including UNLOAD.
Result: "No broken or newly vacuous assertions found. The new value's expectations match the applicable cursor paths, and ["a", "b"] is losslessly representable by pa.list_(pa.string())."
3202ab5 to
9e350b7
Compare
bcbf874 to
cbd12b1
Compare
9e350b7 to
7845405
Compare
cbd12b1 to
632d0c5
Compare
7845405 to
68270a4
Compare
632d0c5 to
d301d77
Compare
The column is added only to the table definition. The session creates it, and the tests that assert whole one_row_complex rows, schemas, dtypes, descriptions, and reflected types derive its expectations for every cursor type from the existing rules. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d301d77 to
c3b08bb
Compare
|
Closing without merging, by the maintainer's decision. Deriving the expected results from the table definitions required tests/pyathena/expected.py to re-model how each cursor returns every type: Athena's text rendering, JSON parsing, and the pandas, Arrow, and Polars types. The module grew large enough to need tests of its own, which is more complexity than the one-place column addition is worth. The hand-written expectations stay. See #848 for the summary. The branch is kept for reference. |
WHAT
This PR adds
col_array_string ARRAY<string>, with the value["a", "b"], toONE_ROW_COMPLEXintests/pyathena/tables.py. The diff is two lines, both in the table definition: the column and its value. No test or rule changes.WHY
Part of #848, for #834. Stacked on #876.
#848's validation plan asks to "show that adding a column to a shared table is a one-place change". Before #874, #875, and #876, adding this column meant editing four places:
one_row_complex.gz, which uses\002separators;After this change, the tests below cover the new column in every representation.
col_array_stringTestCursor.test_complex, S3FS ×2,pandas.util.as_pandas["a", "b"]: the cursors parse Athena's rendering[a, b]into stringsTestSQLAlchemyAthena.test_reflect_selectAthenaArraywithStringitems; value["a", "b"]"[a, b]"as a pandas string column["a", "b"], with dtypeobjectas_arrow,as_polars)"[a, b]",pa.string()/pl.String["a", "b"], withpa.list_(pa.field("array_element", pa.string()))/pl.List(pl.String)TEST
Tested commit: bcbf874, then rebased onto the simplified #875/#876 as cbd12b1 (tested again), then onto later #875/#876 heads; current head c3b08bb. The diff is unchanged.
just lintpassed.pytest -n 4ontests/pyathena/test_cursor.py, both s3fs cursor test files,pandas/test_util.py,sqlalchemy/test_base.py, and the pandas, arrow, and polarstest_cursor.pyfiles, with-k "complex or as_pandas or reflect or table_names or view_names or like or escape or char_length or cast_as_varbinary or filter": 104 passed on bcbf874 and again on cbd12b1. This covers every test that derivesone_row_complexexpectations, plus the other tests that readone_row_complexor list the schema's tables.pytest -n 4 tests/pyathena/test_glue.py tests/pyathena/test_cursor.py tests/pyathena/aio/test_cursor.py tests/pyathena/sqlalchemy/test_base.py tests/pyathena/aio/sqlalchemy/test_base.py -k "glue or throttl or list_table_metadata or get_table_metadata": 43 passed. These tests compare the schema-wide Glue and Athena metadata, which now include the new column.🤖 Generated with Claude Code