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

Add expression nullable support #3452

Merged
merged 1 commit into from
Dec 14, 2021
Merged

Conversation

junli1026
Copy link
Contributor

I hereby agree to the terms of the CLA available at: https://databend.rs/policies/cla/

Summary

Add expression nullable support.

Changelog

  • Improvement

Related Issues

Fixes #issue

Test Plan

Stateless Tests

@databend-bot
Copy link
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

1 similar comment
@databend-bot
Copy link
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@vercel
Copy link

vercel bot commented Dec 14, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/databend/databend/Enz2YQm8UTFZteu3EBr3Q24aPmNB
✅ Preview: Canceled

[Deployment for 35957b0 canceled]

@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2021

Codecov Report

Merging #3452 (35957b0) into main (ee13a4e) will increase coverage by 0%.
The diff coverage is 60%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3452   +/-   ##
=====================================
  Coverage     61%     61%           
=====================================
  Files        610     611    +1     
  Lines      34185   34220   +35     
=====================================
+ Hits       20996   21033   +37     
+ Misses     13189   13187    -2     
Impacted Files Coverage Δ
query/src/sql/statements/statement_create_table.rs 66% <ø> (ø)
common/planners/src/plan_expression.rs 80% <60%> (-3%) ⬇️
query/src/clusters/cluster.rs 40% <0%> (-1%) ⬇️
cli/src/error.rs 32% <0%> (-1%) ⬇️
common/functions/src/scalars/strings/length.rs 0% <0%> (ø)
common/functions/src/scalars/strings/string.rs 97% <0%> (+<1%) ⬆️
...mmon/functions/src/aggregates/aggregate_min_max.rs 56% <0%> (+1%) ⬆️
metasrv/src/store/meta_raft_store.rs 80% <0%> (+2%) ⬆️
common/functions/src/aggregates/aggregate_sum.rs 89% <0%> (+3%) ⬆️
common/functions/src/aggregates/aggregate_avg.rs 94% <0%> (+3%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee13a4e...35957b0. Read the comment docs.

-- test 'create table as select' statement, expect db2.test3 has the data from db1.test1 with casting
CREATE TABLE db2.test3(x Varchar, y Varchar) ENGINE=fuse AS SELECT * FROM db1.test1;
SELECT x FROM db2.test3;
SELECT '====END TEST CREATE TABLE AS SELECT STATEMENT====';

-- clean up test databases
DROP DATABASE db1;
DROP DATABASE db2;
Copy link
Member

Choose a reason for hiding this comment

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

new line needed.

@sundy-li
Copy link
Member

Nullable is annoyment.

ClickHouse takes Nullable as a separated datatype.
Yet MySQL takes Nullable as a property for datatype.

So in the function, there are two kinds of ways to handle nullable:

  1. if args are not nullable, the result should not be nullable except some special functions (isNull.. )
  2. if args are not nullable, the result can be nullable.

I prefer the first one, thus we can control the return type.

@sundy-li
Copy link
Member

query group by nullable(col) may take much more memory/costs than query group by col.

@junli1026
Copy link
Contributor Author

junli1026 commented Dec 14, 2021

Nullable is annoyment.

ClickHouse takes Nullable as a separated datatype. Yet MySQL takes Nullable as a property for datatype.

So in the function, there are two kinds of ways to handle nullable:

  1. if args are not nullable, the result should not be nullable except some special functions (isNull.. )

Sure, will address. Do we have an example of the 'isNull' ? -- the inputs are not nullable but output is null ?

  1. if args are not nullable, the result can be nullable.

I prefer the first one, thus we can control the return type.

@sundy-li
Copy link
Member

MySQL can produce nullable result by none nullable args.

image

@junli1026
Copy link
Contributor Author

junli1026 commented Dec 14, 2021

MySQL can produce nullable result by none nullable args.

image

Right, that is why I just check the function's nullable() return. My initial thoughts are just like, checking the input, if any of them is nullable, the output should be nullable. BTW, I think we return +inf for '3/0', which makes more sense than mysql.

Also aggregate function is a little special too, that, input can be nullable, but output is most not nullable.

@databend-bot
Copy link
Member

Wait for another reviewer approval

@BohuTANG
Copy link
Member

Thank you @junli1026 !

@databend-bot
Copy link
Member

Wait for another reviewer approval

@BohuTANG BohuTANG merged commit b7d589b into datafuselabs:main Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants