Skip to content

Update signature for Expr.name so that schema is no longer required - #3336

Merged
alamb merged 3 commits into
apache:masterfrom
andygrove:expr-name-no-schema
Sep 2, 2022
Merged

Update signature for Expr.name so that schema is no longer required#3336
alamb merged 3 commits into
apache:masterfrom
andygrove:expr-name-no-schema

Conversation

@andygrove

@andygroveandygrove commented Sep 1, 2022

Copy link
Copy Markdown
Member

Which issue does this PR close?

Part of #3330

Rationale for this change

The schema argument to Expr.name was not used and not needed.

What changes are included in this PR?

  • Remove schema argument from Expr.name
  • Remove some code duplication between Display and Debug traits

Are there any user-facing changes?

Yes, API change.

@andygroveandygrove added the api change Changes the API exposed to users of the crate label Sep 1, 2022
@github-actionsgithub-actionsBot added logical-expr Logical plan and expressions optimizer Optimizer rules labels Sep 1, 2022
use datafusion_common::{Result, ScalarValue};

#[test]
fn format_case_when() -> Result<()> {

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I added this test just to demonstrate the difference between ExprDisplay, Debug, and name(). I plan on adding more tests in future PRs to cover other expressions.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Merging #3336 (425996d) into master (3d37016) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #3336 +/- ##
==========================================
+ Coverage 85.74% 85.82% +0.08% 
==========================================
Files 294 294 Lines 53770 53745 -25 ==========================================
+ Hits 46104 46127 +23 + Misses 7666 7618 -48 
Impacted FilesCoverage Δ
datafusion/core/tests/sql/timestamp.rs99.65% <ø> (-0.01%)⬇️
datafusion/expr/src/expr.rs87.50% <100.00%> (+9.52%)⬆️
datafusion/expr/src/expr_schema.rs63.58% <100.00%> (ø)
datafusion/expr/src/logical_plan/builder.rs90.35% <100.00%> (ø)
datafusion/expr/src/utils.rs90.74% <100.00%> (ø)
...tafusion/optimizer/src/common_subexpr_eliminate.rs94.24% <100.00%> (+1.38%)⬆️
datafusion/optimizer/src/filter_push_down.rs98.36% <100.00%> (ø)
datafusion/optimizer/src/projection_push_down.rs98.06% <100.00%> (ø)
datafusion/optimizer/src/simplify_expressions.rs83.52% <100.00%> (ø)
...fusion/optimizer/src/single_distinct_to_groupby.rs98.22% <100.00%> (ø)
... and 7 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@alambalamb 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.

Looks like a great improvement to me. Thanks @andygrove

use datafusion_common::{Result, ScalarValue};

#[test]
fn format_case_when() -> Result<()> {

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.

❤️

_ => {
// we should not be trying to create a name for the expression
// based on the input schema but this is the current behavior
// see https://github.com/apache/arrow-datafusion/issues/2456

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.

Looks like #2456 could be closed too, perhaps

@ursabot

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 3956fc2 and contender = dadd2dc. dadd2dc is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@andygrove
andygrove deleted the expr-name-no-schema branch September 2, 2022 12:52
kmitchener pushed a commit to kmitchener/datafusion that referenced this pull request Sep 4, 2022
…pache#3336)
* Update signature for Expr.name so that schema is no longer required
* remove some duplicate code
* clippy
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api changeChanges the API exposed to users of the cratelogical-exprLogical plan and expressionsoptimizerOptimizer rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@andygrove@codecov-commenter@ursabot@alamb