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

External Table Primary key support #7755

Merged
merged 12 commits into from
Oct 9, 2023
Merged

External Table Primary key support #7755

merged 12 commits into from
Oct 9, 2023

Conversation

mustafasrepo
Copy link
Contributor

Which issue does this PR close?

Closes #7154.
Thanks @parkma99 for the initial work in #7161

Rationale for this change

With the changes in this PR, we can use use primary key information in the external tables also.

What changes are included in this PR?

Are these changes tested?

Yes new tests are added to show that, primary key information is used during planning. With these changes query below can run

SELECT c, b, SUM(d)
FROM multiple_ordered_table2
GROUP BY c;

even if column b is not among the group by expressions, when it is known that column c is primary key. In this case we are sure that, for each unique c value, there will be corresponding b value (b is functionally dependent on the c). Hence, above query can run successfully.

Are there any user-facing changes?

mustafasrepo and others added 8 commits October 4, 2023 16:51
# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@github-actions github-actions bot added sql SQL Planner logical-expr Logical plan and expressions core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Oct 6, 2023
Copy link
Contributor

@ozankabak ozankabak left a comment

Choose a reason for hiding this comment

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

I reviewed this PR and it looks good to me. It'd be good to get some community feedback in case something escaped my eye.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

This looks really nice to me -- thank you @mustafasrepo

I think we should

  1. add a round trip test for the new protobuf code,
  2. Error if unsupported constraints are used

but otherwise, it looks good to go to me.

datafusion/common/src/functional_dependencies.rs Outdated Show resolved Hide resolved
datafusion/proto/src/logical_plan/to_proto.rs Show resolved Hide resolved
datafusion/sql/src/statement.rs Outdated Show resolved Hide resolved
@mustafasrepo mustafasrepo merged commit 68fb90a into apache:main Oct 9, 2023
22 checks passed
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate enhancement New feature or request logical-expr Logical plan and expressions sql SQL Planner sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for primary key for external tables.
4 participants