fix(common): remove unsupported cross join mode from left_join_df - #43016
Conversation
left_join_df's how parameter advertised Literal["left", "right", "inner", "outer", "cross"], but the join is implemented via DataFrame.set_index(...).join(...), whose underlying Index.join does not support "cross" the way pd.merge does -- passing how="cross" silently drops the join keys instead of producing a real cross join. Narrow the type to the modes that are actually supported. _perform_join in models/helpers.py forwards its own how parameter straight into left_join_df, so its signature is narrowed the same way to stay consistent. Follow-up to #41334. Co-Authored-By: Claude <noreply@anthropic.com>
Code Review Agent Run #f53926Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43016 +/- ##
=======================================
Coverage 66.59% 66.59%
=======================================
Files 2863 2863
Lines 161681 161681
Branches 37249 37249
=======================================
Hits 107678 107678
Misses 51961 51961
Partials 2042 2042
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR tightens type annotations around Superset’s DataFrame join helpers to match actual pandas join capabilities, removing an advertised "cross" join mode that isn’t supported by the underlying DataFrame.set_index(...).join(...) implementation.
Changes:
- Narrow
left_join_df(..., how=...)to exclude"cross"from itsLiteral[...]type. - Narrow
ExploreMixin._perform_join(..., how=...)to the same set of supported join modes to keep typing consistent. - Add an inline comment documenting why
"cross"is intentionally unsupported here.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
superset/common/utils/dataframe_utils.py |
Removes "cross" from left_join_df’s how type and documents the limitation of the join implementation. |
superset/models/helpers.py |
Narrows _perform_join’s how type to match left_join_df and avoid propagating an unsupported mode. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
sha174n
left a comment
There was a problem hiding this comment.
LGTM. cross was never valid for the Index.join path anyway, so narrowing the Literal is correct and cannot affect valid callers. CI green.
Follow-up to #41334.
SUMMARY
left_join_df'showparameter is typedLiteral["left", "right", "inner", "outer", "cross"], but the join is implemented viaDataFrame.set_index(...).join(...), whose underlyingIndex.joindoesn't support"cross"the waypd.mergedoes. Passinghow="cross"silently drops the join keys instead of producing a real cross join, so the type signature was advertising a mode that doesn't actually work.This removes
"cross"from theLiteralinleft_join_df(superset/common/utils/dataframe_utils.py).ExploreMixin._perform_joininsuperset/models/helpers.pyforwards its ownhowparameter straight intoleft_join_df, so its signature is narrowed the same way to keep mypy happy and stay consistent.Neither function is ever called with
how="cross"today (join_offset_dfs, the only caller, only ever passes"left"or"outer"for the time-comparison full-range option added in #41334), so this is a type-only fix with no behavior change.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - type-only change, no UI impact.
TESTING INSTRUCTIONS
pytest tests/unit_tests/common/test_time_shifts.pyandpytest tests/unit_tests/common/test_query_context_processor.py -k joinstill pass.pre-commit run(including mypy) passes on the changed files, confirming the narrower type doesn't break the one call site.ADDITIONAL INFORMATION