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

Fix column name for COUNT(*) set by AggregateStatistics #7757

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

qrilka
Copy link
Contributor

@qrilka qrilka commented Oct 6, 2023

This fixes the issue of column name COUNT(UInt8) appearing instead of COUNT(*)

Which issue does this PR close?

Closes #104.

Rationale for this change

This should make produced column name consistent with the input SQL

What changes are included in this PR?

Are these changes tested?

Tested with cargo test

Are there any user-facing changes?

Changes column name for this particular case

This fixes the issue of column name COUNT(UInt8) appearing instead
Resolves apache#104
@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Oct 6, 2023
@qrilka
Copy link
Contributor Author

qrilka commented Oct 6, 2023

The failure in in cargo test doc seems to happen in CI itself

@alamb
Copy link
Contributor

alamb commented Oct 6, 2023

The failure in in cargo test doc seems to happen in CI itself

I believe it was fixed by #7753

Indeed -- https://github.com/apache/arrow-datafusion/actions/runs/6434063712/job/17472738458?pr=7757 looks like some infrastructure error

warning: spurious network error (1 tries remaining): [7] Couldn't connect to server (Failed to connect to crates.io port 443 after 0 ms: Couldn't connect to server)
warning: spurious network error (1 tries remaining): [7] Couldn't connect to server (Failed to connect to crates.io port 443 after 0 ms: Couldn't connect to server)
error: failed to download from `[https://crates.io/api/v1/crates/arrow-schema/47.0.0/download`](https://crates.io/api/v1/crates/arrow-schema/47.0.0/download%60)

Caused by:
  [7] Couldn't connect to server (Failed to connect to crates.io port 443 after 0 ms: Couldn't connect to server)
Error: Process completed with exit code 101.

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.

Thank you @qrilka 🙏 this looks great

I also tried it locally:

DataFusion CLI v31.0.0
❯ select count(*) from (select 1);
+----------+
| COUNT(*) |
+----------+
| 1        |
+----------+
1 row in set. Query took 0.007 seconds.

@@ -93,7 +93,7 @@ query TT
EXPLAIN select count(*) from (values ('a', 1, 100), ('a', 2, 150)) as t (c1,c2,c3)
----
physical_plan
ProjectionExec: expr=[2 as COUNT(UInt8(1))]
ProjectionExec: expr=[2 as COUNT(*)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this covers the output as well.

@alamb
Copy link
Contributor

alamb commented Oct 6, 2023

I restarted the doc test

@alamb
Copy link
Contributor

alamb commented Oct 6, 2023

Closes #104.

This might be the lowest ticket we have had closed in a while 🏆

@alamb alamb merged commit c0409a7 into apache:main Oct 6, 2023
22 checks passed
Ted-Jiang pushed a commit to Ted-Jiang/arrow-datafusion that referenced this pull request Oct 7, 2023
This fixes the issue of column name COUNT(UInt8) appearing instead
Resolves #104
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Rust] COUNT(*) schema name is COUNT(UInt8)
2 participants