Remove DuckDB amalgamation from Velox

[aditi-pandit]

Description

DuckDB v0.8.0 has many fixes for issues encountered during Velox testing e.g.

• Window Variance Fuzzer Failure #4350

• DuckDB has different result for Window in fuzzer test #4532

An attempt to upgrade was done in #5082. There were many problems encountered. Adding this issue to resolve these.

[majetideepak]

A proposal from the PR #5082 is to remove DuckDB as an amalgamation and use it as an external dependency.

Inside Velox, we use DuckDB for

1. Differential testing of operators, functions, etc.
2. The dbgen component for TPC-H Connector/TpchGen
3. Parsing expressions

We can achieve "1" via duckdb as an external dependency.
"2" will need the code to be duplicated which should be fine since dbgen is not going to change much.
"3", we need to build our own parser or duplicate code from DuckDB again. This again is not going to change much and should be doable.

@pedroerp @mbasmanova are there other uses of DuckDB amalgamation? What are your thoughts on the above changes?

[pedroerp]

This makes sense to me. I have some familiarity with the dbgen integration, so I could help with #2.

[aditi-pandit]

Converted.

[mbasmanova]

We can achieve "1" via duckdb as an external dependency.

Sounds good to me. CC: @kgpai

"2" will need the code to be duplicated which should be fine since dbgen is not going to change much.

Sounds good as well.

"3", we need to build our own parser or duplicate code from DuckDB again. This again is not going to change much and should be doable.

Curious why this cannot be achieved via (1)? Are we using non-public APIs that won't be accessible? Any estimate of how much work would that be?

[majetideepak]

The standard installation of DuckDB does not provide API to the parser. The amalgamation allows us to use internal headers and components of which parser is a part.
DuckDB parser is located here https://github.com/duckdb/duckdb/tree/master/src/parser
The https://github.com/duckdb/duckdb/tree/master/src#readme says, DuckDB uses the parser of Postgres ([libpg_query](https://github.com/lfittl/libpg_query))
The current flow is that DuckDB uses Postgres to parse a string as Postgres parse tree. It then transforms Postgres parse tree into DuckDB parse tree. The DuckDB parse tree is then transformed into Velox parse tree.
The ideal implementation would be to directly use Postgres and convert Postgres parse tree to Velox parse tree.
One part of the work involves tree traversal and is simple to implement.
The other work would be the dependency build side. That is bring DuckDB as an external dependency and use their third_party libpq_query https://github.com/duckdb/duckdb/tree/master/third_party/libpg_query
They seem to have modified the libpq_query to make it easy to work with.

[mbasmanova]

@majetideepak Thank you for explaining.

The ideal implementation would be to directly use Postgres and convert Postgres parse tree to Velox parse tree.

Actually, a better implementation would use Presto SQL parser, not Postgres and convert it to Velox tree. Maybe we could use this: https://github.com/prestodb/sql

[majetideepak]

https://github.com/prestodb/sql seems to be using javacc to generate the parser. That will be additional set of dependencies.
Definitely something we should consider.

[majetideepak]

I started to look into using https://github.com/prestodb/sql as the parser in Velox. I am inclined to use this since PrestoDB owns this and PrestoDB and Velox have a shared community.

[majetideepak]

I was wrong about the standard installation of DuckDB not providing API to the parser.
It is present here, include/duckdb/parser/parser.hpp.
I was able to add DuckDB as an external dependency here #6725
There are some minor adjustments required. Some of the tests I ran locally seem to work fine.
I want to try this on CI. Looks like there are some CI hurdles to overcome first.

[majetideepak]

Only about 6 CI tests are failing.
https://app.circleci.com/pipelines/github/facebookincubator/velox/34327/workflows/8dace369-21c6-463e-9fd0-3ff589b74fc4/jobs/224118/tests
I did not count the failing ParquetTpchTests because the Duck Parquet extension was not installed in my PR. That dependency is being removed here #6496.

The remaining issue is that the DuckDB make install does not install headers for extensions such as tpch where dbgen lives. Duck extensions are a separate CMake Project. https://github.com/duckdb/duckdb/blob/main/extension/tpch/CMakeLists.txt
My current workaround is to patch the DuckDB extension CMake file https://github.com/facebookincubator/velox/pull/6725/files#diff-b2b07400fa1c3fed2119d3a9d3a6b402ada4f7180dcec801a25dd85efdb3f648

The alternative is to remove the dependency on the Duck tpch extension in Velox. That will require three changes.
Make TPC-H connector standalone. This requires dbgen to be isolated from duckdb. This is the major one.
The TpchBenchmark and ParquetTpchTest depend on the tpch extension to get Tpch queries. We need to add support for std::string getTpchQuery(int queryNo) in Velox.
Tpch extension is also used to compare against Tpch results in the ParquetTpchTest. With the above API in velox, we can probably create a duckdb table with Velox tpc-h connector data and run queries to verify.

[majetideepak]

#6725 is finally ready sans 1 test failing likely due to a bug in DuckDB.

[majetideepak]

This is now complete.