Skip to content

Conversation

@liukun4515
Copy link
Contributor

Which issue does this PR close?

Closes #3673

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@liukun4515 liukun4515 requested a review from alamb October 1, 2022 11:59
@github-actions github-actions bot added core Core DataFusion crate optimizer Optimizer rules physical-expr Changes to the physical-expr crates labels Oct 1, 2022
@liukun4515 liukun4515 force-pushed the issue_#3673 branch 2 times, most recently from 99efd9e to f3c2148 Compare October 1, 2022 12:12
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.

I think the code looks good to me -- Adding some tests in datafusion/optimizer/src/type_coercion.rs that demonstrate the coercion working (especially with/without a else clause) would make this PR even better

Thanks @liukun4515

Comment on lines 375 to 378
None => Err(DataFusionError::Internal(format!(
"Failed to coerce types {:?} and {:?} in CASE WHEN expression",
then_types, else_type
))),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
None => Err(DataFusionError::Internal(format!(
"Failed to coerce types {:?} and {:?} in CASE WHEN expression",
then_types, else_type
))),
None => Err(DataFusionError::Plan(format!(
"Failed to coerce then ({:?}) and else ({:?}) to common types in CASE WHEN expression",
then_types, else_type
))),

"| 3 |",
"+------------------------------------------------+",
"+----------------------------------------------+",
"| CASE WHEN a.b IS NULL THEN NULL ELSE a.b END |",
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like an improvement but I don't understand the change

@alamb
Copy link
Contributor

alamb commented Oct 2, 2022

Ticket for CI coverage failure: #3678

liukun4515 and others added 2 commits October 4, 2022 15:46
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Copy link
Member

@xudong963 xudong963 left a comment

Choose a reason for hiding this comment

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

LGTM

@liukun4515 liukun4515 merged commit 0111732 into apache:master Oct 4, 2022
@ursabot
Copy link

ursabot commented Oct 4, 2022

Benchmark runs are scheduled for baseline = df9c418 and contender = 0111732. 0111732 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate optimizer Optimizer rules physical-expr Changes to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

support type coercion for case when expr

4 participants