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

Minor: Convert Count's name to lowercase #11028

Merged
merged 8 commits into from
Jun 22, 2024

Conversation

jayzhan211
Copy link
Contributor

Which issue does this PR close?

Part of #10695

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@github-actions github-actions bot added sql SQL Planner optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) substrait labels Jun 20, 2024
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211 jayzhan211 changed the title Convert Count's name to lowercase Minor: Convert Count's name to lowercase Jun 20, 2024
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211 jayzhan211 marked this pull request as ready for review June 21, 2024 00:05
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 @jayzhan211

I merged up from main with this branch to resolve a conflict

@@ -1986,7 +1986,7 @@ mod tests {

assert_batches_sorted_eq!(
["+----+-----------------------------+-----------------------------+-----------------------------+-----------------------------+-------------------------------+----------------------------------------+",
"| c1 | MIN(aggregate_test_100.c12) | MAX(aggregate_test_100.c12) | AVG(aggregate_test_100.c12) | sum(aggregate_test_100.c12) | COUNT(aggregate_test_100.c12) | COUNT(DISTINCT aggregate_test_100.c12) |",
"| c1 | MIN(aggregate_test_100.c12) | MAX(aggregate_test_100.c12) | AVG(aggregate_test_100.c12) | sum(aggregate_test_100.c12) | count(aggregate_test_100.c12) | count(DISTINCT aggregate_test_100.c12) |",
Copy link
Contributor

Choose a reason for hiding this comment

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

@jayzhan211 are we planning to align also MIN, MAX, AVG to be lowercase?

Copy link
Contributor Author

@jayzhan211 jayzhan211 Jun 21, 2024

Choose a reason for hiding this comment

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

Starts from #10695, I plan to rename all the UDAF to lowercase to have a consistent naming

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

Thanks @jayzhan211 I personally prefer function names to be uppercase, its more readable imho, but what is more important all of it has to be the same case to avoid a mess. Are you planning to convert all others to lowercase as well?

@alamb
Copy link
Contributor

alamb commented Jun 21, 2024

Thanks @jayzhan211 I personally prefer function names to be uppercase, its more readable imho, but what is more important all of it has to be the same case to avoid a mess. Are you planning to convert all others to lowercase as well?

I think the case is being converted as part of the migration to UDAF -- for example #10964

@jayzhan211
Copy link
Contributor Author

jayzhan211 commented Jun 21, 2024

Thanks @jayzhan211 I personally prefer function names to be uppercase, its more readable imho, but what is more important all of it has to be the same case to avoid a mess. Are you planning to convert all others to lowercase as well?

We can introduce config setting to have upper case for display, but lowercase for internal use (we get the function or compare with it's name) if you hope so.

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211
Copy link
Contributor Author

Thanks @alamb and @comphead

@jayzhan211 jayzhan211 merged commit 569be9e into apache:main Jun 22, 2024
23 checks passed
@jayzhan211 jayzhan211 deleted the lowercase-count branch June 22, 2024 07:19
xinlifoobar pushed a commit to xinlifoobar/datafusion that referenced this pull request Jun 22, 2024
* push down non-unnest only

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* add doc

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* to lowercase

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* fix tpch

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* Update test

* fix test

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

---------

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
xinlifoobar pushed a commit to xinlifoobar/datafusion that referenced this pull request Jun 22, 2024
* push down non-unnest only

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* add doc

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* to lowercase

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* fix tpch

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* Update test

* fix test

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

---------

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
* push down non-unnest only

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* add doc

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* to lowercase

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* fix tpch

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

* Update test

* fix test

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

---------

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
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 sql SQL Planner sqllogictest SQL Logic Tests (.slt) substrait
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants