Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support constraints in physical query planning #53

Open
wants to merge 14 commits into
base: apache_main
Choose a base branch
from

Conversation

gokselk
Copy link

@gokselk gokselk commented Dec 23, 2024

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@gokselk gokselk changed the title Add functional dependency support for ordering validation Support functional dependencies in physical query planning Dec 23, 2024
@gokselk gokselk marked this pull request as draft December 23, 2024 05:35
@gokselk gokselk force-pushed the feature/physical-planner-functional-dependence branch from 9f3b41c to a680f87 Compare January 3, 2025 05:25
@gokselk gokselk force-pushed the feature/physical-planner-functional-dependence branch 2 times, most recently from 42201f0 to 319f13f Compare January 3, 2025 05:27
@gokselk gokselk changed the title Support functional dependencies in physical query planning Support constraints in physical query planning Jan 3, 2025
@gokselk gokselk marked this pull request as ready for review January 3, 2025 05:29
@gokselk gokselk force-pushed the feature/physical-planner-functional-dependence branch from 319f13f to 82361a7 Compare January 3, 2025 05:39
Copy link
Collaborator

@berkaysynnada berkaysynnada left a comment

Choose a reason for hiding this comment

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

LGTM. Let's discuss the last items written down here. Then, we can upstream this PR.

@@ -254,10 +266,27 @@ impl FileScanConfig {
self.file_schema.metadata().clone(),
));

let proj_indices = match &self.projection {
Some(proj) => proj.to_vec(),
None => (0..(self.file_schema.fields().len()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check match &self.projection has been done above, And also another similar check if self.projection.is_none() exists. Perhaps we can refactor this function, starting with a match pattern on self.projection, wdyt?

Copy link
Author

Choose a reason for hiding this comment

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

Took a different approach, deduplicated the code in 46b860f. Please let me know if this works as well.

datafusion/physical-plan/src/memory.rs Outdated Show resolved Hide resolved
datafusion/sqllogictest/test_files/order.slt Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the optimizer label Jan 7, 2025
@gokselk gokselk force-pushed the feature/physical-planner-functional-dependence branch from 940c330 to 05384e1 Compare January 7, 2025 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants