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.

Sign up for GitHub

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

Jump to bottom

[native] Table writer 1: Pass LocationHandle to HiveInsertTableHandle #18490

Merged
merged 1 commit into from
Nov 7, 2022
Merged

[native] Table writer 1: Pass LocationHandle to HiveInsertTableHandle #18490

merged 1 commit into from
Nov 7, 2022

Conversation

gggrace14
Copy link
Contributor

@gggrace14 gggrace14 commented Oct 13, 2022 •
edited
Loading

Enable the ser/de of protocol::HiveInsertTableHandle.

Extract LocationHandle and HiveColumnHandles from
protocol::HiveOutputTableHandle and protocol::HiveInsertTableHandle,
and add them to Velox connector::HiveInsertTableHandle.
Pass them down to the plan TableWriteNode.

== NO RELEASE NOTE ==

@gggrace14 gggrace14 force-pushed the insert_table_handle branch from 6206b40 to 44b44d9 Compare October 14, 2022 03:43
@gggrace14 gggrace14 changed the title [native] LocationHandle [native] Pass more structures from protocol to TableWriteNode Oct 14, 2022
@gggrace14 gggrace14 marked this pull request as ready for review October 14, 2022 08:33
@gggrace14 gggrace14 requested a review from a team as a code owner October 14, 2022 08:33
@gggrace14 gggrace14 changed the title [native] Pass more structures from protocol to TableWriteNode [native] Table writer 1 Pass more structures from protocol to TableWriteNode Oct 14, 2022
@gggrace14 gggrace14 changed the title [native] Table writer 1 Pass more structures from protocol to TableWriteNode [native] Table writer 1: Pass more structures from protocol to TableWriteNode Oct 14, 2022
@gggrace14 gggrace14 closed this Oct 19, 2022
@gggrace14 gggrace14 deleted the insert_table_handle branch October 19, 2022 09:02
@gggrace14 gggrace14 restored the insert_table_handle branch October 25, 2022 22:47
@gggrace14 gggrace14 reopened this Oct 25, 2022
@gggrace14 gggrace14 force-pushed the insert_table_handle branch from 44b44d9 to cfc827c Compare October 25, 2022 23:48
@gggrace14 gggrace14 changed the title [native] Table writer 1: Pass more structures from protocol to TableWriteNode [native] Table writer 1: Pass LocationHandle to HiveInsertTableHandle Oct 25, 2022
@gggrace14 gggrace14 force-pushed the insert_table_handle branch 5 times, most recently from 4267d9a to e347690 Compare November 3, 2022 17:38
@kgpai kgpai force-pushed the insert_table_handle branch from e347690 to 51ebd59 Compare November 3, 2022 18:56
@gggrace14 gggrace14 force-pushed the insert_table_handle branch from 51ebd59 to 5c67424 Compare November 3, 2022 21:30
amitkdutta
amitkdutta reviewed Nov 4, 2022
View reviewed changes
presto-native-execution/presto_cpp/main/types/PrestoToVeloxQueryPlan.cpp
case protocol::TableType::TEMPORARY:
return connector::hive::LocationHandle::TableType::kTemporary;
default:
throw std::invalid_argument("Unknown table type");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better to throw velox exception from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got feedback on this in #18377, and address all occurrences in this file there.

Corresponding code copy of this PR has already been merged into fbcode repo. This PR is supposed to be just porting to GitHub, so better to keep the code identical of the fbcode copy. It's just Merge was blocked by the protobuf path issue as well as velox_hive_connector OBJECT issue as in facebookincubator/velox#3094.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gggrace14 and everyone.
We do not want to port fbcode changes to GH. We want to avoid any changes in fbcode.
We will get this through, though.

@gggrace14 can you please create and follow up a small issue on changing this exception to Velox exception, please? To be consistent.

presto-native-execution/presto_cpp/main/types/PrestoToVeloxQueryPlan.cpp
return connector::hive::LocationHandle::WriteMode::
kDirectToTargetExistingDirectory;
default:
throw std::invalid_argument("Unknown write mode");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

amitkdutta
amitkdutta reviewed Nov 4, 2022
View reviewed changes
presto-native-execution/presto_cpp/main/types/PrestoToVeloxQueryPlan.cpp Outdated
@@ -1086,7 +1114,7 @@ core::PlanNodePtr VeloxQueryPlanConverter::toVeloxQueryPlan(
node->source)) {
std::optional<core::JoinType> joinType = std::nullopt;
if (equal(node->predicate, semiJoin->semiJoinOutput)) {
joinType = core::JoinType::kLeftSemi;
joinType = core::JoinType::kLeftSemiFilter;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change related here?

Copy link
Contributor Author

@gggrace14 gggrace14 Nov 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to pick up Masha's change, per ask. It's in a separate commit.

@gggrace14 gggrace14 force-pushed the insert_table_handle branch 3 times, most recently from a2d9d92 to 80e4c4f Compare November 4, 2022 07:47
gggrace14
gggrace14 commented Nov 4, 2022
View reviewed changes
presto-native-execution/CMakeLists.txt
@@ -165,6 +165,7 @@ include_directories(.)
include_directories(velox)
include_directories(velox/velox/external/xxhash)
include_directories(${VELOX_ROOT})
include_directories(${CMAKE_BINARY_DIR})
Copy link
Contributor Author

@gggrace14 gggrace14 Nov 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to fix the proto path issue triggered by this PR, found by @kgpai

'velox/velox/dwio/dwrf/proto/dwrf_proto.pb.h' file not found

@mbasmanova
Copy link
Contributor

@gggrace14 CI join is still failing. Do you know why?

@gggrace14 gggrace14 closed this Nov 4, 2022
@gggrace14 gggrace14 reopened this Nov 4, 2022
@gggrace14
Copy link
Contributor Author

@gggrace14 CI join is still failing. Do you know why?

Checking it

@gggrace14 gggrace14 force-pushed the insert_table_handle branch from 80e4c4f to 5509011 Compare November 4, 2022 17:44
spershin
spershin approved these changes Nov 4, 2022
View reviewed changes
Copy link
Contributor

@spershin spershin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gggrace14 Let's get this in.

@gggrace14 gggrace14 force-pushed the insert_table_handle branch from 5509011 to af6cac4 Compare November 4, 2022 21:33
@gggrace14
[native] Pass LocationHandle to HiveInsertTableHandle
1b81afe 
Enable the ser/de of protocol::HiveInsertTableHandle.

Extract LocationHandle and HiveColumnHandles from
protocol::HiveOutputTableHandle and protocol::HiveInsertTableHandle,
and add them to Velox connector::HiveInsertTableHandle.
Pass them down to the plan TableWriteNode.
@gggrace14 gggrace14 force-pushed the insert_table_handle branch from af6cac4 to 1b81afe Compare November 5, 2022 00:58
@gggrace14
Copy link
Contributor Author

gggrace14 commented Nov 5, 2022 •
edited
Loading

Tracking the troubleshooting process for dependency issues

  1. [Error] Could not make proto path relative: velox/dwio/dwrf/proto/dwrf_proto.proto: No such file or directory
    [Fix] Fix protobuf builds when Velox is used as a sub project.  facebookincubator/velox#3062

  2. [Error] fatal error: 'velox/velox/dwio/dwrf/proto/orc_proto.pb.h' file not found
    #include "velox/velox/dwio/dwrf/proto/orc_proto.pb.h"
    [Fix] Add to presto-native-execution/CMakeLists.txt

include_directories(${CMAKE_BINARY_DIR})
  1. [Error]
/opt/rh/gcc-toolset-9/root/usr/include/c++/9/ext/new_allocator.h:147: undefined reference to `facebook::velox::connector::hive::HiveTableHandle::HiveTableHandle(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&

[Fix] facebookincubator/velox#3094

  1. [Error] Failed at [531/548] Linking CXX executable presto_cpp/main/types/tests/presto_expressions_test. Log
    with exit code 137, which is oom.
    [Fix] Build on Docker w/ more resources. It then succeeded, P547972474.

@gggrace14 gggrace14 merged commit 8e66656 into prestodb:master Nov 7, 2022
@gggrace14 gggrace14 deleted the insert_table_handle branch November 7, 2022 06:35
@gggrace14 gggrace14 mentioned this pull request Nov 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amitkdutta amitkdutta amitkdutta left review comments

@spershin spershin spershin approved these changes

@mbasmanova mbasmanova Awaiting requested review from mbasmanova

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gggrace14 @mbasmanova @spershin @amitkdutta