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

Apply minor refactorings to functions-array crate #9787

Closed
erenavsarogullari opened this issue Mar 24, 2024 · 7 comments · Fixed by #9788
Closed

Apply minor refactorings to functions-array crate #9787

erenavsarogullari opened this issue Mar 24, 2024 · 7 comments · Fixed by #9788
Assignees
Labels
enhancement New feature or request

Comments

@erenavsarogullari
Copy link
Member

erenavsarogullari commented Mar 24, 2024

Is your feature request related to a problem or challenge?

functions-array create is introduced in terms of Epic: #9285. This issue aims to apply following minor refactorings to functions-array create:
1- Being added make_scalar_function support to missed invoke functions,
2- Aligning rust-doc across array functions,
3- Removing redundant imports,
4- Fixing typo problems,
5- Rename core.rs -> make_array.rs.

Describe the solution you'd like

Apply minor refactorings (listed above section) to functions-array create

Describe alternatives you've considered

No response

Additional context

No response

Questions:

1- Currently, all array functions are under src directory of functions-array crate. Does it make sense to package them by separating from infra source files as follows?

/functions-array
	/src
		/functions
			array_has.rs
			...
			string.rs
		lib.rs
		macros.rs
		rewrite.rs
		utils.rs

2- array_empty function is named as empty. Should we rename it as array_empty to be consistent with other array functions naming convention?

@erenavsarogullari erenavsarogullari added the enhancement New feature or request label Mar 24, 2024
@erenavsarogullari
Copy link
Member Author

take

@erenavsarogullari erenavsarogullari changed the title Apply minor refactorings to functions-array create Apply minor refactorings to functions-array crate Mar 25, 2024
@jayzhan211
Copy link
Contributor

jayzhan211 commented Mar 25, 2024

  1. It makes sense if they are getting more complex, but I think it is not that complex yet. I prefer flat structure
  2. I prefer not name them the same as function name. Actually, I prefer core than make_array, contains than array_has, but they are not a big problem.

btw, I think we will also build array-aggregate function #7213 #7214 in this package. We can arrange them after most of the aggregate function is completed.

@erenavsarogullari
Copy link
Member Author

@jayzhan211 Thanks for reply.
1- Sure, we can revisit this in terms of our new requirements.
2- I was wondering if we need consistency between https://github.com/apache/arrow-datafusion/blob/main/datafusion/functions-array/src/empty.rs#L33 and https://github.com/apache/arrow-datafusion/blob/main/datafusion/functions-array/src/empty.rs#L58. In general, we use array_* prefix for array functions naming convention so should we rename empty with array_empty by aligning with other array functions? It is not big deal, just would double check.

select empty(arrow_cast(make_array(1), 'LargeList(Int64)'));
----
false

@jayzhan211
Copy link
Contributor

jayzhan211 commented Mar 25, 2024

@jayzhan211 Thanks for reply. 1- Sure, we can revisit this in terms of our new requirements. 2- I was wondering if we need consistency between https://github.com/apache/arrow-datafusion/blob/main/datafusion/functions-array/src/empty.rs#L33 and https://github.com/apache/arrow-datafusion/blob/main/datafusion/functions-array/src/empty.rs#L58. In general, we use array_* prefix for array functions naming convention so should we rename empty with array_empty by aligning with other array functions? It is not big deal, just would double check.

select empty(arrow_cast(make_array(1), 'LargeList(Int64)'));
----
false

#7290
The name comes from clickhouse, so I think it is fine to name it without prefixarray, but I think we can have an alias as array_empty, I dont have strong opinion on which should be the default name. I will name it array_empty if clickhouse does not have the function. We usually follow the name and behavior from postgres, duckdb, clickhouse or others

@erenavsarogullari
Copy link
Member Author

erenavsarogullari commented Mar 25, 2024

Yes, makes sense to follow industry standard. I can create an issue in terms of our direction and cover the fix, test coverage and documentation.

@erenavsarogullari
Copy link
Member Author

erenavsarogullari commented Mar 26, 2024

@jayzhan211 please find array_empty and list_empty functions support as alias to empty function: https://github.com/erenavsarogullari/arrow-datafusion/commit/1402a48175470d0e996dcec460eb89b4c9d1e313. I can submit PR if the direction makes sense. Thanks for early feedback.

@jayzhan211
Copy link
Contributor

@jayzhan211 please find array_empty and list_empty functions support as alias to empty function: erenavsarogullari@1402a48. I can submit PR if the direction makes sense. Thanks for early feedback.

It makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants