Skip to content

Preserve Hive string types in metadata fallback reflection - #784

Merged
laughingman7743 merged 1 commit into
masterfrom
fix/metadata-string-reflection
Sep 20, 2026
Merged

laughingman7743 merged 1 commit into
masterfrom
fix/metadata-string-reflection

Conversation

@laughingman7743

@laughingman7743 laughingman7743 commented Sep 20, 2026 •

Copy link
Copy Markdown
Member

WHAT

Normalize unbounded varchar from information_schema.columns to SQLAlchemy String, matching reflection of Hive STRING through Athena's metadata API.
Keep bounded VARCHAR(n) and CHAR(n) types and lengths unchanged.

Extend the existing Hive string reflection integration test to query the fallback directly as well as use the Inspector, with exact type and length assertions.
Both paths execute against Athena through the existing fixtures; no new mocks are used.
Update the existing unit expectation and the fallback documentation.

WHY

Follow-up to #780 and the information_schema fallback introduced in #777.
This fixes the string-type mismatch on that fallback path.

After a throttled metadata request, the fallback returned VARCHAR() where the metadata API returned String().
This made reflected types depend on throttling and failed the existing test_create_table_with_varchar_text_column on Python 3.11 and 3.12 in PR #770's CI.
PR #770 has since merged after a successful CI rerun; this follow-up fixes the metadata type discrepancy observed in its earlier run.

Validation:

  • Before the implementation fix, the expanded live test failed only for the fallback's unbounded string case: 1 failed, 5 passed.

  • After the fix, the same six live cases passed with REST and all six passed with async REST, run sequentially with -n 1.

  • The existing failing VARCHAR/Text reflection integration test and the existing fallback conversion unit test both passed (2 tests).

  • just format, just lint, and just docs lint passed.

  • The current-head CI passed all 18 checks, including the five Python versions for regular, synchronous SQLAlchemy and asynchronous SQLAlchemy suites, docs lint/build, and benchmark tooling.

  • Both self-review rounds and the independent Claude Code static review returned CLEAN; records are inline.

  • Current-head CI passed. The post-Restore SQLAlchemy binary compliance and preserve CSV binary NULLs #770 master integration was also checked for conflicts and preservation of the four-file patch.

column_name,
data_type,
# Athena exposes Hive STRING as unbounded VARCHAR in information_schema.
"string" if data_type == "varchar" else data_type,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round one: implementation and tests / CLEAN

Base: f7b23698bb711c7d02b892d5f0295f02f1885e04
Head: 72649ac1bd24aaa5413661dc11fd80ae4bc5d07d

Reviewed all four changed files and traced _get_columns into _columns_from_information_schema and _column/_get_column_type. Only the exact unbounded varchar spelling is normalized on the fallback path; bounded VARCHAR/CHAR, normal API reflection, nested-type handling, cache keys, sorting, resource ownership, retry rules and async forwarding are unchanged. The expanded existing fixture-based test performs real Athena queries without new mocks, and exact type checks distinguish String from its VARCHAR subclass. Negative control before the fix: the fallback String case failed and the other five passed. After the fix: six REST and six async REST cases passed, plus the original failing integration case and existing conversion unit case. Format/lint/docs lint passed.

column_name,
data_type,
# Athena exposes Hive STRING as unbounded VARCHAR in information_schema.
"string" if data_type == "varchar" else data_type,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round two: compatibility, operations and factual claims / CLEAN

Base: f7b23698bb711c7d02b892d5f0295f02f1885e04
Head: 72649ac1bd24aaa5413661dc11fd80ae4bc5d07d

Audited the PR description, changed comment and documentation against the live before/after evidence and callers. Hive unbounded strings now have consistent reflected class identity across the API and fallback. VARCHAR(52) and CHAR(52) retain both their concrete classes and length in REST and async REST tests. The change issues no additional API calls or SQL queries and changes neither retries nor cache freshness. No claim is made of new federated-catalog validation or complete CI success. Direct fallback integration tests make the regression deterministic without mocking a throttle; their entry-point dispatch and retry behavior are existing coverage, not newly measured here. The only pre-existing stub test change updates its expected output type; no mock framework was added. Full matrix and rendered docs validation remain pending in CI.

column_name,
data_type,
# Athena exposes Hive STRING as unbounded VARCHAR in information_schema.
"string" if data_type == "varchar" else data_type,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review (relayed): CLEAN.

Base: f7b23698bb711c7d02b892d5f0295f02f1885e04
Head: 72649ac1bd24aaa5413661dc11fd80ae4bc5d07d
Reviewer: Claude Code claude-fable-5-1, Max profile, high effort; session 8041f200-2c37-45eb-aba5-5f24497970e8.

The completed report was read. It reviewed all four changed files, the fallback callers, type mappings and regex, DDL compilation, async adapter, suite fixtures and related tests. It found no actionable issue: only exact unbounded varchar is remapped, bounded VARCHAR/CHAR retain lengths, and direct type resolution plus resource/retry behavior are unchanged. This is static review only; the reviewer ran no tests or builds. The tracked-source snapshot was unchanged and no tool permissions were denied.

An optional suggestion was to duplicate bounded-length coverage in the existing stub unit test. Deferred: the live sync and async regression cases already verify exact VARCHAR/CHAR classes and lengths, and the requested testing convention prioritizes existing real-Athena fixtures. No new mock coverage is needed for this two-line conversion change. Full current-head CI is still pending.

@laughingman7743
laughingman7743 marked this pull request as ready for review September 20, 2026 09:43
@laughingman7743
laughingman7743 merged commit 8c3ac04 into master Sep 20, 2026
18 checks passed
@laughingman7743
laughingman7743 deleted the fix/metadata-string-reflection branch September 20, 2026 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant