Sync fork main with upstream - #9
Conversation
…07-31 # Conflicts: # .github/workflows/numbarrow_ci.yml
Upstream moved the shared build job to pandas 2.3.2. The fork-only build-macos job never conflicted, so it kept 2.2.3 and would have tested a different pandas than Linux and Windows.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
MiniMax Code ReviewThe user is asking me to review a pull request with multiple changes. Let me analyze each file change:
Now let me review each change for issues: pyproject.toml changes: The shift to Python 3.12 as a minimum requirement represents a substantial compatibility leap. The dependency expansions for numba and pyarrow are reasonable, but the version bumps could introduce breaking changes. The CLAUDE.md changes: mapinarrow_factory.py changes: General concerns: Code Review: Pull RequestSummaryThis PR makes several coordinated changes: updating Python version to 3.12, updating dependencies (numba, pyarrow, pandas), and modifying CI to test "below the floor" with Issues Found1. CLAUDE.md: Conflicting Documentation (Medium)The CLAUDE.md says both:
This is contradictory. If the package "still builds, imports and passes the full suite on 3.10 and 3.11", then 3.10/3.11 are effectively supported. The framing suggests this is a workaround to avoid updating Recommendation: Remove the 2. numbarrow/core/mapinarrow_factory.py: Fragile Unpacking (Medium)vals = arrow_array_adapter(col_pa)
*_, col_bitmap, col_data = valsThis discards all but the last two return values using
Recommendation: Either:
3. pyproject.toml: Unbounded Upper Version Limits (Low)"pyarrow<=24.0.0"This upper bound will continue to require updates as PyArrow releases new versions. Consider:
4. LICENSE: Copyright Change (Info)Changed from "Mikhail Goykhman" to "NumbArrow GitHub Repository Contributors". Ensure this change is intentional and that Mikhail Goykhman (the original author) has agreed to this re-licensing/copyright change. 5. CI Workflow: Duplicate Changes (Low)The same two changes (pandas version, Positive Changes
Action Items
|
There was a problem hiding this comment.
Pull request overview
This PR syncs the fork’s main branch with upstream/main, bringing in upstream changes to packaging metadata, licensing/authorship, and PySpark mapInArrow integration, plus aligning CI dependency pins.
Changes:
- Updated project metadata and dependency constraints in
pyproject.toml(Python/Numba/PyArrow bounds; test extras). - Adjusted
make_mapinarrow_functo accept the newer multi-value return shape fromarrow_array_adapter. - Updated GitHub Actions workflows to run/build with Python 3.12 and align pandas pinning across jobs; updated LICENSE header.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Updates supported Python version and dependency bounds; adds a test extra requirement. |
numbarrow/core/mapinarrow_factory.py |
Updates adapter unpacking logic used to build data_dict / bitmap_dict for mapInArrow. |
LICENSE |
Updates copyright holder/year to reflect upstream changes. |
.github/workflows/numbarrow_release.yml |
Moves release build environment to Python 3.12. |
.github/workflows/numbarrow_ci.yml |
Aligns pandas pin across build jobs; retains python/numba matrices. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ] | ||
| dynamic = ["version"] | ||
| requires-python = ">=3.10" | ||
| requires-python = ">=3.12" |
| vals = arrow_array_adapter(col_pa) | ||
| *_, col_bitmap, col_data = vals | ||
| col_bitmap = col_bitmap if isinstance(col_bitmap, dict) else {} if col_bitmap is None else {col: col_bitmap} # noqa: E501 | ||
| col_data = col_data if isinstance(col_data, dict) else {col: col_data} |
Upstream raised requires-python to >=3.12, which made every 3.10 and 3.11 matrix job fail at 'pip install -e .' with 'Package numbarrow requires a different Python'. The floor is metadata only: the package builds, imports and passes the full suite on both, so the flag keeps the existing coverage rather than dropping two interpreters. Measured on this branch, 34 passed in each of 3.10/3.11 crossed with numba 0.60.0/0.63.0, against pyarrow 24.0.0.
The Build & Dev block restated requires-python and four dependency ranges. This sync silently invalidated four of them at once: Python >=3.10 (now >=3.12), pyarrow <=15.0.0 (now <=24.0.0), and the numba and pyspark ceilings were never recorded. Nothing failed, because nothing checks prose. Replaced with a pointer to the declarations that are actually enforced, plus the one fact pyproject.toml cannot express -- that CI tests 3.10 and 3.11 below the declared floor on purpose, and what that does and does not mean.
Brings fork
mainlevel withupstream/main, which was two commits ahead:c10525b(improvedmapinarrow_factory) and0f696a9(updated authors, license).Merged, not squashed, so upstream's commits keep their ancestry and do not reappear as unmerged at the next sync.
Conflict resolution
One conflict, in
numbarrow_ci.yml, both hunks in the sharedbuildjob:python-version— upstream pins"3.12", the fork drives it frommatrix.python-version. Kept the fork's matrix.pandas2.2.3 → 2.3.2 on a line the fork had also changed to addnumba==${{ matrix.numba-version }}. Kept both: the fork's numba matrix line and upstream's pandas bump.The fork-only
build-macosjob never conflicted, so it keptpandas==2.2.3and would have tested a different pandas than Linux and Windows. Aligned to 2.3.2 in a follow-up commit on this branch.requires-pythonraised to>=3.120f696a9also raisedrequires-pythonfrom>=3.10to>=3.12(alongsidenumba<0.67.0,pyarrow<=24.0.0, and asetuptoolstest dep). That made every 3.10 and 3.11 matrix job fail atpip install -e .:The floor is metadata only — nothing in the code or the new dependency ceilings needs 3.12. Measured on this branch against
pyarrow 24.0.0:python -m build --wheelalso succeeds on 3.10.So both jobs now install with
--ignore-requires-python, keeping the existing three-interpreter coverage rather than dropping two. This deliberately tests below upstream's declared floor: a real user on 3.10 is refused by pip until upstream changes it, so these jobs are regression signal rather than a supported configuration.Verified after the merge
numbarrow/,LICENSE,pyproject.tomlbyte-identical toupstream/mainREADME.mddiffers by the two pre-existing fork-only lines definingdf_in/output_schemain the example, which the fork's own doc-codeblock check requires and upstream has no equivalent of.github/scripts/extract_codeblocks.py,doc-codeblock-flake8.yml,link-check.yml,CLAUDE.md