Add `VELOX_BUILD_MINIMAL_WITH_DWIO` compilation option #8682

pedroerp · 2024-02-06T05:19:21Z

VELOX_BUILD_MINIMAL_WITH_DWIO allows developers using Velox to compile only dwio (in addition to Velox minimal), but without pulling all other dependencies and internal libraries (exec, connectors, parser, aggregates, storage adapters, etc).

netlify · 2024-02-06T05:19:28Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`d682418`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/65cb97d6f011ad0008161e72

kgpai

Looks good, some nits..

CMakeLists.txt

kgpai · 2024-02-06T05:46:41Z

CMakeLists.txt

  set(VELOX_ENABLE_EXAMPLES ON)
 endif()

+if(${VELOX_BUILD_DWIO})
+  set(VELOX_ENABLE_DWIO ON)
+endif()


nit: What if we have contradictory build flags - say VELOX_ENABLE_HIVE_CONNECTOR ON, but VELOX_ENABLE_DWIO OFF ,should we check for that ?

@kgpai Good question. These flags are a bit all over the place today. I can add a check here, but it seems like we should separate the flags we use internally to control which directories we compile, from the external facing flags that users can toggle, since in many cases they can be incompatible.

Something like this, for example, won't work as expected today:

$ EXTRA_CMAKE_FLAGS=" -DVELOX_BUILD_MINIMAL=ON -DVELOX_ENABLE_HIVE_CONNECTOR=ON" make

Since in line 126 we will overwrite the enable hive connector flag to OFF. And even if it does, hive needs exec and the compression libraries, which won't be available.

Point being that users can't really (or shouldn't) tweak VELOX_ENABLE_HIVE_CONNECTOR manually.

Cc: @assignUser @majetideepak

I added the check in any case since is likely a longer discussion.

Yes, reorganizing the options and also giving some nice output at the beginning of the configure phase etc. is on my list but atm lower priority. Adding CMakePresets.json would also be good (partially replaces makefile, get's picked up by IDEs...).

majetideepak

@pedroerp what is the difference between VELOX_BUILD_DWIO and VELOX_ENABLE_DWIO? Can we have just one?

pedroerp · 2024-02-06T16:31:15Z

Makefile

 	$(MAKE) cmake BUILD_DIR=debug BUILD_TYPE=debug EXTRA_CMAKE_FLAGS="${EXTRA_CMAKE_FLAGS} -DVELOX_BUILD_MINIMAL=ON"
 	$(MAKE) build BUILD_DIR=debug

+min_debug: minimal_debug


for backward compatibility

assignUser

LGTM

assignUser · 2024-02-06T17:14:25Z

CMakeLists.txt

  set(VELOX_ENABLE_EXAMPLES ON)
 endif()

+if(${VELOX_BUILD_DWIO})
+  set(VELOX_ENABLE_DWIO ON)
+endif()


Yes, reorganizing the options and also giving some nice output at the beginning of the configure phase etc. is on my list but atm lower priority. Adding CMakePresets.json would also be good (partially replaces makefile, get's picked up by IDEs...).

CMakeLists.txt

pedroerp · 2024-02-06T17:25:32Z

@pedroerp what is the difference between VELOX_BUILD_DWIO and VELOX_ENABLE_DWIO? Can we have just one?

@majetideepak Unfortunately, we can't because of the problem I described here:

https://github.com/facebookincubator/velox/pull/8682/files#r1480059051

majetideepak · 2024-02-07T00:12:48Z

Unfortunately, we can't because of the problem I described here:

@pedroerp I don't think we need two CMake options. We can have one for external usage. The current pattern is to use VELOX_ENABLE_* (like VELOX_ENABLE_PARQUET). For internal usage, we can use set(). For example, set(VELOX_ENABLE_DUCKDB ON) is defined based on whether testing or test utils are on.
There is no easy way to avoid conflicting options except to disallow them or have checks. In your example, we should ideally remove VELOX_ENABLE_HIVE_CONNECTOR option since that cannot exist on its own.
We could set(VELOX_ENABLE_HIVE_CONNECTOR ON) if either of DWRF, Parquet, or storage adapters options are on.

pedroerp · 2024-02-12T22:01:39Z

@majetideepak Maybe I'm missing something, but I think that there are two different concepts here:

One wants to enable a "DWIO compilation mode", which basically enables everything needed by DWIO, and disables everything else not needed (storage adapters, examples, benchmarks, tests, etc). This is what I'm trying to achieve.
One just wants to make sure DWIO is enabled (along with its dependencies). This is the VELOX_ENABLE_PARQUET behavior.

The problem is that by default we build a lot of things, so if I just wanted to build enough to support DWIO (1), with a single flag, I would need to manually turn OFF every other flag. Likely I won't get the entire list, and more could be added in the future, adding stuff I don't need to my compilation time.

So with a single flag, what one would naturally try to do is to enable both VELOX_MINIMAL and VELOX_ENABLE_DWIO. This, counter-intuitively, won't do the job as VELOX_MINIMAL will effectively disable DWIO and overwrite the flag the user is enabled trying to enable.

Cc: @assignUser @kgpai

majetideepak · 2024-02-13T02:57:57Z

@pedroerp In this PR, VELOX_ENABLE_DWIO can simply be a CMake variable set(VELOX_ENABLE_DWIO, ON). It is mainly controlled by other options (VELOX_BUILD_MINIMAL, VELOX_ENABLE_HIVE_CONNECTOR, VELOX_BUILD_DWIO). VELOX_ENABLE_PARQUET is not controlled by any other option.
I also see that the compression libraries will be disabled if VELOX_ENABLE_DWIO = OFF and VELOX_ENABLE_PARQUET=ON which is a problem.
I feel the current solution is adding more to the already broken semantics.
If you want 1) .. enables everything needed by DWIO, and disables everything else ..., let's add VELOX_BUILD_MINIMAL_WITH_DWIO and make it explicit similar to VELOX_BUILD_MINIMAL.

majetideepak · 2024-02-13T03:09:07Z

There is value in 2) for users who want to disable DWRF and enable only PARQUET. We could add option(VELOX_ENABLE_DWRF "Build dwrf (file format readers/writers)." ON) similar to VELOX_ENABLE_PARQUET. Users will have to disable VELOX_ENABLE_DWRF and enable VELOX_ENABLE_PARQUET.
The compression libraries and adapters will depend on VELOX_ENABLE_HIVE_CONNECTOR which has to tie into these options.

assignUser · 2024-02-13T03:36:28Z

Both of your approaches work but as I don't know what the normal use cases would be/which scenarios are more likely. There is a better builtin solution to linking option than just manually set()ing them: https://cmake.org/cmake/help/latest/module/CMakeDependentOption.html but refactoring everything to use this is probably out of scope for this PR?

pedroerp · 2024-02-13T04:52:45Z

I also see that the compression libraries will be disabled if VELOX_ENABLE_DWIO = OFF and VELOX_ENABLE_PARQUET=ON which is a problem.

Parquet is part of dwio, so VELOX_ENABLE_PARQUET should imply VELOX_ENABLE_DWIO. I'll add a check to enforce this.

if you want 1) .. enables everything needed by DWIO, and disables everything else ..., let's add VELOX_BUILD_MINIMAL_WITH_DWIO and make it explicit similar to VELOX_BUILD_MINIMAL.

That was my intent with the VELOX_BUILD_DWIO flag, but we can to rename it to VELOX_BUILD_MINIMAL_WITH_DWIO like you suggested to make it more explicit.

The compression libraries and adapters will depend on VELOX_ENABLE_HIVE_CONNECTOR which has to tie into these options.

Hive connector has a dependency on DWIO (since it needs file readers/writers), but AFAIK no direct dependency on compression libraries. This was only done because hive connector and dwio were bundled together.

@majetideepak

majetideepak · 2024-02-13T05:13:22Z

@pedroerp let's remove VELOX_ENABLE_DWIO option since similar to VELOX_ENABLE_HIVE_CONNECTOR it conflicts with other options.
For your need, VELOX_BUILD_MINIMAL_WITH_DWIO should suffice.

majetideepak · 2024-02-13T05:16:12Z

Long term as @assignUser suggested, we should use https://cmake.org/cmake/help/latest/module/CMakeDependentOption.html and tighten the semantics. Adding checks is not scalable.

majetideepak

Thanks, @pedroerp

majetideepak · 2024-02-13T16:15:44Z

CMakeLists.txt

@@ -467,7 +476,11 @@ else()
  set(FOLLY_BENCHMARK Folly::follybenchmark)
 endif()

-if(NOT ${VELOX_BUILD_MINIMAL})
+# WIO (ORC/DWRF), Substrait and experimental/codegen depend on protobuf.


`VELOX_BUILD_MINIMAL_WITH_DWIO` allows developers using Velox to compile only dwio (in addition to Velox minimal), but without pulling all other dependencies and internal libraries (exec, connectors, etc).

facebook-github-bot · 2024-02-13T21:18:27Z

@pedroerp has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-02-13T23:54:36Z

@pedroerp merged this pull request in f0583e7.

conbench-facebook · 2024-02-14T00:17:07Z

Conbench analyzed the 1 benchmark run on commit f0583e76.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 6, 2024

pedroerp force-pushed the velox-dwio branch from eee1add to 2b8fb8d Compare February 6, 2024 05:23

pedroerp requested review from kgpai, majetideepak and assignUser February 6, 2024 05:23

pedroerp force-pushed the velox-dwio branch from 2b8fb8d to 840dc24 Compare February 6, 2024 05:25

kgpai approved these changes Feb 6, 2024

View reviewed changes

majetideepak reviewed Feb 6, 2024

View reviewed changes

pedroerp force-pushed the velox-dwio branch 2 times, most recently from 2e33a79 to b79e967 Compare February 6, 2024 16:29

pedroerp commented Feb 6, 2024

View reviewed changes

assignUser approved these changes Feb 6, 2024

View reviewed changes

pedroerp force-pushed the velox-dwio branch from b79e967 to 3c7d483 Compare February 6, 2024 17:23

pedroerp force-pushed the velox-dwio branch from 3c7d483 to fb3235e Compare February 9, 2024 05:17

pedroerp force-pushed the velox-dwio branch 2 times, most recently from daea9b2 to f191d9c Compare February 13, 2024 16:15

majetideepak approved these changes Feb 13, 2024

View reviewed changes

Add VELOX_BUILD_MINIMAL_WITH_DWIO compilation option

d682418

`VELOX_BUILD_MINIMAL_WITH_DWIO` allows developers using Velox to compile only dwio (in addition to Velox minimal), but without pulling all other dependencies and internal libraries (exec, connectors, etc).

pedroerp force-pushed the velox-dwio branch from f191d9c to d682418 Compare February 13, 2024 16:24

pedroerp changed the title ~~Add VELOX_BUILD_DWIO compilation option~~ Add VELOX_BUILD_MINIMAL_WITH_DWIO compilation option Feb 13, 2024

Yuhta approved these changes Feb 13, 2024

View reviewed changes

facebook-github-bot closed this in f0583e7 Feb 13, 2024

facebook-github-bot added the Merged label Feb 13, 2024

FelixYBW mentioned this pull request Feb 14, 2024

[VL] Daily Update Velox Version (2024-02-14) apache/incubator-gluten#4693

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `VELOX_BUILD_MINIMAL_WITH_DWIO` compilation option #8682

Add `VELOX_BUILD_MINIMAL_WITH_DWIO` compilation option #8682

pedroerp commented Feb 6, 2024 •

edited

Loading

netlify bot commented Feb 6, 2024 •

edited

Loading

kgpai left a comment

kgpai Feb 6, 2024

pedroerp Feb 6, 2024 •

edited

Loading

pedroerp Feb 6, 2024

assignUser Feb 6, 2024

majetideepak left a comment

pedroerp Feb 6, 2024

assignUser left a comment

assignUser Feb 6, 2024

pedroerp commented Feb 6, 2024 •

edited

Loading

majetideepak commented Feb 7, 2024 •

edited

Loading

pedroerp commented Feb 12, 2024 •

edited

Loading

majetideepak commented Feb 13, 2024 •

edited

Loading

majetideepak commented Feb 13, 2024 •

edited

Loading

assignUser commented Feb 13, 2024

pedroerp commented Feb 13, 2024

majetideepak commented Feb 13, 2024

majetideepak commented Feb 13, 2024

majetideepak left a comment

majetideepak Feb 13, 2024

facebook-github-bot commented Feb 13, 2024

facebook-github-bot commented Feb 13, 2024

conbench-facebook bot commented Feb 14, 2024

Add VELOX_BUILD_MINIMAL_WITH_DWIO compilation option #8682

Add VELOX_BUILD_MINIMAL_WITH_DWIO compilation option #8682

Conversation

pedroerp commented Feb 6, 2024 • edited Loading

netlify bot commented Feb 6, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

kgpai left a comment

Choose a reason for hiding this comment

kgpai Feb 6, 2024

Choose a reason for hiding this comment

pedroerp Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

pedroerp Feb 6, 2024

Choose a reason for hiding this comment

assignUser Feb 6, 2024

Choose a reason for hiding this comment

majetideepak left a comment

Choose a reason for hiding this comment

pedroerp Feb 6, 2024

Choose a reason for hiding this comment

assignUser left a comment

Choose a reason for hiding this comment

assignUser Feb 6, 2024

Choose a reason for hiding this comment

pedroerp commented Feb 6, 2024 • edited Loading

majetideepak commented Feb 7, 2024 • edited Loading

pedroerp commented Feb 12, 2024 • edited Loading

majetideepak commented Feb 13, 2024 • edited Loading

majetideepak commented Feb 13, 2024 • edited Loading

assignUser commented Feb 13, 2024

pedroerp commented Feb 13, 2024

majetideepak commented Feb 13, 2024

majetideepak commented Feb 13, 2024

majetideepak left a comment

Choose a reason for hiding this comment

majetideepak Feb 13, 2024

Choose a reason for hiding this comment

facebook-github-bot commented Feb 13, 2024

facebook-github-bot commented Feb 13, 2024

conbench-facebook bot commented Feb 14, 2024

Add `VELOX_BUILD_MINIMAL_WITH_DWIO` compilation option #8682

Add `VELOX_BUILD_MINIMAL_WITH_DWIO` compilation option #8682

pedroerp commented Feb 6, 2024 •

edited

Loading

netlify bot commented Feb 6, 2024 •

edited

Loading

pedroerp Feb 6, 2024 •

edited

Loading

pedroerp commented Feb 6, 2024 •

edited

Loading

majetideepak commented Feb 7, 2024 •

edited

Loading

pedroerp commented Feb 12, 2024 •

edited

Loading

majetideepak commented Feb 13, 2024 •

edited

Loading

majetideepak commented Feb 13, 2024 •

edited

Loading