incubator-gluten icon indicating copy to clipboard operation
incubator-gluten copied to clipboard

[GLUTEN-6887][VL] switch to upstream Velox

Open zhouyuan opened this issue 1 year ago • 5 comments

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)

zhouyuan avatar Oct 14 '24 06:10 zhouyuan

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

github-actions[bot] avatar Oct 14 '24 06:10 github-actions[bot]

CC @PHILO-HE @zhztheplayer

zhouyuan avatar Oct 14 '24 06:10 zhouyuan

There are several things to do before we switch to upstream:

  1. 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.
  2. We still need some where to easily track our changes, like the link above. git apply isn't good because the batch is hard to maintain.
  3. Let's switch to git submodule
  4. there are some sed -i command in our build-velox.sh. Let's take this opportunity to put them together as other commits.

FelixYBW avatar Oct 14 '24 20:10 FelixYBW

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.

zhztheplayer avatar Oct 15 '24 01:10 zhztheplayer

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

zhouyuan avatar Oct 15 '24 01:10 zhouyuan

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[bot] avatar Dec 07 '24 02:12 github-actions[bot]

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

FelixYBW avatar Dec 16 '24 21:12 FelixYBW

@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

zhouyuan avatar Dec 17 '24 13:12 zhouyuan

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.

FelixYBW avatar Dec 17 '24 19:12 FelixYBW

  1. Let's switch to git submodule

+1 on git submodule, and move velox under cpp folder

Yohahaha avatar Dec 18 '24 01:12 Yohahaha

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.

Yohahaha avatar Dec 18 '24 01:12 Yohahaha

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[bot] avatar Feb 01 '25 01:02 github-actions[bot]

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[bot] avatar Feb 12 '25 01:02 github-actions[bot]