Skip to content

Improve documentation and add Display impl to EquivalenceProperties - #12590

Merged
alamb merged 1 commit into
apache:mainfrom
alamb:alamb/equivalent_props
Sep 24, 2024
Merged

Improve documentation and add Display impl to EquivalenceProperties#12590
alamb merged 1 commit into
apache:mainfrom
alamb:alamb/equivalent_props

Conversation

@alamb

@alambalamb commented Sep 23, 2024

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #12446

Closes #.

Rationale for this change

Basically while working on #12562 I found printing out and understanding EquivalenceProperties to be hard and I really liked the way @berkaysynnada formatted them in the comments here #12446 (comment)

For example:

order: [a@0 ASC,c@2 DESC], const: [b@1]

What changes are included in this PR?

  1. Implement code to format EquivalenceProperties in an easier to use way
  2. Improve documentation
  3. Add example
  4. Rename EquivalenceProperties::add_constants to EquivalenceProperties::with_constants so it conforms to an API that consumes self (leaving a deprecated EquivalenceProperties::add_constants to assist migration)

Are these changes tested?

Yes, by CI and doc tests

Are there any user-facing changes?

Better docs, easier to use APIs.

No API changes, but I deprecated add_constants and added with_constants

@alamb
alambforce-pushed the alamb/equivalent_props branch from f0a4501 to 825e4fcCompareSeptember 23, 2024 13:07
@github-actionsgithub-actionsBot added physical-expr Changes to the physical-expr crates core Core DataFusion crate and removed core Core DataFusion crate labels Sep 23, 2024
@alamb
alamb marked this pull request as draft September 23, 2024 13:08
@alamb
alambforce-pushed the alamb/equivalent_props branch from 825e4fc to 381b468CompareSeptember 23, 2024 13:19
@github-actionsgithub-actionsBot added the core Core DataFusion crate label Sep 23, 2024
/// PhysicalSortExpr::new_default(col_c).desc(),
/// ]);
///
/// assert_eq!(eq_properties.to_string(), "order: [a@0 ASC,c@2 DESC], const: [b@1]")

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This shows the example of the Display property I wanted -- that shows the columns in a reasonably human friendly way

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Screenshot 2024-09-23 at 9 46 10 AM

@alamb
alambforce-pushed the alamb/equivalent_props branch from 9fc7178 to f300930CompareSeptember 23, 2024 17:06
@alamb
alamb marked this pull request as ready for review September 23, 2024 17:06
@alamb

Copy link
Copy Markdown
ContributorAuthor

FWY @wiedld

@berkaysynnadaberkaysynnada left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Following the equivalence logs is also hard for me, thanks @alamb. This is very nice.

@alamb

Copy link
Copy Markdown
ContributorAuthor

Thank you for the review @berkaysynnada

@alamb
alamb merged commit 380d843 into apache:mainSep 24, 2024
bgjackma pushed a commit to bgjackma/datafusion that referenced this pull request Sep 25, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

coreCore DataFusion cratephysical-exprChanges to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@alamb@berkaysynnada