Skip to content

Conversation

@daniel-thom
Copy link
Collaborator

@daniel-thom daniel-thom commented Oct 21, 2025

This is a workaround for the fact that the latest pyhive code is not published on pypi.org. The latest published version has a bug fixed by this commit. The pyhive source code was donated to Apache Kyuubi some time ago. They have not yet released a new version with this fix. In order to publish chronify (and dsgrid) packages that support Apache Spark on pypi.org, we need this commit. (You cannot publish packages that install from git repositories.)

We do not know when the kyuubi project will release an official version.

This seems to be the least-bad solution out of some bad options. Also considered:

  • Require users to manually install from the kyuubi repository. This is really not good.
  • Publish our own version of pyhive on pypi.org. Other users may have already done this. I don't want to do that and I don't want to depend on random packages.

@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2025

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.59%. Comparing base (1e0b6e2) to head (d07c5a6).

Files with missing lines Patch % Lines
src/chronify/time_series_mapper_datetime.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #55      +/-   ##
==========================================
- Coverage   91.59%   91.59%   -0.01%     
==========================================
  Files          47       49       +2     
  Lines        4032     4043      +11     
==========================================
+ Hits         3693     3703      +10     
- Misses        339      340       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This is a workaround for the fact that the latest pyhive package is not
published on pypi.org.
Copy link
Member

@elainethale elainethale left a comment

Choose a reason for hiding this comment

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

Makes sense to me. I assume dsgrid will import pyhive after importing chronify (i.e., not carry around our own copy)?

pyproject.toml Outdated
# Required by pyhive
"future",
"python-dateutil",

Copy link
Member

Choose a reason for hiding this comment

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

nit: blank line

@daniel-thom
Copy link
Collaborator Author

daniel-thom commented Oct 21, 2025

Makes sense to me. I assume dsgrid will import pyhive after importing chronify (i.e., not carry around our own copy)?

dsgrid will not need to import pyhive. Pyhive gets registered with sqlalchemy when pyhive is loaded (indirectly through chronify now). Our software doesn't actually make calls to pyhive.

@elainethale
Copy link
Member

Makes sense to me. I assume dsgrid will import pyhive after importing chronify (i.e., not carry around our own copy)?

dsgrid will not need to import pyhive. Pyhive gets registered with sqlalchemy when chronify is installed. Our software doesn't actually make calls to pyhive.

Even better!

@daniel-thom
Copy link
Collaborator Author

This PR was bypassed in favor of #56. I think that was a mistake because dependency groups cannot be used by users installing from pipy.org. I think we should use this instead. Users will be able to

pip install "chronify[spark]"

and get all functionality.

@daniel-thom daniel-thom merged commit 731845e into main Dec 4, 2025
4 checks passed
@daniel-thom daniel-thom deleted the include-pyhive branch December 4, 2025 16:21
github-actions bot pushed a commit that referenced this pull request Dec 4, 2025
Include pyhive inside the chronify package
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.

4 participants