Skip to content

Conversation

@drrtuy
Copy link
Contributor

@drrtuy drrtuy commented Oct 8, 2022

Which issue does this PR close?

Closes #1192.

Rationale for this change

This is a bug fix.

What changes are included in this PR?

Are there any user-facing changes?

The expression -null now returns NULL.

There are no API changes that I am aware #of.

@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates labels Oct 8, 2022
@drrtuy drrtuy marked this pull request as ready for review October 8, 2022 15:34
@github-actions github-actions bot added the core Core DataFusion crate label Oct 8, 2022
@drrtuy
Copy link
Contributor Author

drrtuy commented Oct 8, 2022

There is a tradeoff, namely PG returns an error for the expression, however Oracle, MariaDB/MySQL returns NULL for -NULL expression. The behavior depends on what is the exemplar DB is.

Copy link
Member

@jackwener jackwener left a comment

Choose a reason for hiding this comment

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

Great job.

There is a tradeoff, namely PG returns an error for the expression, however Oracle, MariaDB/MySQL returns NULL for -NULL expression. The behavior depends on what is the exemplar DB is.

I think that behavior of PG is caused by PG forgot to implement it, handle about NULL Literal is complex.

From the type system, -NULL is also NULL.

@jackwener
Copy link
Member

jackwener commented Oct 9, 2022

PG will deduce the type of null based on other parameters.

we can specify the null type, it will ok.

jackwener=# select -null::int;
 ?column?
----------

(1 row)

deduce the type of null based on other parameters.

jackwener=# select 1+null;
 ?column?
----------

(1 row)

error, can't deduce

jackwener=# select null+null;
ERROR:  operator is not unique: unknown + unknown
LINE 1: select null+null;
                   ^
HINT:  Could not choose a best candidate operator. You might need to add explicit type casts.
jackwener=#

@alamb
Copy link
Contributor

alamb commented Oct 10, 2022

Thank you @drrtuy -- I agree with @jackwener that being more permissive than PG is a good thing.

@alamb alamb merged commit c02e9d4 into apache:master Oct 10, 2022
@ursabot
Copy link

ursabot commented Oct 10, 2022

Benchmark runs are scheduled for baseline = 8022827 and contender = c02e9d4. c02e9d4 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 logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

- (negation) with NULL literals does not work: can't be evaluated because the expression's type is Utf8, not signed

4 participants