[GLUTEN-6887][VL] switch to upstream Velox
What changes were proposed in this pull request?
This patch changed from OAP/Velox to Meta/Velox + several Gluten patches
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)
https://github.com/apache/incubator-gluten/issues/6887
CC @PHILO-HE @zhztheplayer
There are several things to do before we switch to upstream:
- Upstream our oap/velox commit to Velox as much as possible. there are still 11 commits now: https://github.com/facebookincubator/velox/compare/main...oap-project:velox:update. See if we can ping the author of those PRs.
- We still need some where to easily track our changes, like the link above.
git applyisn't good because the batch is hard to maintain. - Let's switch to git submodule
- there are some
sed -icommand in our build-velox.sh. Let's take this opportunity to put them together as other commits.
there are some sed -i command in our build-velox.sh. Let's take this opportunity to put them together as other commits.
+1
Let's switch to git submodule
I heard some voices from other open source projects that git submodule has comparatively steep curve to work with. Perhaps we don't have to make the decision right now and lower the priority of doing that.
Hi @FelixYBW thank you for the inputs, indeed this is just a draft to check how far before can we move to the direction - will keep looking on this
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.
@zhouyuan should we replace the 'sed -i xxx' in https://github.com/apache/incubator-gluten/blob/main/ep/build-velox/src/get_velox.sh as a commit of OAP velox?
Also the changes in https://github.com/apache/incubator-gluten/blob/main/ep/build-velox/src/modify_velox.patch
Then let's try to upstream the commits as much as possible. Let's see what's the left patches and define the next step.
@zhouyuan should we replace the 'sed -i xxx' in https://github.com/apache/incubator-gluten/blob/main/ep/build-velox/src/get_velox.sh as a commit of OAP velox?
Also the changes in https://github.com/apache/incubator-gluten/blob/main/ep/build-velox/src/modify_velox.patch
Then let's try to upstream the commits as much as possible. Let's see what's the left patches and define the next step.
Yes, in general the private changes are reduced to decimal avg, parquet struct reader, timestamp support - which requires more time on align on the design/code with Velox community
The changes in get_velox.sh is not maintained well, some of them are not used any more. BTW it's not used if you are using vcpkg based static build.
The two changes in "modify_velox.patch" is reduced since last month and the rest changes are not supported by Velox community. We may need more time to discuss with them to align.
-yuan
Yes, in general the private changes are reduced to decimal avg, parquet struct reader, timestamp support - which requires more time on align on the design/code with Velox community
Let's follow up the PRs.
The changes in get_velox.sh is not maintained well, some of them are not used any more. BTW it's not used if you are using vcpkg based static build.
Good to know. Let's add comments there. @Yohahaha we are going to stop maintaining the dynamic build script. Currently we still have 1 CI to track the dynamic one.
The two changes in "modify_velox.patch" is reduced since last month and the rest changes are not supported by Velox community. We may need more time to discuss with them to align.
If Velox community doesn't accept it, let's put it as a commit in oap/velox instead of a patch to apply.
- Let's switch to git submodule
+1 on git submodule, and move velox under cpp folder
The changes in get_velox.sh is not maintained well, some of them are not used any more. BTW it's not used if you are using vcpkg based static build.
Good to know. Let's add comments there. @Yohahaha we are going to stop maintaining the dynamic build script. Currently we still have 1 CI to track the dynamic one.
thanks for the reminder, I will keep tracking dynamic build CI.
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.
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.