Leave unset storage values out of the Glue table parameters - #890
Conversation
Athena's GetTableMetadata and ListTableMetadata now leave the storage values a table does not have out of its parameters: a view reports no inputformat, outputformat, or serde.serialization.lib, and an Iceberg table in AwsDataCatalog no inputformat or outputformat. The Glue conversion still added them as None, so the Glue metadata no longer matched Athena's for views and Iceberg tables, and reflected table properties gained None values when the Glue fallback answered. Add each storage value only when Glue sets it. Empty strings, such as a view's location or an S3 Tables table's formats, are still reported. Closes #887 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| parameters.update( | ||
| {f"serde.param.{k}": v for k, v in (serde.get("Parameters") or {}).items()} | ||
| ) | ||
| parameters.update({k: v for k, v in storage.items() if v is not None}) |
There was a problem hiding this comment.
Self-review round one (implementation behavior): CLEAN
Scope: base a86a180ebbe5e18920cd75802ea5350ce8d33f10 .. head e86ec41c753ca9359e1de8e71d7cf29be64ac3b4, both changed files.
Covered:
- Callers:
table_metadatais used byGlueMetadataClient.get_tableandlist_tables. The sync cursor and the aio cursor (which offloads the same client to a thread) both reach it, so one change covers both paths. - SerDe handling: before,
serde.param.*entries were added onlyif "SerdeInfo" in descriptor. Nowdescriptor.get("SerdeInfo") or {}yields no entries whenSerdeInfois missing or empty, so SerDe parameters are unchanged. The library key is now left out whenSerializationLibraryis missing (a view'sSerdeInfo: {}), as Athena reports it. Noneversus empty string: the filter isis not None, soLocation: ''(views) andInputFormat: ''/OutputFormat: ''(S3 Tables) are kept, matching the measured Athena shapes.- Consumers:
AthenaTableMetadata.location/input_format/output_format/serde_serialization_libuse.get()and returnNoneeither way.table_properties(reflected asawsathena_tblproperties) loses theNoneentries, which Athena's own path never had. - Edge case: a Glue table-level parameter named
location/inputformat/… used to be overwritten by the descriptor value even when that wasNone. Now a table-level value survives when the descriptor has none. No measured table has such a parameter, and this is not a regression for the measured shapes.
Tests: the offline view and iceberg cases fail against the old code, and the new s3_tables case pins that empty strings are kept. The live tests compare against Athena directly (34 Glue-related tests passed locally).
Limitation: if Athena in another region, or after a rollback, still returns the None-valued keys, only those None entries in parameters would differ; the accessors return the same values.
| }, | ||
| { | ||
| "table_type": "ICEBERG", | ||
| "location": "s3://bucket--table-s3", |
There was a problem hiding this comment.
Self-review round two (claims, callers, operations): FINDINGS (PR description only, corrected)
Scope: base a86a180ebbe5e18920cd75802ea5350ce8d33f10 .. head e86ec41c753ca9359e1de8e71d7cf29be64ac3b4, the PR body, commit message, changed docstring, and the Glue fallback docs.
Claims checked:
- The measurement table in the PR body matches the probe output: 5
GetTableMetadatacalls per table, each returning a single shape; Hive tables equal; the view with onlylocation: ''; Iceberg inAwsDataCatalogwith onlylocation; S3 Tables withinputformat: ''/outputformat: ''and no SerDe library. - "The AWS suite failed on every branch": only Require pandas 3.0 and pyarrow 22.0 #885's CI and local runs at the heads of Drop Python 3.10 support #884 and Require pandas 3.0 and pyarrow 22.0 #885 were observed. The body now states that.
- "Answer throttled metadata requests from Glue in the cursor #803, unreleased":
git tag --containson the commit that addedpyathena/glue.pyis empty. - "The accessors use
.get()" (pyathena/model.pylocation/input_format/output_format/serde_serialization_lib) and "awsathena_tblpropertiescarries theNoneentries" (pyathena/sqlalchemy/base.pyreflectsmetadata.table_properties, which keeps every non-serde.param.key): both confirmed in the code. - The docstring now says values are added "whenever they are set (an empty string included)". This matches the
is not Nonefilter and thes3_tablesandviewtest cases.
Existing callers: parameters/table_properties lose only None values on the Glue path. That makes them equal to what the Athena path returns now, and the accessors are unchanged.
Documentation: docs/sqlalchemy.md and the usage docs describe when Glue answers, not the parameter shape, so they need no change.
Operations: no request is added or removed; only the conversion of responses already fetched changes.
Evidence limits: 34 Glue-related tests passed locally against AWS at this head, and the full suite will run once the PR is Ready. The None-to-absent change was measured only in us-west-2.
…ters Leaving an unset storage value out must not expose a table parameter of the same name, which the descriptor replaced before: remove that parameter too, and replace the SerDe library only when the descriptor has SerDe information, as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| parameters.update( | ||
| {f"serde.param.{k}": v for k, v in (serde.get("Parameters") or {}).items()} | ||
| ) | ||
| parameters.update({k: v for k, v in storage.items() if v is not None}) |
There was a problem hiding this comment.
Independent review (relayed): FINDINGS (1 regression; repaired in 278e47c) (static)
Reviewer: OpenAI Codex CLI 0.157.1 (codex exec, model gpt-6-sol, sandbox read-only, ephemeral session 01a0eb00-5bac-75a0-8658-c2bd61b77c22), which did not author the change.
Scope: base a86a180ebbe5e18920cd75802ea5350ce8d33f10 .. head e86ec41c753ca9359e1de8e71d7cf29be64ac3b4, in a clean detached snapshot without .env. The prompt gave the intended behavior and the Athena measurements, and omitted the PR number, description, commit message, and self-review findings. The reviewer was not allowed to edit, build, test, use the network, or write to GitHub. Afterwards, the snapshot and PR worktree were unchanged.
Reviewer's result:
Surfaces covered: the specified diff; Glue conversion for missing and empty storage fields, SerDe parameters, table-level parameters, and Iceberg columns; sync and async cursor fallbacks;
AthenaTableMetadata; SQLAlchemy reflection and DDL options; tests and relevant docs. This was a static review; I ran no tests.Verdict: FINDINGS
Regression introduced by this change
- pyathena/glue.py:268: Skipping a missing descriptor value leaves a same-named table-level parameter in place. For example, if
ParameterscontainsinputformatandoutputformatbutStorageDescriptoromits both, the old conversion returnedNonefor those formats; the new conversion exposes the table-level strings.AthenaTableMetadata.file_formatthen becomes non-None, changing SQLAlchemy’s reflectedawsathena_file_formatand potentially emitted DDL. An emptySerdeInfowith a table-levelserde.serialization.libhas the same effect onrow_format. The tests pin the reported view, Iceberg, Hive, and S3 Tables shapes and would catch the oldNoneentries, but do not cover these collisions. The supplied Athena measurements do not establish its result for a collision.Pre-existing issues: None identified in the reviewed scope.
Author verification: confirmed. Round one had noted the same collision, but only as an unmeasured edge case. With Parameters holding inputformat and a descriptor without InputFormat, this head exposes the table-level string, where the old code returned None. AthenaTableMetadata.file_format and row_format read those keys, so reflected awsathena_file_format/awsathena_row_format would change.
There was a problem hiding this comment.
Repair: 278e47c
table_metadata again replaces the same table parameters as before and changes only how an unset value is reported. location, inputformat and outputformat are always replaced, and serde.serialization.lib only when the descriptor has SerdeInfo. When the descriptor value is None, the same-named table parameter is now removed instead of being set to None. A table-level value therefore never shows through, and file_format/row_format return what they did before for any collision. The docstring now states this precedence.
Regression test: the new test_table_metadata[table_parameter_collision] case covers a table-level location/inputformat/serde.serialization.lib with a view-like descriptor. It fails against e86ec41 and passes at 278e47c.
Validation at 278e47c: just lint passes; offline test_glue.py 11 passed; the Glue-related AWS tests (-k "glue or Glue" across test_glue.py, the sync/async cursor tests, and the sync/async SQLAlchemy base tests) 35 passed locally.
Next: both self-review perspectives on the repair, then an independent follow-up.
There was a problem hiding this comment.
Independent follow-up review of the repair (relayed): CLEAN (static)
Reviewer: OpenAI Codex CLI 0.157.1 (codex exec, model gpt-6-sol, sandbox read-only, ephemeral session 01a0eb07-9b5a-7780-ac6d-9b5c3a41819d).
Scope: git range-diff a86a180..e86ec41 a86a180..278e47c plus the complete diff, in a clean detached snapshot of 278e47c without .env. The prompt stated the change's contract and did not include the PR, the earlier finding, or the self-review records. No edits, builds, tests, network access, or GitHub writes were allowed. Afterwards, the snapshot was unchanged.
Reviewer's result:
Surfaces covered: The range-diff and both requested diffs;
GlueMetadataClient.table_metadataacross absent, empty, and colliding storage values; SerDe parameters;AthenaTableMetadataaccessors; synchronous and asynchronous cursor metadata paths; SQLAlchemy reflection; and the changed tests and docstring.Verdict: CLEAN. The only observed change for valid Glue table shapes is that unset descriptor storage values are omitted instead of set to
None. Empty strings and the merge-base precedence rules remain intact. No actionable regressions or pre-existing issues were identified in scope.This was a static, read-only review; no tests were run.
| parameters = dict(table.get("Parameters") or {}) | ||
| for key, value in storage.items(): | ||
| if value is None: | ||
| parameters.pop(key, None) |
There was a problem hiding this comment.
Self-review of the repair, round one (implementation behavior): CLEAN
Scope: git range-diff a86a180..e86ec41 a86a180..278e47c. The base is unchanged, and the repair adds one commit touching pyathena/glue.py table_metadata and tests/pyathena/test_glue.py. Both old objects exist locally.
Equivalence check: master's table_metadata (a86a180) was compared with this head over every combination of four table-level storage parameters (each present or absent) × Location/InputFormat/OutputFormat absent, '', or set × SerdeInfo absent, {}, with a library, with an empty library, or with only parameters. In all 2160 combinations, the new parameters equal the old parameters with the None-valued entries removed. So the precedence over same-named table parameters, the SerDe condition, and serde.param.* are unchanged, and the only difference is None becoming absent.
Callers: the same get_table/list_tables paths as in round one. The AthenaTableMetadata accessors (location, input_format, output_format, serde_serialization_lib, and file_format/row_format derived from them) return the same values as on master for every input.
Tests: the table_parameter_collision case fails on e86ec41 and passes here. Offline 11 passed, and the Glue-related AWS tests 35 passed at 278e47c.
| replacing parameters of the same name: the location and formats, the | ||
| SerDe library whenever the descriptor has SerDe information, and SerDe | ||
| parameters with a ``serde.param.`` prefix. A value the descriptor | ||
| leaves unset is left out rather than reported as None; an empty string |
There was a problem hiding this comment.
Self-review of the repair, round two (claims): CLEAN
Scope: the same range-diff, the repair commit message, the new docstring, the thread reply, and the PR body.
- Docstring: "replacing parameters of the same name: the location and formats, the SerDe library whenever the descriptor has SerDe information" matches the code (
storagealways has the three keys, and the SerDe key only if"SerdeInfo" in descriptor). "A value the descriptor leaves unset is left out rather than reported as None; an empty string is kept" matches theis Nonebranch and theview/s3_tablescases. - Commit message and reply: "as before" holds by the 2160-combination comparison with master.
- PR body: still accurate. Its WHAT describes adding values "only when Glue sets them", and the table-parameter precedence is unchanged from before, so the description needs no correction.
- Operations: unchanged; no request is added.
1 participant
WHAT
GlueMetadataClient.table_metadata(pyathena/glue.py) now adds the flattened storage values (location,inputformat,outputformat,serde.serialization.lib) to a table's parameters only when Glue sets them. Previously it always added the location and formats, and added the SerDe library whenever the descriptor hadSerdeInfo, usingNonewhen Glue had no value. Empty strings are still reported, such as a view'slocation: ''and an S3 Tables table's empty formats.The descriptor still replaces the same table parameters as before: the location and formats always, and the SerDe library when the descriptor has
SerdeInfo. When the descriptor value is unset, the same-named table parameter is removed instead of being set toNone, so a table-level value never shows through. Compared with master over 2160 input combinations, the only difference is thatNone-valued entries become absent.The offline
test_table_metadatacases for a view and an Iceberg table now expect noNone-valued storage keys. A news3_tablescase pins the empty-string formats, and atable_parameter_collisioncase pins the precedence.WHY
Closes #887. Athena's
GetTableMetadata/ListTableMetadatanow omit the storage values a table does not have. The Glue fallback (#803, unreleased) no longer matched Athena for views and for Iceberg tables inAwsDataCatalog. The AWS suite failed intest_glue.py::test_reads_what_athena_reportsand in the sync and async versions oftest_throttled_metadata_reads_glue. This was seen in #885's CI and locally at the heads of #884 and #885, so it is not tied to either branch. Users of the fallback also got extraNone-valued entries inAthenaTableMetadata.parametersand in the reflectedawsathena_tblproperties. The property accessors (input_format,output_format,serde_serialization_lib,location) use.get()and returnNoneeither way.Measured on 2026-09-29 in us-west-2 by comparing raw
GetTableMetadataresponses (5 calls each) with the converted GlueGetTableresponses for the same tables. The probe was temporary and is not committed.VIRTUAL_VIEW)location: ''onlyinputformat,outputformat,serde.serialization.libasNoneAwsDataCataloglocationonlyinputformat,outputformatasNonelocation,inputformat: '',outputformat: ''Every call returned the same shape. Earlier on the same day, the same test passed and failed within minutes, which suggests Athena was rolling the change out then.
TEST
Tested commit: 278e47c
just format,just lint: pass.pytest --noconftest tests/pyathena/test_glue.py -k "test_table_metadata or test_supports or current": 11 passed.uv run --env-file .env 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 Glue": 35 passed. This includes the three tests that failed before the fix and the Glue-backed reflection tests, including S3 Tables.Not run locally: the full AWS suites; the PR run covers them once Ready.
🤖 Generated with Claude Code