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 regexp_replace function with position argument #4411

Merged
merged 2 commits into from
Mar 20, 2024

Conversation

PHILO-HE
Copy link
Contributor

@PHILO-HE PHILO-HE commented Jan 16, 2024

What changes were proposed in this pull request?

In the main code, gluten will make regexp_replace fall back if its last pos arg is specified by user with a non-default value. We note upstream velox has a fix for this function. So we are removing such fallback for velox backend.

This PR depends on a velox PR which fixes newly found issues: facebookincubator/velox#8387

How was this patch tested?

Existing spark UTs.

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:

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

1 similar comment
Copy link

Run Gluten Clickhouse CI

@marin-ma
Copy link
Contributor

Could you update the PR description? Thanks!

Copy link

Run Gluten Clickhouse CI

1 similar comment
Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

2 similar comments
Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

@FelixYBW
Copy link
Contributor

@PHILO-HE Sorry, I just synced with @codyschierbeck. regexp_replace in Velox isn't ready yet. Let's wait until his PR is merged

@PHILO-HE
Copy link
Contributor Author

Let's wait for upstream velox pr facebookincubator/velox#8333.

Copy link

github-actions bot commented Mar 5, 2024

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale stale label Mar 5, 2024
Copy link

This PR was auto-closed because it has been stalled for 10 days with no activity. Please feel free to reopen if it is still valid. Thanks.

@github-actions github-actions bot closed this Mar 15, 2024
@PHILO-HE PHILO-HE reopened this Mar 15, 2024
Copy link

Run Gluten Clickhouse CI

@github-actions github-actions bot removed the stale stale label Mar 16, 2024
@PHILO-HE PHILO-HE force-pushed the fix-regexp_replace branch from ad4c9f8 to 1b8e47a Compare March 19, 2024 08:11
Copy link

Run Gluten Clickhouse CI

@PHILO-HE PHILO-HE force-pushed the fix-regexp_replace branch from 1b8e47a to cca720c Compare March 19, 2024 08:41
Copy link

Run Gluten Clickhouse CI

1 similar comment
Copy link

Run Gluten Clickhouse CI

@PHILO-HE PHILO-HE force-pushed the fix-regexp_replace branch from c513c42 to 525eb54 Compare March 20, 2024 02:25
Copy link

Run Gluten Clickhouse CI

@PHILO-HE
Copy link
Contributor Author

As facebookincubator/velox#8387 has been merged, we can land this pr. @marin-ma, do you have any other comment? Thanks!

Copy link
Contributor

@marin-ma marin-ma 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] Fix regexp_replace function [VL] Support regexp_replace function with position argument Mar 20, 2024
@PHILO-HE PHILO-HE merged commit 92bdfd5 into apache:main Mar 20, 2024
20 of 21 checks passed
@GlutenPerfBot
Copy link
Contributor

===== Performance report for TPCH SF2000 with Velox backend, for reference only ====

query log/native_4411_time.csv log/native_master_03_19_2024_e977d79e3_time.csv difference percentage
q1 34.35 36.21 1.861 105.42%
q2 23.94 24.11 0.168 100.70%
q3 38.37 36.64 -1.726 95.50%
q4 37.76 38.02 0.256 100.68%
q5 69.08 69.07 -0.009 99.99%
q6 7.32 5.91 -1.416 80.66%
q7 83.27 82.31 -0.961 98.85%
q8 83.28 85.26 1.984 102.38%
q9 122.87 121.93 -0.941 99.23%
q10 44.35 45.61 1.258 102.84%
q11 19.79 20.57 0.783 103.96%
q12 29.14 28.32 -0.822 97.18%
q13 47.30 47.17 -0.129 99.73%
q14 17.16 16.86 -0.303 98.23%
q15 30.64 31.19 0.551 101.80%
q16 13.80 16.26 2.463 117.84%
q17 99.66 99.14 -0.514 99.48%
q18 140.62 142.77 2.152 101.53%
q19 15.46 14.68 -0.774 95.00%
q20 27.14 28.29 1.150 104.24%
q21 226.30 227.96 1.664 100.74%
q22 13.82 14.00 0.183 101.33%
total 1225.40 1232.28 6.876 100.56%

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.

4 participants