Add script to modify Thrift include guard variables #7
Conversation
velox/duckdb/README.md
Outdated
| Then modify the include guard variables in parquet-amalgamation.hpp by prefixing them with | ||
| DUCKDB using the script velox/external/duckdb/modify_include_guards.sh as follows: | ||
|
|
||
| while in the velox/external/duckdb directory |
There was a problem hiding this comment.
cd <path/to/velox>/velox/external/duckdb/
There was a problem hiding this comment.
| @@ -0,0 +1,10 @@ | |||
| echo $1 | |||
| TAGS=$(sed -n 's/^#ifndef \(_[A-Z][A-Z_]*_\)/\1/p' ${1}) | |||
There was a problem hiding this comment.
We only want to substitute the Thrift headers, so should we match _THRIFT_ too?
velox/duckdb/README.md
Outdated
| @@ -16,6 +16,12 @@ Then copy the generated files to velox/external/duckdb: | |||
| rsync -vrh src/amalgamation/duckdb.* <path/to/velox>/velox/external/duckdb/ | |||
| rsync -vrh src/amalgamation/parquet* <path/to/velox>/velox/external/duckdb/ | |||
|
|
|||
| Then modify the include guard variables in parquet-amalgamation.hpp by prefixing them with | |||
| DUCKDB using the script velox/external/duckdb/modify_include_guards.sh as follows: | |||
There was a problem hiding this comment.
Shall we explain briefly why we need to do this?
| @@ -0,0 +1,10 @@ | |||
| echo $1 | |||
There was a problem hiding this comment.
On the first line add #!/bin/bash
Then follows the copyright header
|
|
||
| while in the velox/external/duckdb directory | ||
| ./modify_include_guards.sh parquet-amalgamation.hpp | ||
|
|
There was a problem hiding this comment.
There is also one _THRIFT_TOSTRING_H_ in parquet-amalgamation.cpp
|
@jwyles-ahana Thank you for submitting the PR so quickly! Would you make the commit title and PR title shorter? Ideally we want them to be within 50 characters. |
There was a problem hiding this comment.
git clone https://github.com/cwida/duckdb.git
==>
git clone https://github.com/duckdb/duckdb.git
https://github.com/cwida/duckdb.git is not being actively maintained. We should change it to duckdb's repo.
|
Moreover, bitwise_cast() in the hpp was not behind any include guard, and will have duplicate definition if we include the standalone Thrift library. Will you be able to see which header file it belongs to, and put it back behind its original include guard? |
|
@jwyles-ahana The commit message needs to follow this guideline: https://gist.github.com/robertpainsi/b632364184e70900af4ab688decf6f53 |
…ions DuckDB includes headers from Thrift. The new parquet reader also needs to include the same Thrift headers. To avoid collisions during compilation we need to alias the include guard variables used in the header so that they don't conflict. The script finds all include guard variables that begin with '_THRIFT' and prepends a '_DUCKDB' to them.
8e89f19
to
5db4530
Compare
Add script to modify include guard variables in parquet-amalgamation.hpp. The script finds all the include guard variables in the file, then substitutes them with _DUCKDB prefixed to them. This is to avoid interfering with the include guards from Thrift files used by the new parquet reader.