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

Align UDF names to lowercase or uppercase #11779

Closed
edmondop opened this issue Aug 2, 2024 · 3 comments
Closed

Align UDF names to lowercase or uppercase #11779

edmondop opened this issue Aug 2, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@edmondop
Copy link
Contributor

edmondop commented Aug 2, 2024

Describe the bug

As a part of performing this change, @jayzhan211 recommended to use min and max as a default name for the new UDFs rather than the lowercase. However, performing this change makes test fails because in several places we use MAX and MIN, for example:

https://github.com/apache/datafusion/blob/0332eb569a5428ac385fe892ce7b5fb40d52c8c0/datafusion/sqllogictest/test_files/tpch/q15.slt.part use MAX uppercase that would cause build failure when comparing LogicalPlan like in the following build:
https://github.com/apache/datafusion/actions/runs/10215600305/job/28265554129?pr=11013

In the rust tests as well we use avg but MAX, so we need to update those tests as well

"| 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) |",

To Reproduce

No response

Expected behavior

No response

Additional context

I have not verified if MAX and MIN are the only UDF for which we use uppercase rather than lowercase. I personally prefer to have all SQL KEYWORDS (including UDF names) uppercase for readability

@jayzhan211
Copy link
Contributor

Sadly this is the last name to be lowercase, others are already converted. I don't have strong preference on upper case or lower case as long as they are all consistent

@edmondop
Copy link
Contributor Author

edmondop commented Aug 2, 2024

Agreed,I think it's better to read

SELECT MAX(stuff) rather than SELECT max(stuff) and keep the lowercase for columns and non-keyword

@edmondop edmondop changed the title Align UDF name to lowercase or uppercase Align UDF names to lowercase or uppercase Aug 2, 2024
@jayzhan211
Copy link
Contributor

Agreed,I think it's better to read

SELECT MAX(stuff) rather than SELECT max(stuff) and keep the lowercase for columns and non-keyword

The lowercase/uppercase conversion is quite painful since you need to change many places. Especially for dataframe test

It is worth to find out an easy way to update the dataframe test beforehand #10373

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

No branches or pull requests

2 participants