Skip to content

Conversation

@pjmore
Copy link
Contributor

@pjmore pjmore commented May 1, 2021

Which issue does this PR close?

Closes #202 .

What changes are included in this PR?

Modified the method to_array_of_size from ScalarValue to add support for boolean lists.

Are there any user-facing changes?

I don't believe so, I couldn't find any relevant documentation about supported datatypes for COUNT DISTINCT but I'm not sure.

@Dandandan
Copy link
Contributor

@pjmore looks good! Maybe you could also add a test for it?

@codecov-commenter
Copy link

codecov-commenter commented May 1, 2021

Codecov Report

Merging #230 (9e48830) into master (23d02bb) will increase coverage by 0.04%.
The diff coverage is 97.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
+ Coverage   76.46%   76.50%   +0.04%     
==========================================
  Files         135      135              
  Lines       23250    23287      +37     
==========================================
+ Hits        17777    17815      +38     
+ Misses       5473     5472       -1     
Impacted Files Coverage Δ
...tafusion/src/physical_plan/distinct_expressions.rs 91.32% <97.22%> (+1.03%) ⬆️
datafusion/src/scalar.rs 54.14% <100.00%> (+0.11%) ⬆️
datafusion/src/physical_plan/group_scalar.rs 57.64% <0.00%> (+2.35%) ⬆️

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 23d02bb...9e48830. Read the comment docs.

@pjmore
Copy link
Contributor Author

pjmore commented May 1, 2021

@Dandandan I've added a test.

Copy link
Contributor

@Dandandan Dandandan left a comment

Choose a reason for hiding this comment

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

LGTM

@Dandandan
Copy link
Contributor

Dandandan commented May 2, 2021

@pjmore thanks! Could you fix the clippy error?

@alamb
Copy link
Contributor

alamb commented May 3, 2021

This is great @pjmore -- thank you -- I also tested locally and it works as advertised. 👍

> CREATE EXTERNAL TABLE t (a boolean) STORED AS CSV LOCATION '/tmp/foo.csv';
0 rows in set. Query took 0 seconds.
> select count(distinct a) from t;
+-------------------+
| COUNT(DISTINCT a) |
+-------------------+
| 1                 |
+-------------------+
1 rows in set. Query took 0 seconds.

👍

@alamb alamb merged commit b9b3d86 into apache:master May 3, 2021
@houqp houqp added the bug Something isn't working label Jul 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

COUNT DISTINCT does not support for Boolean

5 participants