[Data] Fixing handling of renames in projection pushdown by alexeykudinkin · Pull Request #58033 · ray-project/ray

alexeykudinkin · 2025-10-23T04:14:33Z

Description

This change properly handles of pushing of the renaming projections into read ops (that support projections, like parquet reads).

Related issues

Link related issues: "Fixes #1234", "Closes #1234", or "Related to #1234".

Additional information

Optional: Add implementation details, API changes, usage examples, screenshots, etc.

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

gemini-code-assist

Code Review

This pull request introduces a significant improvement to projection pushdown by enabling column renames to be pushed into the read operations. The changes are well-structured, introducing a new utility for collapsing transitive rename maps and updating the logical operators and datasources accordingly. The core logic in the ProjectionPushdown rule correctly distinguishes between simple projections that can be fully pushed down and complex ones that require keeping the Project operator. I've identified a few areas for improvement, including removing leftover debug statements, adding a missing type hint for better maintainability, and minor code refinements for clarity and robustness.

gemini-code-assist · 2025-10-23T04:15:46Z

python/ray/data/_internal/collections.py

+
+        # Follow the chain until we reach a key that's not in the mapping
+        while cur in d:
+            next = d[cur]


The variable next shadows the built-in function next(). It's a good practice to avoid shadowing built-ins to prevent potential confusion and bugs. Consider renaming it to something more specific like next_key or next_val.

Suggested change

next = d[cur]

next_val = d[cur]

gemini-code-assist · 2025-10-23T04:15:46Z

python/ray/data/_internal/datasource/parquet_datasource.py

+def _combine_rename_map(
+    prev_column_rename_map: Optional[Dict[str, str]],
+    new_column_rename_map: Optional[Dict[str, str]],
+):
+    if not prev_column_rename_map:
+        combined = new_column_rename_map
+    elif not new_column_rename_map:
+        combined = prev_column_rename_map
+    else:
+        combined = prev_column_rename_map | new_column_rename_map
+
+    return collapse_transitive_map(combined)


This function is missing a return type hint. Adding it would improve type safety and code clarity. Additionally, the logic for combining the dictionaries can be made more concise and robust against None values by using or {}.

def _combine_rename_map( prev_column_rename_map: Optional[Dict[str, str]], new_column_rename_map: Optional[Dict[str, str]], ) -> Dict[str, str]: combined = (prev_column_rename_map or {}) | (new_column_rename_map or {}) return collapse_transitive_map(combined)

python/ray/data/_internal/logical/rules/projection_pushdown.py

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

srinathk10

LGTM

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

## Description This change properly handles of pushing of the renaming projections into read ops (that support projections, like parquet reads). ## Related issues > Link related issues: "Fixes #1234", "Closes #1234", or "Related to #1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

…8037) ## Description Cherry-pick of #58033 ## Related issues > Link related issues: "Fixes #1234", "Closes #1234", or "Related to #1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- > Thank you for contributing to Ray! 🚀 > Please review the [Ray Contribution Guide](https://docs.ray.io/en/master/ray-contribute/getting-involved.html) before opening a pull request. > ⚠️ Remove these instructions before submitting your PR. > 💡 Tip: Mark as draft if you want early feedback, or ready for review when it's complete. ## Description > Briefly describe what this PR accomplishes and why it's needed. ## Related issues > Link related issues: "Fixes #1234", "Closes #1234", or "Related to #1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

…#58033) ## Description This change properly handles of pushing of the renaming projections into read ops (that support projections, like parquet reads). ## Related issues > Link related issues: "Fixes ray-project#1234", "Closes ray-project#1234", or "Related to ray-project#1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Alexey Kudinkin <ak@anyscale.com> Signed-off-by: xgui <xgui@anyscale.com>

…#58033) ## Description This change properly handles of pushing of the renaming projections into read ops (that support projections, like parquet reads). ## Related issues > Link related issues: "Fixes ray-project#1234", "Closes ray-project#1234", or "Related to ray-project#1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

…#58033) ## Description This change properly handles of pushing of the renaming projections into read ops (that support projections, like parquet reads). ## Related issues > Link related issues: "Fixes ray-project#1234", "Closes ray-project#1234", or "Related to ray-project#1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Alexey Kudinkin <ak@anyscale.com> Signed-off-by: Aydin Abiar <aydin@anyscale.com>

…#58033) ## Description This change properly handles of pushing of the renaming projections into read ops (that support projections, like parquet reads). ## Related issues > Link related issues: "Fixes ray-project#1234", "Closes ray-project#1234", or "Related to ray-project#1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Alexey Kudinkin <ak@anyscale.com> Signed-off-by: Future-Outlier <eric901201@gmail.com>

…#58033) (ray-project#58037) ## Description Cherry-pick of ray-project#58033 ## Related issues > Link related issues: "Fixes ray-project#1234", "Closes ray-project#1234", or "Related to ray-project#1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- > Thank you for contributing to Ray! 🚀 > Please review the [Ray Contribution Guide](https://docs.ray.io/en/master/ray-contribute/getting-involved.html) before opening a pull request. > ⚠️ Remove these instructions before submitting your PR. > 💡 Tip: Mark as draft if you want early feedback, or ready for review when it's complete. ## Description > Briefly describe what this PR accomplishes and why it's needed. ## Related issues > Link related issues: "Fixes ray-project#1234", "Closes ray-project#1234", or "Related to ray-project#1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

…#58033) ## Description This change properly handles of pushing of the renaming projections into read ops (that support projections, like parquet reads). ## Related issues > Link related issues: "Fixes ray-project#1234", "Closes ray-project#1234", or "Related to ray-project#1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

…#58033) ## Description This change properly handles of pushing of the renaming projections into read ops (that support projections, like parquet reads). ## Related issues > Link related issues: "Fixes ray-project#1234", "Closes ray-project#1234", or "Related to ray-project#1234". ## Additional information > Optional: Add implementation details, API changes, usage examples, screenshots, etc. --------- Signed-off-by: Alexey Kudinkin <ak@anyscale.com> Signed-off-by: peterxcli <peterxcli@gmail.com>

Fixing projection pushdown

bd1c630

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

alexeykudinkin requested review from a team as code owners October 23, 2025 04:14

alexeykudinkin added the go add ONLY when ready to merge, run all tests label Oct 23, 2025

gemini-code-assist bot reviewed Oct 23, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

Tidying up

91406d7

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

alexeykudinkin changed the title ~~[Data] Fixing projection pushdown~~ [Data] Fixing handling of renames in projection pushdown Oct 23, 2025

lint

f4f47ad

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

This comment was marked as outdated.

Sign in to view

srinathk10 approved these changes Oct 23, 2025

View reviewed changes

Relocated collections utils

8070405

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

alexeykudinkin force-pushed the ak/prj-pdwn-fix branch from 79cad0e to 8070405 Compare October 23, 2025 05:22

Fixing py-doc

3d8e93d

Signed-off-by: Alexey Kudinkin <ak@anyscale.com>

alexeykudinkin enabled auto-merge (squash) October 23, 2025 06:21

github-actions bot disabled auto-merge October 23, 2025 06:21

alexeykudinkin enabled auto-merge (squash) October 23, 2025 06:22

alexeykudinkin merged commit 4130e4d into master Oct 23, 2025
8 checks passed

alexeykudinkin deleted the ak/prj-pdwn-fix branch October 23, 2025 06:57

alexeykudinkin mentioned this pull request Oct 23, 2025

[Data] Fixing handling of renames in projection pushdown (#58033) #58037

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Data] Fixing handling of renames in projection pushdown#58033

[Data] Fixing handling of renames in projection pushdown#58033
alexeykudinkin merged 5 commits intomasterfrom
ak/prj-pdwn-fix

alexeykudinkin commented Oct 23, 2025 •

edited

Loading

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Oct 23, 2025

Uh oh!

gemini-code-assist bot Oct 23, 2025

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

srinathk10 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

alexeykudinkin commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Additional information

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

srinathk10 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alexeykudinkin commented Oct 23, 2025 •

edited

Loading