Conversation
| ) | ||
| assert expected_type in ddl_string | ||
|
|
||
| def test_external_parquet_struct_columns_round_trip(self, engine): |
There was a problem hiding this comment.
This live test is the right way to prove the fix, but the PR has no record that it, or any other AWS check, has run. As requested in #855 and required by docs/contributing.md, please run the live CREATE TABLE and read-back in your own AWS account, on the commit before the fix and on the fixed commit, and record the results in TEST. Include the tested commit, the Python and dependency versions, the exact commands, the passed/failed/skipped counts, and whether each result came from real AWS. The current TEST section lists planned steps; it is not a validation record.
WHAT and WHY also contain the same text. WHY should point to #855 and the scope agreed there.
External-fork PRs are not run in the project's AWS CI.
| ) | ||
|
|
||
|
|
||
| def _delete_s3_prefix(location: str) -> None: |
There was a problem hiding this comment.
This helper, the boto3 import, and the try/finally cleanup in test_external_parquet_struct_columns_round_trip are not needed. Each test session creates its own schema, pytest_sessionfinish drops it with DROP DATABASE ... CASCADE, and the staging bucket expires objects after one day. The other tests in this file that call table.create(bind=conn), such as test_create_table_bucketing, rely on that and do no per-test cleanup. Please remove the helper and the cleanup block and follow the existing pattern.
| That includes top-level columns, fields of a STRUCT, STRUCT values inside MAP, and STRUCT values inside ARRAY. | ||
| Integer fields, and integer MAP keys and values, use `INT` in that DDL. | ||
| `CAST` and other SQL expressions keep `ROW(...)`, `MAP(...)`, and `ARRAY(...)`, and spell integers as `INTEGER`. | ||
| An empty `AthenaStruct()` column remains `ROW()`. |
There was a problem hiding this comment.
Please drop these two lines. The note about what earlier releases emitted is a migration note, and it belongs in the release notes, which the maintainers write. The user guide should describe the current behavior. The empty-AthenaStruct() sentence presents ROW() in a column definition as documented behavior, but it is not valid Athena CREATE TABLE syntax. That rendering is unchanged by this PR and does not need to be documented here.
| Returns: | ||
| True when the type should use Hive DDL syntax. | ||
| """ | ||
| if kw.get("_athena_array_ddl") or isinstance(kw.get("type_expression"), Column): |
There was a problem hiding this comment.
nit: _athena_array_ddl now also marks STRUCT and MAP column DDL, so its name no longer matches its meaning, and the new comment on visit_INTEGER is needed only to explain that. It is a private keyword used only in this module, so please rename it, for example to _athena_hive_ddl. Also, _hive_column_ddl reads like a predicate but mutates kw. A name that says it enables the mode would make that side effect visible at the call sites.
Rename the private _athena_array_ddl keyword to _athena_hive_ddl, since it now also covers STRUCT and MAP column DDL, and rename _hive_column_ddl to _enable_hive_column_ddl so the kw side effect is visible at call sites. Remove the S3 cleanup helper and try/finally from the live STRUCT round-trip test to follow the existing pattern, and drop the migration note and ROW() sentence from docs/sqlalchemy.md.
|
Pushed af04b14. Next I'll run the live |
| assert self._format_sql(stmt).endswith(f"WHERE x LIKE '{pattern}'") | ||
|
|
||
|
|
||
| class TestStructColumnDDL: |
There was a problem hiding this comment.
Please fold these tests into the existing TestAthenaDDLCompiler class below instead of adding a separate class. They exercise the same AthenaDDLCompiler through CreateTable(...).compile(...), so they belong with its other compile-only DDL tests. Move _ddl there as a helper next to _s3tables_dialect, and generalize the class docstring, which currently describes only the S3 Tables support, so that it covers column type rendering as well.
| ) | ||
| assert expected_type in ddl_string | ||
|
|
||
| def test_external_parquet_struct_columns_round_trip(self, engine): |
There was a problem hiding this comment.
I ran this against real Athena in the maintainer's AWS account on af04b14 (Python 3.13.1, SQLAlchemy 2.0.46, boto3/botocore 1.43.102).
just lint: passed (ruff check, ruff format --check, mypy).uv run --env-file .env pytest -n 1 tests/pyathena/sqlalchemy/test_compiler.py tests/pyathena/sqlalchemy/test_base.py -k "test_compiler or struct or map_types or complex_nested or array_types or create_table": 194 passed, 0 failed, 0 skipped. This includes this test's liveCREATE TABLE,INSERT, and read-back.- Before the fix: with
pyathena/sqlalchemy/compiler.pyreverted to the merge base (659676c) and this test's two DDL string assertions temporarily removed, Athena rejects the generatedprofile ROW(...)DDL atStartQueryExecutionwithInvalidRequestException: line 1:8: mismatched input 'EXTERNAL'.
So the fix is confirmed on real Athena, and the only remaining change is folding TestStructColumnDDL into TestAthenaDDLCompiler.
This branch has not been deployed
2 participants
WHAT
In compiler.py, recognize column DDL context from the existing
type_expression=columnpassed by AthenaDDLCompiler.get_column_specification and propagate the existing nested Hive rendering behavior through STRUCT fields and MAP children. Generalize or reuse the existing ARRAY context flag locally so STRUCT uses angle brackets, colon-separated fields, the DDL identifier preparer, and nested INT spelling at every depth, while preserving direct type compilation without column context and the separate AthenaStatementCompiler CAST renderer. Keep scalar column types and all float mappings unchanged; all runtime wiring is already in compiler.py, with no new helper solely for testing or dialect registration change.CREATE TABLE currently emits ROW(...) for top-level AthenaStruct columns and for STRUCT values inside MAP, although Athena requires Hive STRUCT<name:type, ...> syntax in table DDL. ARRAY already enables the correct nested rendering through
_athena_array_ddl, so the same struct gets different output depending on its parent. Existing compilation assertions in test_base.py enshrine the invalid output. The maintainer explicitly confirmed the STRUCT defect and narrowed the requested fix to it; the float proposal was rejected as documented, intentional behavior, so this plan fully covers the accepted scope without a partial-scope exemption.Fixes #855
WHY
In compiler.py, recognize column DDL context from the existing
type_expression=columnpassed by AthenaDDLCompiler.get_column_specification and propagate the existing nested Hive rendering behavior through STRUCT fields and MAP children. Generalize or reuse the existing ARRAY context flag locally so STRUCT uses angle brackets, colon-separated fields, the DDL identifier preparer, and nested INT spelling at every depth, while preserving direct type compilation without column context and the separate AthenaStatementCompiler CAST renderer. Keep scalar column types and all float mappings unchanged; all runtime wiring is already in compiler.py, with no new helper solely for testing or dialect registration change.TEST
Compile real CreateTable expressions with a top-level struct, nested structs, MAP-to-STRUCT values, MAP-to-MAP-to-STRUCT, STRUCT containing MAP/ARRAY, and ARRAY-to-MAP-to-STRUCT; assert Hive syntax throughout and retain the already-correct ARRAY behavior.
Exercise reserved words and field names requiring quoting, including embedded quote characters, using AthenaDDLIdentifierPreparer conventions; compare against CAST quoting separately.
Compile CAST expressions for the same STRUCT/MAP/ARRAY combinations before and after the fix and assert unchanged ROW(...), MAP(...), and ARRAY(...) expression syntax. Retain direct visitor tests without DDL context and existing fallback/unsupported-type error coverage; avoid inventing new empty-STRUCT semantics.
Retain Float/FLOAT/REAL versus Double expectations and cover Float precision in actual column DDL so the rejected float change cannot slip in. Verify unrelated scalar column compilation remains unchanged.
In test_base.py, extend the existing engine-fixture tests beyond compilation: create external Parquet tables containing top-level and MAP-nested structs, insert representative nested values, SELECT stored fields and assert their values, and clean up tables/data even on failure. Record the pre-fix CREATE failure and post-fix CREATE plus read-back results from the contributor's own AWS account.
Run just format, then just lint before pytest; run focused compiler tests and affected dialect tests, just test pyathena, and just test sqla. Because shared compilers also serve async dialects, run the relevant tests/pyathena/aio/sqlalchemy/ coverage and just test sqla-async. Follow docs/testing.md for environment setup even for targeted compiler tests; do not assume they need no AWS configuration.
Run just docs lint, just docs build, and the working-tree Sphinx build described in docs/testing.md. Report exact tested commit, dependency versions, commands, results, and skipped coverage in the PR template TEST section.
AI was used for assistance.