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

[VL] support array_join #2705

Merged
merged 1 commit into from
Aug 15, 2023
Merged

[VL] support array_join #2705

merged 1 commit into from
Aug 15, 2023

Conversation

majian4work
Copy link
Contributor

What changes were proposed in this pull request?

(Please fill in changes proposed in this fix)

(Fixes: #ISSUE-ID)

How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)

(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

@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/oap-project/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
Copy link

Run Gluten Clickhouse CI

@github-actions
Copy link

Run Gluten Clickhouse CI

@majian4work
Copy link
Contributor Author

@PHILO-HE Could you review this PR?

@github-actions
Copy link

Run Gluten Clickhouse CI

PHILO-HE
PHILO-HE previously approved these changes Aug 11, 2023
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.

LGTM.

@@ -658,6 +658,14 @@ object ExpressionConverter extends SQLConfHelper with Logging {
substraitExprName.get,
replaceWithExpressionTransformer(arrayMin.child, attributeSeq),
arrayMin)
case arrayJoin: ArrayJoin =>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be necessary, as it will be handled by normalFunctionExpressionTransform

Copy link
Contributor

Choose a reason for hiding this comment

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

@Ma-Jian1, please have a further check. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll double check it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's covered by normalFunctionExpressionTransform, revert it.

@@ -658,6 +658,14 @@ object ExpressionConverter extends SQLConfHelper with Logging {
substraitExprName.get,
replaceWithExpressionTransformer(arrayMin.child, attributeSeq),
arrayMin)
case arrayJoin: ArrayJoin =>
Copy link
Contributor

Choose a reason for hiding this comment

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

@Ma-Jian1, please have a further check. Thanks!

@@ -95,6 +95,7 @@ object CHExpressionUtil {
)

final val CH_BLACKLIST_SCALAR_FUNCTION: Map[String, FunctionValidator] = Map(
ARRAY_JOIN -> DefaultValidator(),
Copy link
Contributor

Choose a reason for hiding this comment

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

@exmy, @zzcclp, please note this change for CH.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's ok.

@github-actions
Copy link

Run Gluten Clickhouse CI

@github-actions
Copy link

Run Gluten Clickhouse CI

@github-actions
Copy link

Run Gluten Clickhouse CI

@github-actions
Copy link

Run Gluten Clickhouse CI

@github-actions
Copy link

Run Gluten Clickhouse CI

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.

LGTM. Assume it is covered by the imported spark UT.

@PHILO-HE PHILO-HE merged commit 600ed59 into apache:main Aug 15, 2023
@majian4work majian4work deleted the array_join branch August 15, 2023 01:48
facebook-github-bot pushed a commit to facebookincubator/velox that referenced this pull request Jan 6, 2025
Summary:
Gluten removed the registration of Presto sql functions. This PR registers
Presto array_join function in Spark for reuse.

apache/incubator-gluten#2705

Pull Request resolved: #11948

Reviewed By: bikramSingh91

Differential Revision: D67867310

Pulled By: kevinwilfong

fbshipit-source-id: 14b18faa7b46802741b58de4ca5e03d5979137d4
athmaja-n pushed a commit to athmaja-n/velox that referenced this pull request Jan 10, 2025
Summary:
Gluten removed the registration of Presto sql functions. This PR registers
Presto array_join function in Spark for reuse.

apache/incubator-gluten#2705

Pull Request resolved: facebookincubator#11948

Reviewed By: bikramSingh91

Differential Revision: D67867310

Pulled By: kevinwilfong

fbshipit-source-id: 14b18faa7b46802741b58de4ca5e03d5979137d4
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.

3 participants