Skip to content

[KYUUBI #7786] Give HiveDialect its own colspecs dict - #7787

Open
rusackas wants to merge 2 commits into
apache:masterfrom
rusackas:fix-hivedialect-colspecs
Open

rusackas wants to merge 2 commits into
apache:masterfrom
rusackas:fix-hivedialect-colspecs

Conversation

@rusackas

Copy link
Copy Markdown
Member

Why are the changes needed?

python/pyhive/sqlalchemy_hive.py's HiveDialect never defines its own colspecs, so it inherits sqlalchemy.engine.default.DefaultDialect.colspecs directly, the same dict object, not a copy:

>>> from sqlalchemy.engine.default import DefaultDialect
>>> from pyhive.sqlalchemy_hive import HiveDialect
>>> HiveDialect.colspecs is DefaultDialect.colspecs
True

Every SQLAlchemy built-in dialect (postgresql, mysql, etc.) defines its own colspecs dict, even when there's nothing to override yet, specifically to avoid this. Mutating a dialect's colspecs in place (HiveDialect.colspecs[SomeType] = ...) is a completely normal way to customize how a type renders for one dialect. Without its own dict, that mutation silently corrupts type handling for every other dialect in the same process that also hasn't defined its own colspecs, not just Hive.

This isn't hypothetical: Apache Superset's Databricks engine spec subclasses HiveDialect for Databricks support and patches HiveDialect.colspecs[types.String] = ... to customize string-type handling. Because of the shared object, that one line broke type compilation for ~38 unrelated third-party dialects loaded in the same process, until Superset worked around it on its own side (apache/superset#44707).

Filed the identical fix upstream at dropbox/PyHive#490 before noticing this module had already been donated here. Confirmed the bug is present in this copy too.

Fixes #7786.

How was this patch tested?

Gave HiveDialect its own colspecs = {} and confirmed the isolation directly, same check as in the linked issue:

>>> HiveDialect.colspecs is DefaultDialect.colspecs
False
>>> HiveDialect.colspecs['marker'] = 'hive-only'
>>> DefaultDialect.colspecs
{}

colspecs was and remains empty for this dialect; it's just no longer the same object as DefaultDialect's, so a mutation on one no longer leaks into the other.

I don't have a live Hive/Kyuubi server available in my environment to run the full python/ test suite end to end, but this change touches only a class-level attribute with no effect on connection or query logic, and verified it above directly against this module.

Was this patch assisted by generative AI tooling?

Assisted-by: Claude:claude-sonnet-5

HiveDialect never defined its own colspecs, so it inherited
sqlalchemy.engine.default.DefaultDialect.colspecs directly, the same
dict object, not a copy. Mutating a dialect's colspecs in place to
customize one type is a normal thing to do (Apache Superset's
Databricks engine spec does exactly this), but without its own dict
that mutation silently corrupted type handling for every other
dialect in the process that also hadn't defined its own colspecs.

Give it an empty colspecs dict, same as every SQLAlchemy built-in
dialect does.

Fixes apache#7786.

Assisted-by: Claude:claude-sonnet-5
@rusackas

Copy link
Copy Markdown
Member Author

One more question while this is open: is there any intention to publish python/pyhive as its own installable package now that it lives here? Right now pip install pyhive still pulls dropbox's original, and this copy's own setup.py still points back at github.com/dropbox/PyHive.

The reason I'm asking: apache/superset worked around the shared-colspecs bug on its own side (apache/superset#44707) rather than waiting on a fix here or at dropbox, specifically because there wasn't a maintained release either of us could pin to. We'd genuinely like to pip install straight from this repo once there's a real release, and happy to help with packaging/release work if that'd move things along.

…d to this change (BatchesResourceSuite open-batch-session/lock-timeout, ToolRegistryThreadSafetyTest concurrency race).

This branch has not been deployed

No deployments
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.

[Bug] HiveDialect shares its colspecs dict with SQLAlchemy's DefaultDialect (python/pyhive)

1 participant