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.

Sign up for GitHub

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

Jump to bottom

[VL] Ensure get(GetArrayItem) function is offloaded #5789

Merged
merged 1 commit into from
May 21, 2024
Merged

[VL] Ensure get(GetArrayItem) function is offloaded #5789

merged 1 commit into from
May 21, 2024

Conversation

Yohahaha
Copy link
Contributor

@Yohahaha Yohahaha commented May 17, 2024 •
edited

Support get(GetArrayItem) function with native implementation in Velox backend, and move original implementation to CH module.

@github-actions GitHub Actions
Copy link

Thanks for opening a pull request!

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

https://github.com/apache/incubator-gluten/issues

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

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

jackylee-ch
jackylee-ch previously approved these changes May 17, 2024
View reviewed changes
@Yohahaha
Copy link
Contributor Author

please do not merge, GetArrayItemTransformer need refactor.

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

3 similar comments
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@FelixYBW FelixYBW changed the title [VL] Ensure get(GetArrayItem) function offloaded [DNM] [VL] Ensure get(GetArrayItem) function offloaded May 17, 2024
@FelixYBW
Copy link
Contributor

please do not merge, GetArrayItemTransformer need refactor.

marked in title. Feel free to remove once refactor is done.

@Yohahaha Yohahaha changed the title [DNM] [VL] Ensure get(GetArrayItem) function offloaded [VL] Ensure get(GetArrayItem) function offloaded May 19, 2024
@Yohahaha
Copy link
Contributor Author

I move the original GetArrayItem implementation into CH module since velox does not need anymore. cc @taiyang-li @zzcclp

@Yohahaha
Copy link
Contributor Author

@PHILO-HE @jackylee-ch would you help review again? thank you!

jackylee-ch
jackylee-ch previously approved these changes May 20, 2024
View reviewed changes
Copy link
Contributor

@jackylee-ch jackylee-ch left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@Yohahaha
fix
153db04 
fix

fix

fix

Signed-off-by: Yang Zhang <yangchuan.zy@alibaba-inc.com>
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

PHILO-HE
PHILO-HE approved these changes May 20, 2024
View reviewed changes
Copy link
Contributor

@PHILO-HE PHILO-HE left a comment

Choose a reason for hiding this comment

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

Thanks!

@PHILO-HE PHILO-HE changed the title [VL] Ensure get(GetArrayItem) function offloaded [VL] Ensure get(GetArrayItem) function is offloaded May 21, 2024
@PHILO-HE PHILO-HE merged commit ef26a2e into apache:main May 21, 2024
45 checks passed
@Yohahaha Yohahaha deleted the ensre-get-offloaded branch May 21, 2024 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackylee-ch jackylee-ch jackylee-ch approved these changes

@PHILO-HE PHILO-HE PHILO-HE approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Yohahaha @FelixYBW @jackylee-ch @PHILO-HE