[native] Add TPC-DS connector #23067

pdabre12 · 2024-06-24T19:32:38Z

Description

Adds the native TPC-DS connector and the codegen (dsdgen) files. The TPC-DS connector will be a wrapper for the data generator (dsdgen) provided by the TPC organization, originally implemented in C. This implementation must mimic the behavior of the original C implementation. DuckDB has a TPC-DS connector that uses C++ files to wrap the C implementation, and we will use these C++ files for our implementation.

Motivation and Context

The goal is to add a TPC-DS connector to enhance our ability to write end-to-end tests for Prestissimo and conduct micro-benchmarks in Velox. This addition will significantly improve our testing capabilities by ensuring comprehensive coverage and performance validation.

Impact

With this change, we can now write e2e tests and microbenchmarks.

Test Plan

Native end-to-end tests.
Future enhancements will include adding SpeedTest and ConnectorTest to the Velox repository.

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

TPC-DS Connector Changes
* Add TPC-DS connector for Presto C++. :pr:`23067`

Resolves: #22361

linux-foundation-easycla · 2024-06-24T19:32:42Z

The committers listed above are authorized under a signed CLA.

✅ login: pdabre12 / name: Pratik Joseph Dabre (aa5ae0f, ce1e567, fc6337d)

steveburnett · 2024-07-15T13:42:00Z

Consider updating the Use Cases in the Presto C++ section of the Presto doc to add TPC-DS to the supported connectors.

steveburnett · 2024-08-05T14:50:56Z

Consider a release note entry similar to the following:

== RELEASE NOTES ==

TPC-DS Connector Changes
* Add TPC-DS connector for Presto C++. :pr:`23067`

steveburnett

LGTM! (docs)

Pull updated branch, new local docs build, looks good. Thanks!

pdabre12 · 2024-08-09T20:32:41Z

@aditi-pandit @yingsu00
This PR is now ready for review. Please take a look.
Thanks.

pdabre12 · 2024-08-09T20:42:41Z

FYI: @czentgr

aditi-pandit

Thanks @pramodsatya and @pdabre12.

Have done a quick round of review.

presto-native-execution/presto_cpp/presto_protocol/presto_protocol.yml

presto-native-execution/presto_cpp/main/connectors/tpcds/dsdgen/dsdgen-c/StringBuffer.cpp

presto-native-execution/presto_cpp/main/connectors/CMakeLists.txt

presto-native-execution/presto_cpp/main/connectors/tpcds/TpcdsGen.cpp

czentgr

Thanks for all the work done here!

presto-native-execution/etc/catalog/tpcds.properties

czentgr · 2024-08-12T18:40:35Z

presto-native-execution/presto_cpp/main/connectors/CMakeLists.txt

+# Without this hack, there are multiple link errors similar to the one below
+# only on GCC. "undefined reference to `vtable for
+# velox::connector::tpcds::TpcdsTableHandle`. TODO: Fix this hack.
+target_link_libraries(velox_exec_test_lib presto_tpcds_connector)


velox_exec_test_lib is a collection of velox utilities.
Which objects gets the undefined reference?

It gives an undefined reference to TpcdsTableHandle.
It's similar to

presto/presto-native-execution/presto_cpp/main/CMakeLists.txt

Line 93 in 4dd678d

# "undefined reference to `vtable for velox::connector::tpch::TpchTableHandle`"

.

presto-native-execution/presto_cpp/main/connectors/tpcds/DSDGenIterator.cpp

presto-native-execution/presto_cpp/main/connectors/tpcds/TpcdsConnector.cpp

presto-native-execution/presto_cpp/main/connectors/tpcds/TpcdsConnector.h

presto-native-execution/presto_cpp/main/connectors/tpcds/TpcdsGen.cpp

czentgr · 2024-08-12T22:51:00Z

presto-native-execution/presto_cpp/main/connectors/tpcds/TpcdsGen.h

+
+Table fromTableName(std::string_view tableName);
+
+RowVectorPtr genTpcdsCallCenter(


These all have the same signature but only a different name.
You can use a macro to define the signature and then just generate the signature for each name.

Changed the code a little bit, can you take another look now?

presto-tpcds/src/main/java/com/facebook/presto/tpcds/TpcdsMetadata.java

pdabre12 · 2024-08-13T21:01:23Z

@aditi-pandit @czentgr.
Thanks for the review.
Stalling this PR until we figure out the licensing.

pdabre12 · 2024-08-29T21:49:42Z

@czentgr @aditi-pandit
The licensing issue is resolved.
Addressed your comments , can you have another look?
Thank you.

Will address the failing header checks soon.

majetideepak

I think the dsdgen should be moved to the external folder.

czentgr

My intiial comments. There might be more but to get you going.

presto-native-execution/presto_cpp/main/connectors/tpcds/CMakeLists.txt

czentgr · 2024-08-29T21:32:23Z

presto-native-execution/presto_cpp/main/connectors/tpcds/CMakeLists.txt

+# Add subdirectories
+add_subdirectory(dsdgen/dsdgen-c)
+
+add_library(append_info OBJECT include/append_info-c.cpp)


Kinda strange there is a cpp file in a directory called "include" which I presume means cmake doesn't look for CMakeLists.txt in it so we need the CMP0079 policy to define the target here.

Yes, that's why we need the policy here.
Changed the directory name to "utils".

presto-native-execution/presto_cpp/main/connectors/tpcds/DSDGenIterator.cpp

presto-native-execution/presto_cpp/main/connectors/tpcds/DSDGenIterator.h

majetideepak

@pdabre12 Some comments. ~~I think we can merge the external portion first in a separate commit as they are copied externally. That makes it easy to review the new bits.~~ I see that it is already a separate commit.

@aditi-pandit, @tdcmeehan I think we should have a RFC for this work. What do you think? The RFC can be simple.

majetideepak · 2024-09-11T14:31:01Z

presto-docs/src/main/sphinx/presto-cpp.rst

+* TPCH connector, with ``tpch.naming=standard`` catalog property.
+
+* TPCDS connector.


Do we have a column naming issue for TPCDS similar to TPCH?

I am not aware of the column naming issue for TPCH, can you please elaborate?

majetideepak · 2024-09-11T14:59:12Z

presto-native-execution/presto_cpp/main/connectors/tpcds/TpcdsGen.h

+#pragma once
+
+#include "velox/common/memory/Memory.h"
+#include "velox/vector/ComplexVector.h"


Why do we need this header? Can we forward declare?

+1. Should be possible to forward declare RowVectorPtr.

presto-native-execution/presto_cpp/main/connectors/tpcds/TpcdsGen.cpp

presto-native-execution/presto_cpp/main/types/CMakeLists.txt

presto-native-execution/presto_cpp/main/connectors/tpcds/utils/append_info-c.hpp

presto-tpcds/src/main/java/com/facebook/presto/tpcds/TpcdsMetadata.java

aditi-pandit

@pdabre12, @pramodsatya : Have a first round of comments.

presto-native-execution/presto_cpp/main/types/CMakeLists.txt

aditi-pandit · 2024-09-12T06:58:44Z

presto-native-execution/presto_cpp/presto_protocol/special/TpcdsTransactionHandle.hpp.inc

+// TpcdsTransactionHandle is special since
+// the corresponding class in Java is an enum.
+
+namespace facebook::presto::protocol {


Can we add API's to PrestoToVeloxConnector to handle transaction handles as well ?

Thanks for the suggestion, could this be taken up in a separate PR? (since it affects transaction handles for all connectors)

presto-native-execution/presto_cpp/main/connectors/tpcds/utils/append_info-c.hpp

presto-native-execution/presto_cpp/main/connectors/tpcds/DSDGenIterator.h

aditi-pandit · 2024-09-12T10:48:12Z

presto-native-execution/presto_cpp/main/connectors/tpcds/DSDGenIterator.h

+      vector_size_t parallel,
+      vector_size_t child);
+
+  void initializeTable(const std::vector<VectorPtr>& children, int table);


Please add comments for each of the methods in this class as this is the final interface seen over dsdgen

presto-native-execution/presto_cpp/main/connectors/tpcds/DSDGenIterator.h

aditi-pandit · 2024-09-12T14:08:41Z

@pdabre12 Some comments. ~~I think we can merge the external portion first in a separate commit as they are copied externally. That makes it easy to review the new bits.~~ I see that it is already a separate commit.

@aditi-pandit, @tdcmeehan I think we should have a RFC for this work. What do you think? The RFC can be simple.

@pdabre12 @majetideepak : Small RFC is okay. Let's bring this to Native worker working group next session so that it gets immediate attention from others.

steveburnett

LGTM! (docs)

Pull branch, local docs build.

Co-authored-by: Pramod Satya <pramod.satya@ibm.com>

pdabre12 force-pushed the tpcds-connector branch from 69d9c1b to a88f29d Compare June 24, 2024 19:33

pramodsatya force-pushed the tpcds-connector branch from a88f29d to 03214b7 Compare June 24, 2024 20:31

pdabre12 force-pushed the tpcds-connector branch from 8734eef to 3f40e32 Compare June 28, 2024 17:13

pdabre12 changed the title ~~[native] Adds the TPC-DS connector~~ [WIP] [native] Adds the TPC-DS connector Jul 9, 2024

pdabre12 force-pushed the tpcds-connector branch from 4d36367 to 375dc59 Compare July 15, 2024 20:37

pramodsatya force-pushed the tpcds-connector branch from 375dc59 to c9e0947 Compare July 17, 2024 18:54

pdabre12 force-pushed the tpcds-connector branch 2 times, most recently from 64ab39e to f08bd87 Compare August 2, 2024 23:49

pdabre12 force-pushed the tpcds-connector branch from dc5f0e2 to 8a5fe7c Compare August 6, 2024 21:34

steveburnett previously approved these changes Aug 7, 2024

View reviewed changes

pdabre12 dismissed steveburnett’s stale review via 42377b8 August 7, 2024 20:00

pdabre12 force-pushed the tpcds-connector branch from 32aa4e4 to 42377b8 Compare August 7, 2024 20:00

pramodsatya requested review from yingsu00 and aditi-pandit August 7, 2024 20:15

pramodsatya changed the title ~~[WIP] [native] Adds the TPC-DS connector~~ [native] Adds the TPC-DS connector Aug 7, 2024

pramodsatya force-pushed the tpcds-connector branch from 1f7db4d to 1b86b4d Compare August 8, 2024 19:10

pdabre12 force-pushed the tpcds-connector branch 2 times, most recently from 6d9a0c5 to e69c19d Compare August 9, 2024 20:30

pdabre12 marked this pull request as ready for review August 9, 2024 20:32

pdabre12 requested review from elharo and a team as code owners August 9, 2024 20:32

pdabre12 requested a review from presto-oss August 9, 2024 20:32

pdabre12 requested a review from czentgr August 9, 2024 20:42

aditi-pandit reviewed Aug 12, 2024

View reviewed changes

czentgr reviewed Aug 12, 2024

View reviewed changes

pdabre12 force-pushed the tpcds-connector branch from ad02d92 to 6f39bf1 Compare August 29, 2024 21:36

majetideepak changed the title ~~[native] Adds the TPC-DS connector~~ [native] Add TPC-DS connector Sep 6, 2024

majetideepak requested changes Sep 6, 2024

View reviewed changes

pdabre12 force-pushed the tpcds-connector branch 2 times, most recently from 47e2545 to cc187f0 Compare September 6, 2024 19:54

czentgr reviewed Sep 6, 2024

View reviewed changes

pdabre12 force-pushed the tpcds-connector branch from cc187f0 to eb763e0 Compare September 10, 2024 20:28

majetideepak reviewed Sep 11, 2024

View reviewed changes

aditi-pandit reviewed Sep 12, 2024

View reviewed changes

pdabre12 force-pushed the tpcds-connector branch from eb763e0 to ce9914b Compare September 23, 2024 23:53

pramodsatya force-pushed the tpcds-connector branch from ce9914b to 9dba598 Compare September 24, 2024 04:06

steveburnett previously approved these changes Sep 24, 2024

View reviewed changes

pdabre12 dismissed steveburnett’s stale review via 5ff51ef September 24, 2024 21:34

pdabre12 force-pushed the tpcds-connector branch from 5ff51ef to a1afddd Compare October 2, 2024 17:45

pramodsatya force-pushed the tpcds-connector branch 2 times, most recently from b9c985d to fb56a83 Compare October 4, 2024 22:24

pdabre12 and others added 2 commits October 8, 2024 16:01

Add config property 'tpcds.toggle-char-to-varchar'

aa5ae0f

[native] Dsdgen codegen files

fc6337d

Co-authored-by: Pramod Satya <pramod.satya@ibm.com>

pramodsatya force-pushed the tpcds-connector branch from fb56a83 to 09a13d0 Compare October 8, 2024 21:02

pdabre12 force-pushed the tpcds-connector branch from 09a13d0 to a373413 Compare October 14, 2024 15:33

[native] Add TPC-DS connector

ce1e567

Co-authored-by: Pramod Satya <pramod.satya@ibm.com>

pdabre12 force-pushed the tpcds-connector branch from a373413 to ce1e567 Compare October 14, 2024 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[native] Add TPC-DS connector #23067

[native] Add TPC-DS connector #23067

pdabre12 commented Jun 24, 2024 •

edited

Loading

linux-foundation-easycla bot commented Jun 24, 2024 •

edited

Loading

steveburnett commented Jul 15, 2024

steveburnett commented Aug 5, 2024

steveburnett left a comment

pdabre12 commented Aug 9, 2024

pdabre12 commented Aug 9, 2024 •

edited

Loading

aditi-pandit left a comment

czentgr left a comment

czentgr Aug 12, 2024

pdabre12 Aug 29, 2024

czentgr Aug 12, 2024

pdabre12 Aug 29, 2024

pdabre12 commented Aug 13, 2024

pdabre12 commented Aug 29, 2024 •

edited

Loading

majetideepak left a comment

czentgr left a comment

czentgr Aug 29, 2024

pdabre12 Sep 10, 2024

majetideepak left a comment •

edited

Loading

majetideepak Sep 11, 2024

pdabre12 Oct 2, 2024

majetideepak Sep 11, 2024

aditi-pandit Sep 12, 2024

aditi-pandit left a comment

aditi-pandit Sep 12, 2024

pramodsatya Sep 24, 2024

aditi-pandit Sep 12, 2024

aditi-pandit commented Sep 12, 2024

steveburnett left a comment


		Table fromTableName(std::string_view tableName);

		RowVectorPtr genTpcdsCallCenter(

		* TPCH connector, with ``tpch.naming=standard`` catalog property.

		* TPCDS connector.

[native] Add TPC-DS connector #23067

Are you sure you want to change the base?

[native] Add TPC-DS connector #23067

Conversation

pdabre12 commented Jun 24, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

linux-foundation-easycla bot commented Jun 24, 2024 • edited Loading

steveburnett commented Jul 15, 2024

steveburnett commented Aug 5, 2024

steveburnett left a comment

Choose a reason for hiding this comment

pdabre12 commented Aug 9, 2024

pdabre12 commented Aug 9, 2024 • edited Loading

aditi-pandit left a comment

Choose a reason for hiding this comment

czentgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdabre12 commented Aug 13, 2024

pdabre12 commented Aug 29, 2024 • edited Loading

majetideepak left a comment

Choose a reason for hiding this comment

czentgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditi-pandit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditi-pandit commented Sep 12, 2024

steveburnett left a comment

Choose a reason for hiding this comment

pdabre12 commented Jun 24, 2024 •

edited

Loading

linux-foundation-easycla bot commented Jun 24, 2024 •

edited

Loading

pdabre12 commented Aug 9, 2024 •

edited

Loading

pdabre12 commented Aug 29, 2024 •

edited

Loading

majetideepak left a comment •

edited

Loading