Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport to duckdb 1.0.0 ? #1

Closed
carlopi opened this issue Jul 8, 2024 · 8 comments
Closed

Backport to duckdb 1.0.0 ? #1

carlopi opened this issue Jul 8, 2024 · 8 comments

Comments

@carlopi
Copy link

carlopi commented Jul 8, 2024

Feature request: I think it should be possible to backport this extension to work also with 1.0.0, that could be interesting since most users are on 1.0.0.
A blueprint that should probably just work is this PR Alex-Monahan/sql-only-duckdb-extension#1 against the similar sql-only-duckdb-extension.

Once this is on 1.0.0, it should basically just work if submitted as community-extension: https://github.com/duckdb/community-extensions, do consider submit a PR there.

@lmangani
Copy link
Collaborator

lmangani commented Jul 8, 2024

Ciao Carlo 👋 Nice to meet fellow Amsterdammers from DuckDB HQs!

Thanks for the heads up. I assumed this extension would work with v1.0.0 already without patches but its a good point. I'm new to the extension ecosystem, any PR or suggestion is greatly appreciated, if not I'll try to backport the changes from main repo.

We're testing the extension with our project quackpipe currently, but once we're done with development and testing we'll most definitely submit to the community extension repository!

@carlopi
Copy link
Author

carlopi commented Jul 8, 2024

There are a few things we should get better at documenting, we are iterating on that.

PR is basically the same as the other one:

  • move duckdb submodule to point to v1.0.0
  • add the relevant cpp and header files
  • move from pointing to duckdb headers to local headers
  • add cpp files into CMakeLists

Once you do the first step, the other are forced since compiler/linker will otherwise complain.

@lmangani
Copy link
Collaborator

lmangani commented Jul 8, 2024

move duckdb submodule to point to v1.0.0

OK. I'm trying to apply the other PR to this repo but I'm experiencing quite a lot of errors linking so I'm guessing step 1 is missing. If you don't mind helping, how do you point the duckdb submodule to v1.0.0?

[472/533] Linking CXX shared library extension/dynamic_sql_clickhouse/dynamic_sql_clickhouse.duckdb_extension
FAILED: extension/dynamic_sql_clickhouse/dynamic_sql_clickhouse.duckdb_extension
: && /usr/bin/c++ -fPIC -O3 -DNDEBUG -ffunction-sections -fdata-sections -O3 -DNDEBUG   -shared -Wl,-soname,dynamic_sql_clickhouse.duckdb_extension -o extension/dynamic_sql_clickhouse/dynamic_sql_clickhouse.duckdb_extension extension/dynamic_sql_clickhouse/CMakeFiles/dynamic_sql_clickhouse_loadable_extension.dir/src/dynamic_sql_clickhouse_extension.cpp.o extension/dynamic_sql_clickhouse/CMakeFiles/dynamic_sql_clickhouse_loadable_extension.dir/src/default_table_functions.cpp.o  src/libduckdb_static.a  -Wl,--gc-sections  -Wl,--exclude-libs,ALL  /usr/lib/x86_64-linux-gnu/libssl.so  /usr/lib/x86_64-linux-gnu/libcrypto.so  extension/dynamic_sql_clickhouse/libdynamic_sql_clickhouse_extension.a  extension/parquet/libparquet_extension.a  extension/jemalloc/libjemalloc_extension.a  src/libduckdb_static.a  extension/dynamic_sql_clickhouse/libdynamic_sql_clickhouse_extension.a  extension/parquet/libparquet_extension.a  extension/jemalloc/libjemalloc_extension.a  -ldl  third_party/fsst/libduckdb_fsst.a  third_party/fmt/libduckdb_fmt.a  third_party/libpg_query/libduckdb_pg_query.a  third_party/re2/libduckdb_re2.a  third_party/miniz/libduckdb_miniz.a  third_party/utf8proc/libduckdb_utf8proc.a  third_party/hyperloglog/libduckdb_hyperloglog.a  third_party/fastpforlib/libduckdb_fastpforlib.a  third_party/skiplist/libduckdb_skiplistlib.a  third_party/mbedtls/libduckdb_mbedtls.a  third_party/yyjson/libduckdb_yyjson.a  /usr/lib/x86_64-linux-gnu/libssl.so  /usr/lib/x86_64-linux-gnu/libcrypto.so && cd /usr/src/duckdb-extension-clickhouse-sql/build/release/extension/dynamic_sql_clickhouse && /usr/bin/cmake -DEXTENSION=/usr/src/duckdb-extension-clickhouse-sql/build/release/extension/dynamic_sql_clickhouse/dynamic_sql_clickhouse.duckdb_extension -DPLATFORM_FILE=/usr/src/duckdb-extension-clickhouse-sql/build/release/duckdb_platform_out -DDUCKDB_VERSION="0be3e7b436" -DEXTENSION_VERSION="3c03f30" -DNULL_FILE=/usr/src/duckdb-extension-clickhouse-sql/duckdb/scripts/null.txt -P /usr/src/duckdb-extension-clickhouse-sql/duckdb/scripts/append_metadata.cmake
/usr/bin/ld: src/libduckdb_static.a(ub_duckdb_catalog_default_entries.cpp.o): in function `duckdb::DefaultTableFunctionGenerator::DefaultTableFunctionGenerator(duckdb::Catalog&, duckdb::SchemaCatalogEntry&)':
ub_duckdb_catalog_default_entries.cpp:(.text._ZN6duckdb29DefaultTableFunctionGeneratorC2ERNS_7CatalogERNS_18SchemaCatalogEntryE+0x0): multiple definition of `duckdb::DefaultTableFunctionGenerator::DefaultTableFunctionGenerator(duckdb::Catalog&, duckdb::SchemaCatalogEntry&)'; extension/dynamic_sql_clickhouse/CMakeFiles/dynamic_sql_clickhouse_loadable_extension.dir/src/default_table_functions.cpp.o:default_table_functions.cpp:(.text._ZN6duckdb29DefaultTableFunctionGeneratorC2ERNS_7CatalogERNS_18SchemaCatalogEntryE+0x0): first defined here
/usr/bin/ld: src/libduckdb_static.a(ub_duckdb_catalog_default_entries.cpp.o): in function `duckdb::DefaultTableFunctionGenerator::DefaultTableFunctionGenerator(duckdb::Catalog&, duckdb::SchemaCatalogEntry&)':
ub_duckdb_catalog_default_entries.cpp:(.text._ZN6duckdb29DefaultTableFunctionGeneratorC2ERNS_7CatalogERNS_18SchemaCatalogEntryE+0x0): multiple definition of `duckdb::DefaultTableFunctionGenerator::DefaultTableFunctionGenerator(duckdb::Catalog&, duckdb::SchemaCatalogEntry&)'; extension/dynamic_sql_clickhouse/CMakeFiles/dynamic_sql_clickhouse_loadable_extension.dir/src/default_table_functions.cpp.o:default_table_functions.cpp:(.text._ZN6duckdb29DefaultTableFunctionGeneratorC2ERNS_7CatalogERNS_18SchemaCatalogEntryE+0x0): first defined here
/usr/bin/ld: src/libduckdb_static.a(ub_duckdb_catalog_default_entries.cpp.o): in function `duckdb::DefaultTableFunctionGenerator::GetDefaultEntries[abi:cxx11]()':
ub_duckdb_catalog_default_entries.cpp:(.text._ZN6duckdb29DefaultTableFunctionGenerator17GetDefaultEntriesB5cxx11Ev+0x0): multiple definition of `duckdb::DefaultTableFunctionGenerator::GetDefaultEntries[abi:cxx11]()'; extension/dynamic_sql_clickhouse/CMakeFiles/dynamic_sql_clickhouse_loadable_extension.dir/src/default_table_functions.cpp.o:default_table_functions.cpp:(.text._ZN6duckdb29DefaultTableFunctionGenerator17GetDefaultEntriesB5cxx11Ev+0x0): first defined here
/usr/bin/ld: src/libduckdb_static.a(ub_duckdb_catalog_default_entries.cpp.o): in function `duckdb::DefaultTableFunctionGenerator::CreateInternalTableMacroInfo(duckdb::DefaultTableMacro const&, duckdb::unique_ptr<duckdb::MacroFunction, std::default_delete<duckdb::MacroFunction>, true>)':
ub_duckdb_catalog_default_entries.cpp:(.text._ZN6duckdb29DefaultTableFunctionGenerator28CreateInternalTableMacroInfoERKNS_17DefaultTableMacroENS_10unique_ptrINS_13MacroFunctionESt14default_deleteIS5_ELb1EEE+0x0): multiple definition of `duckdb::DefaultTableFunctionGenerator::CreateInternalTableMacroInfo(duckdb::DefaultTableMacro const&, duckdb::unique_ptr<duckdb::MacroFunction, std::default_delete<duckdb::MacroFunction>, true>)'; extension/dynamic_sql_clickhouse/CMakeFiles/dynamic_sql_clickhouse_loadable_extension.dir/src/default_table_functions.cpp.o:default_table_functions.cpp:(.text._ZN6duckdb29DefaultTableFunctionGenerator28CreateInternalTableMacroInfoERKNS_17DefaultTableMacroENS_10unique_ptrINS_13MacroFunctionESt14default_deleteIS5_ELb1EEE+0x0): first defined here
/usr/bin/ld: src/libduckdb_static.a(ub_duckdb_catalog_default_entries.cpp.o): in function `duckdb::DefaultTableFunctionGenerator::CreateTableMacroInfo(duckdb::DefaultTableMacro const&)':
ub_duckdb_catalog_default_entries.cpp:(.text._ZN6duckdb29DefaultTableFunctionGenerator20CreateTableMacroInfoERKNS_17DefaultTableMacroE+0x0): multiple definition of `duckdb::DefaultTableFunctionGenerator::CreateTableMacroInfo(duckdb::DefaultTableMacro const&)'; extension/dynamic_sql_clickhouse/CMakeFiles/dynamic_sql_clickhouse_loadable_extension.dir/src/default_table_functions.cpp.o:default_table_functions.cpp:(.text._ZN6duckdb29DefaultTableFunctionGenerator20CreateTableMacroInfoERKNS_17DefaultTableMacroE+0x0): first defined here
/usr/bin/ld: src/libduckdb_static.a(ub_duckdb_catalog_default_entries.cpp.o): in function `duckdb::DefaultTableFunctionGenerator::CreateDefaultEntry(duckdb::ClientContext&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)':
ub_duckdb_catalog_default_entries.cpp:(.text._ZN6duckdb29DefaultTableFunctionGenerator18CreateDefaultEntryERNS_13ClientContextERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE+0x0): multiple definition of `duckdb::DefaultTableFunctionGenerator::CreateDefaultEntry(duckdb::ClientContext&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)'; extension/dynamic_sql_clickhouse/CMakeFiles/dynamic_sql_clickhouse_loadable_extension.dir/src/default_table_functions.cpp.o:default_table_functions.cpp:(.text._ZN6duckdb29DefaultTableFunctionGenerator18CreateDefaultEntryERNS_13ClientContextERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE+0x0): first defined here
collect2: error: ld returned 1 exit status

Thanks in advance!

@carlopi
Copy link
Author

carlopi commented Jul 8, 2024

I think something like:

cd duckdb
git fetch --tags
git checkout v1.0.0
cd ..
git add duckdb
git commit -m "Point duckdb submodule to v1.0.0"

should work

@carlopi
Copy link
Author

carlopi commented Jul 8, 2024

Idea should be that after that the content of duckdb folder should be the commit tagged as v1.0.0.

@lmangani
Copy link
Collaborator

lmangani commented Jul 8, 2024

Thanks Carlo, it did the trick. Will update the submodule to point at v1.0.0 but this raises a couple questions:

  • Should a "main" version be kept as development master? Or shall we only use official releases to develop?
  • Does this assume developers should maintain copies of the cpp and header files locally to extensions?

The current PR is here

Thanks a million! 🦆

@carlopi
Copy link
Author

carlopi commented Jul 8, 2024

This is an hack of sort to backport a feature that was not yet made available in 1.0.0. But I think it's still worth since it allows SQL extensions to directly target 1.0.0.

I should take a look at what's the cleaner way to have this work transparently both for 1.0.0 and main (likely some preprocessor #ifdef). When I have it I can send a fix.

As to versus what version of DuckDB one should program, it depends, I think the most useful is targeting latest stable (that is what you can ship to users) + keeping an eye on integrating newer stuff.

Can you maybe open an issue with this question on this either in the extension_template or in the community_extension? Given the answer touches most people, I think it's worth having in a central place.

@carlopi carlopi mentioned this issue Jul 9, 2024
@lmangani
Copy link
Collaborator

lmangani commented Jul 9, 2024

Completed with #3

@lmangani lmangani closed this as completed Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants