Skip to content

[CH-320] Support space function - #1054

Merged
zzcclp merged 2 commits into
apache:mainfrom
KevinyhZou:support_space_function
Apr 19, 2023
Merged

[CH-320] Support space function#1054
zzcclp merged 2 commits into
apache:mainfrom
KevinyhZou:support_space_function

Conversation

@KevinyhZou

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Support space function for clickhouse
the releated pr: Kyligence/ClickHouse#321
the realted issue: Kyligence/ClickHouse#320

How was this patch tested?

by add unit test in GlutenClickHouseTPCHSUite.scala

@KevinyhZou KevinyhZou changed the title [CH-320]support space function [CH-320] Support space function Mar 1, 2023
@github-actions

github-actions Bot commented Mar 1, 2023

Copy link
Copy Markdown

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/oap-project/gluten/issues

Then could you also rename commit message and pull request title in the following format?

[Gluten-${ISSUES_ID}] ${detailed message}

See also:

@zzcclp

zzcclp commented Mar 20, 2023

Copy link
Copy Markdown
Contributor

it needs to modify ch backend to support this function ?
please rebase

@KevinyhZou
KevinyhZou force-pushed the support_space_function branch from d63fa1d to a17b12d Compare March 27, 2023 04:11
@KevinyhZou

KevinyhZou commented Mar 27, 2023

Copy link
Copy Markdown
Contributor Author

@zzcclp rebase done. this function use repeat function to implement, I have sumit a pr to ch to support this function: ClickHouse/ClickHouse#47049, and it was suggested to use repeat function replace this.

@zzcclp

zzcclp commented Mar 30, 2023

Copy link
Copy Markdown
Contributor

please rebase and rerun all test case

@KevinyhZou
KevinyhZou force-pushed the support_space_function branch from a17b12d to bfc023c Compare April 2, 2023 13:37
@kyligence-git

Copy link
Copy Markdown
Contributor

Can one of the admins verify this patch?

@KevinyhZou

Copy link
Copy Markdown
Contributor Author

done @zzcclp

@KevinyhZou
KevinyhZou force-pushed the support_space_function branch from ae78a21 to 30196f7 Compare April 18, 2023 06:43

@zzcclp zzcclp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@zzcclp

zzcclp commented Apr 19, 2023

Copy link
Copy Markdown
Contributor

BTW, @rui-mo please help to check whether velox supports this function ?

@rui-mo

rui-mo commented Apr 19, 2023

Copy link
Copy Markdown
Contributor

BTW, @rui-mo please help to check whether velox supports this function ?

@zzcclp Looks not yet. It's OK to add it in Gluten because it will fallback if there is no registration for this function in Velox.

@zzcclp

zzcclp commented Apr 19, 2023

Copy link
Copy Markdown
Contributor

BTW, @rui-mo please help to check whether velox supports this function ?

@zzcclp Looks not yet. It's OK to add it in Gluten because it will fallback if there is no registration for this function in Velox.

does it need to add this function into VELOX_EXPR_BLACKLIST ?

@rui-mo

rui-mo commented Apr 19, 2023

Copy link
Copy Markdown
Contributor

does it need to add this function into VELOX_EXPR_BLACKLIST ?

@zzcclp That is only temporarily needed for those implemented in Velox but have different semantics or results. For the unimplemented ones, a search to the function registration map will help reject.

@zzcclp
zzcclp merged commit de4250e into apache:main Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants