Upgrade DuckDB to 0.8.1 #6725

majetideepak · 2023-09-25T02:56:40Z

Move from DuckDB amalgamation to using the latest DuckDB 0.8.1 as an external dependency.
See discussion here #5589

netlify · 2023-09-25T02:56:45Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`06208d7`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/653b20607a0cae0008ffc1f2

majetideepak · 2023-10-09T14:57:41Z

There is only one test that is failing
https://app.circleci.com/pipelines/github/facebookincubator/velox/35337/workflows/486c084d-e899-496d-9c10-7cedf0c2a96e/jobs/232713/tests
HashJoinTest.semiProjectWithFilter:
According to the DuckDB documentation, EXISTS operator should never return NULL. However, it does for this particular query. Likely a bug in DuckDB?

../../velox/exec/tests/utils/QueryAssertions.cpp:1061
Failed
Expected 15, got 15
3 extra rows, 3 missing rows
3 of extra rows:
	null | 40 | false
	null | 40 | false
	null | 40 | false

3 of missing rows:
	null | 40 | null
	null | 40 | null
	null | 40 | null

Note: DuckDB only supports timestamps of millisecond precision. If this test involves timestamp inputs, please make sure you use the right precision.
DuckDB query: SELECT t0, t1, EXISTS (SELECT * FROM u WHERE u0 = t0 AND t1 <> u1) FROM t

majetideepak · 2023-10-09T16:03:29Z

@kgpai, @mbasmanova, Can you import and share what the failures look like internally? Thanks.

assignUser · 2023-10-09T16:48:34Z

Wow, very nice to get rid of the amalgamation! Will review later but 🎉

majetideepak · 2023-10-09T19:10:36Z

The tests are taking a lot longer likely due to DuckDB being built in Debug mode. We need to build it in Release mode always. For some reason, I am unable to enable Release mode only for DuckDB via bundling.
See failures here 4fe7d1d
We should probably make it SYSTEM always in CI.

mbasmanova · 2023-10-11T13:12:11Z

@majetideepak

Can you import and share what the failures look like internally?

Deepak, this is a significant undertaking, hence, will need to be prioritized first. I see that CI is red and you mentioned that tests are taking a long time. Maybe resolve these issues first while we are looking for ways to free up bandwidth.

majetideepak · 2023-10-11T16:04:21Z

@mbasmanova and I discussed offline.
Meta's internal build system uses DuckDB 0.8.1. We will use this version in CI as well. 0.8.1 is one release prior to the current recent release (0.9.*) and it should be more stable.
The Velox tpch dbgen depends on the DuckDB TPC-H extension. We must remove this dependency first.
See #7001

pedroerp · 2023-10-23T16:21:05Z

Thanks @majetideepak for looking into this. Now that the dbgen dependency is decoupled, what are the next steps on this PR?

majetideepak · 2023-10-24T17:06:48Z

@pedroerp, this is ready for review

assignUser

LGTM, really nice change!

assignUser · 2023-10-25T03:39:51Z

.circleci/dist_compile.yml

@@ -31,6 +31,28 @@ commands:
            git submodule sync --recursive
            git submodule update --init --recursive

+  install-duckdb:


This is good for now but in a follow up we should add this to the docker images.

Agreed! I don't have the power to update docker images.

Actually you have :) We have a workflow in place that updates the dockerfiles after changes to the files are merged to main: https://github.com/facebookincubator/velox/actions/workflows/docker.yml

Ah! Good to know. Thanks.

assignUser · 2023-10-25T03:43:51Z

CMakeLists.txt

@@ -458,6 +458,9 @@ if("${CMAKE_CXX_COMPILER_ID}" MATCHES "GNU")
    message(
      FATAL_ERROR "VELOX requires gcc > 8. Found ${CMAKE_CXX_COMPILER_VERSION}")
  endif()
+
+  # Find Threads library
+  find_package(Threads REQUIRED)


Why? If ddb requires Threads it will/should look for it. Feel free to resolve if not for ddb but rather our changed code.

I started seeing errors such as below in our code with this PR.

CMake Error at velox/common/memory/tests/CMakeLists.txt:15 (add_executable): Target "velox_memory_test" links to target "Threads::Threads" but the target was not found. Perhaps a find_package() call is missing for an IMPORTED target, or an ALIAS target is missing?

See https://app.circleci.com/pipelines/github/facebookincubator/velox/36404/workflows/3bdaa340-22b1-46df-8377-449100e5bbe9/jobs/241760

Ah I see so it likely was a transitive dependency satisfied via DDB amalgamation that is now missing. Makes sense to add it in that case

CMakeLists.txt

pedroerp · 2023-10-27T01:41:53Z

@majetideepak I can help import, but github is saying there are conflicts that must be resolved in the DuckWrapper files first. I guess you need to first rebase on top of the other PR?

majetideepak · 2023-10-27T02:29:59Z

@pedroerp Rebased.

facebook-github-bot · 2023-10-27T14:59:10Z

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

assignUser · 2023-10-28T01:44:07Z

(I am going to rerun the benchmark to see if the regression sticks around)

assignUser · 2023-11-16T15:57:37Z

fyi: the regression was false positive: https://github.com/facebookincubator/velox/actions/runs/6662181171/job/18731909455?pr=6725#step:16:736

(the conbench altert has all the runs, another commit would override that iirc)

facebook-github-bot · 2023-11-22T06:46:49Z

@pedroerp merged this pull request in 98d6c05.

conbench-facebook · 2023-11-22T07:10:35Z

Conbench analyzed the 1 benchmark run on commit 98d6c053.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

mbasmanova · 2023-11-22T12:13:10Z

Seeing build failures: #7692

Summary: After DuckDB upgrade (PR #6725) we see build errors: ``` /opt/gluten/ep/build-velox/build/velox_ep/_build/release/_deps/duckdb-src/src/planner/binder/expression/bind_star_expression.cpp:123:4: error: 'duckdb_re2' has not been declared 123 | duckdb_re2::RE2 regex(regex_str); | ^~~~~~~~~~ ``` If we compile re2 and fmt before DuckDB, there will cause dependency conflict. A fix is to compile DuckDB before re2 and fmt. Pull Request resolved: #7722 Reviewed By: pedroerp Differential Revision: D51605184 Pulled By: mbasmanova fbshipit-source-id: 54acb0a0f672abe710f6f398fa465ec46d38573c

mbasmanova · 2024-01-17T12:18:37Z

velox/exec/tests/utils/QueryAssertions.cpp

  array.reserve(size);
  for (auto i = 0; i < size; i++) {
    auto innerRow = offset + i;
    if (elements->isNullAt(innerRow)) {
-      array.emplace_back(::duckdb::Value(nullptr));


Looks like this change is responsible for breaking Join Fuzzer: #7943

CC: @majetideepak @kgpai @pedroerp

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 Sep 25, 2023

majetideepak force-pushed the pgparser branch 4 times, most recently from 11a73b0 to 8edb4fd Compare September 27, 2023 00:24

majetideepak force-pushed the pgparser branch from 8edb4fd to 252c957 Compare October 6, 2023 22:04

majetideepak changed the title ~~Upgrade DuckDB to 0.8.1~~ Upgrade DuckDB to 0.9.0 Oct 6, 2023

majetideepak force-pushed the pgparser branch 6 times, most recently from 8ab1418 to 1d1b603 Compare October 9, 2023 12:00

majetideepak marked this pull request as ready for review October 9, 2023 12:25

majetideepak force-pushed the pgparser branch from e965803 to a3d7fb5 Compare October 9, 2023 14:54

majetideepak force-pushed the pgparser branch from a3d7fb5 to a5da908 Compare October 9, 2023 15:00

majetideepak requested review from kgpai, pedroerp, assignUser, mbasmanova and aditi-pandit October 9, 2023 15:01

majetideepak force-pushed the pgparser branch from 4fe7d1d to 41c23ef Compare October 9, 2023 18:55

majetideepak force-pushed the pgparser branch 3 times, most recently from 65dd120 to 3f46b47 Compare October 24, 2023 16:01

assignUser approved these changes Oct 25, 2023

View reviewed changes

majetideepak force-pushed the pgparser branch 2 times, most recently from 3c62b5a to e8e8ad5 Compare October 25, 2023 13:32

majetideepak added 2 commits October 26, 2023 22:27

Upgrade DuckDB to 0.8.1

41faa35

Get FastPFor from source

06208d7

majetideepak force-pushed the pgparser branch from e8e8ad5 to 06208d7 Compare October 27, 2023 02:28

assignUser mentioned this pull request Nov 3, 2023

Fix cyclic dependency velox_exec <-> velox_codegen #6785

Closed

facebook-github-bot closed this in 98d6c05 Nov 22, 2023

facebook-github-bot added the Merged label Nov 22, 2023

mbasmanova mentioned this pull request Nov 22, 2023

Build errors: error: unknown type name 'StringPiece' #7692

Open

This was referenced Nov 25, 2023

Move duckdb compile before re2 and fmt #7722

Closed

[VL] enable 11-22 upstreaming velox rebase apache/incubator-gluten#3773

Merged

MinMaxByNTest.sortedGlobal and MinMaxByNTest.sortedGroupBy failed in debug mode build #7749

Open

majetideepak deleted the pgparser branch January 3, 2024 14:00

mbasmanova reviewed Jan 17, 2024

View reviewed changes

mbasmanova mentioned this pull request Jan 17, 2024

Nightly Join fuzzer consistent faluire 'duckdb::InternalException' values[i].type() == values[0].type() #7943

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade DuckDB to 0.8.1 #6725

Upgrade DuckDB to 0.8.1 #6725

majetideepak commented Sep 25, 2023 •

edited

netlify bot commented Sep 25, 2023 •

edited

majetideepak commented Oct 9, 2023 •

edited

majetideepak commented Oct 9, 2023

assignUser commented Oct 9, 2023

majetideepak commented Oct 9, 2023

mbasmanova commented Oct 11, 2023

majetideepak commented Oct 11, 2023 •

edited

pedroerp commented Oct 23, 2023

majetideepak commented Oct 24, 2023

assignUser left a comment

assignUser Oct 25, 2023

majetideepak Oct 25, 2023

assignUser Oct 25, 2023

majetideepak Oct 25, 2023

assignUser Oct 25, 2023

majetideepak Oct 25, 2023 •

edited

assignUser Oct 25, 2023

pedroerp commented Oct 27, 2023

majetideepak commented Oct 27, 2023

facebook-github-bot commented Oct 27, 2023

assignUser commented Oct 28, 2023

assignUser commented Nov 16, 2023 •

edited

facebook-github-bot commented Nov 22, 2023

conbench-facebook bot commented Nov 22, 2023

mbasmanova commented Nov 22, 2023

mbasmanova Jan 17, 2024

Upgrade DuckDB to 0.8.1 #6725

Upgrade DuckDB to 0.8.1 #6725

Conversation

majetideepak commented Sep 25, 2023 • edited

netlify bot commented Sep 25, 2023 • edited

✅ Deploy Preview for meta-velox canceled.

majetideepak commented Oct 9, 2023 • edited

majetideepak commented Oct 9, 2023

assignUser commented Oct 9, 2023

majetideepak commented Oct 9, 2023

mbasmanova commented Oct 11, 2023

majetideepak commented Oct 11, 2023 • edited

pedroerp commented Oct 23, 2023

majetideepak commented Oct 24, 2023

assignUser left a comment

Choose a reason for hiding this comment

assignUser Oct 25, 2023

Choose a reason for hiding this comment

majetideepak Oct 25, 2023

Choose a reason for hiding this comment

assignUser Oct 25, 2023

Choose a reason for hiding this comment

majetideepak Oct 25, 2023

Choose a reason for hiding this comment

assignUser Oct 25, 2023

Choose a reason for hiding this comment

majetideepak Oct 25, 2023 • edited

Choose a reason for hiding this comment

assignUser Oct 25, 2023

Choose a reason for hiding this comment

pedroerp commented Oct 27, 2023

majetideepak commented Oct 27, 2023

facebook-github-bot commented Oct 27, 2023

assignUser commented Oct 28, 2023

assignUser commented Nov 16, 2023 • edited

facebook-github-bot commented Nov 22, 2023

conbench-facebook bot commented Nov 22, 2023

mbasmanova commented Nov 22, 2023

mbasmanova Jan 17, 2024

Choose a reason for hiding this comment

majetideepak commented Sep 25, 2023 •

edited

netlify bot commented Sep 25, 2023 •

edited

majetideepak commented Oct 9, 2023 •

edited

majetideepak commented Oct 11, 2023 •

edited

majetideepak Oct 25, 2023 •

edited

assignUser commented Nov 16, 2023 •

edited