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

[GLUTEN-3572][VL] Remove --arrow_home option and fix "libarrow not found" in debug mode #3573

Merged
merged 7 commits into from
Nov 8, 2023

Conversation

Surbhi-Vijay
Copy link
Contributor

@Surbhi-Vijay Surbhi-Vijay commented Oct 31, 2023

What changes were proposed in this pull request?

How was this patch tested?

Tested manually by running the script

@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:

@Surbhi-Vijay Surbhi-Vijay changed the title Update documentation [GLUTEN-3572][VL]Bug fix: compile.sh script fails with libarrow not found when built with Debug flag. Oct 31, 2023
@github-actions
Copy link

#3572

@PHILO-HE
Copy link
Contributor

PHILO-HE commented Nov 2, 2023

Hi @Surbhi-Vijay, thanks for your fix!
I think --arrow_home option can be removed since currently we just use arrow binary built out by velox. There may be no need to allow users to specify their own arrow home. We can just infer ARROW_HOME according to VELOX_HOME.

@Surbhi-Vijay
Copy link
Contributor Author

Yes @PHILO-HE I was also thinking the same. But I did not remove it, thinking if we need it for testing purposes.
I will go ahead and remove all --arrow_home usage. Please let me know if you see any issues with it

@PHILO-HE
Copy link
Contributor

PHILO-HE commented Nov 2, 2023

In old version, gluten's required arrow version is different from velox's. And we once needed to let gluten build arrow from source for its own use. Now a unified version built out by velox is used, so there is no need to keep --arrow_home for gluten.

Please go ahead to remove it and update document. Thanks!

Copy link

github-actions bot commented Nov 2, 2023

Run Gluten Clickhouse CI

@Surbhi-Vijay
Copy link
Contributor Author

@PHILO-HE @rui-mo I see that modify_arrow.patch and modify_velox.patch is only applied in build_velox.sh and not in builddeps-veloxbe.sh. Is this patch required in certain cases only ?

@rui-mo
Copy link
Contributor

rui-mo commented Nov 2, 2023

@Surbhi-Vijay Build_velox.sh should be called by builddeps-veloxbe.sh.
https://github.com/oap-project/gluten/blob/main/dev/builddeps-veloxbe.sh#L105-L106

@Surbhi-Vijay
Copy link
Contributor Author

@Surbhi-Vijay Build_velox.sh should be called by builddeps-veloxbe.sh. https://github.com/oap-project/gluten/blob/main/dev/builddeps-veloxbe.sh#L105-L106

oh yes! missed out on it. Thanks for clarifying.

Copy link

github-actions bot commented Nov 2, 2023

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Nov 2, 2023

Run Gluten Clickhouse CI

@PHILO-HE PHILO-HE changed the title [GLUTEN-3572][VL]Bug fix: compile.sh script fails with libarrow not found when built with Debug flag. [GLUTEN-3572][VL] Remove --arrow_home option and fix "libarrow not found" in debug mode Nov 3, 2023
docs/get-started/Velox.md Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Nov 6, 2023

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Nov 7, 2023

Run Gluten Clickhouse CI

@Surbhi-Vijay
Copy link
Contributor Author

@PHILO-HE Please let me know if there are any open comments to address.

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.

Looks good! Thanks so much for your contribution!

@PHILO-HE PHILO-HE merged commit 29b5899 into apache:main Nov 8, 2023
17 checks passed
@GlutenPerfBot
Copy link
Contributor

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

query log/native_3573_time.csv log/native_master_11_07_2023_e3eff1d8f_time.csv difference percentage
q1 33.56 34.38 0.817 102.43%
q2 25.17 25.03 -0.148 99.41%
q3 37.75 38.14 0.399 101.06%
q4 35.71 37.57 1.857 105.20%
q5 72.21 71.50 -0.708 99.02%
q6 7.64 6.26 -1.380 81.94%
q7 85.18 82.22 -2.954 96.53%
q8 87.34 86.95 -0.388 99.56%
q9 120.75 119.81 -0.946 99.22%
q10 52.60 51.26 -1.337 97.46%
q11 19.62 19.73 0.114 100.58%
q12 24.75 24.39 -0.358 98.55%
q13 48.96 50.30 1.338 102.73%
q14 18.85 17.67 -1.188 93.70%
q15 30.53 30.35 -0.175 99.43%
q16 16.22 16.20 -0.019 99.88%
q17 100.68 101.51 0.830 100.82%
q18 148.56 148.26 -0.303 99.80%
q19 15.10 16.17 1.072 107.10%
q20 30.21 30.31 0.103 100.34%
q21 223.92 224.88 0.959 100.43%
q22 13.21 14.08 0.871 106.59%
total 1248.52 1246.98 -1.543 99.88%

@GlutenPerfBot
Copy link
Contributor

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

query log/native_master_11_08_2023_time.csv log/native_master_11_07_2023_e3eff1d8f_time.csv difference percentage
q1 34.11 34.38 0.266 100.78%
q2 25.13 25.03 -0.104 99.59%
q3 38.62 38.14 -0.474 98.77%
q4 36.07 37.57 1.498 104.15%
q5 71.84 71.50 -0.339 99.53%
q6 7.59 6.26 -1.332 82.45%
q7 85.82 82.22 -3.595 95.81%
q8 86.97 86.95 -0.025 99.97%
q9 121.86 119.81 -2.054 98.31%
q10 54.80 51.26 -3.542 93.54%
q11 20.59 19.73 -0.859 95.83%
q12 24.63 24.39 -0.245 99.01%
q13 49.95 50.30 0.341 100.68%
q14 16.99 17.67 0.673 103.96%
q15 30.97 30.35 -0.615 98.01%
q16 16.51 16.20 -0.311 98.12%
q17 102.40 101.51 -0.890 99.13%
q18 148.28 148.26 -0.014 99.99%
q19 15.13 16.17 1.037 106.85%
q20 29.97 30.31 0.342 101.14%
q21 222.20 224.88 2.682 101.21%
q22 13.55 14.08 0.534 103.94%
total 1254.01 1246.98 -7.024 99.44%

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