[native] Table writer 2: Add Presto write protocols#18377
[native] Table writer 2: Add Presto write protocols#18377gggrace14 merged 1 commit intoprestodb:masterfrom
Conversation
gggrace14
force-pushed
the
insert
branch
4 times, most recently
from
d39e15a to
47a898e
Compare
gggrace14
force-pushed
the
insert
branch
3 times, most recently
from
596d4f9 to
78198b6
Compare
gggrace14
force-pushed
the
insert
branch
2 times, most recently
from
964c0eb to
c84c999
Compare
gggrace14
changed the title
gggrace14
changed the title
gggrace14
force-pushed
the
insert
branch
5 times, most recently
from
9e68793 to
c069b87
Compare
gggrace14
changed the title
gggrace14
changed the title
gggrace14
force-pushed
the
insert
branch
3 times, most recently
from
6c33e23 to
80b2b95
Compare
gggrace14
force-pushed
the
insert
branch
3 times, most recently
from
ca52b64 to
39ca0c5
Compare
|
@gggrace14 There are CI failures. Are these related? |
mbasmanova
left a comment
mbasmanova
left a comment
There was a problem hiding this comment.
@gggrace14 Looks good overall. Some comments below.
There was a problem hiding this comment.
Use VELOX_USER_CHECK and include tableType in the error message (see toJsonString helper function) to simplify troubleshooting.
There was a problem hiding this comment.
Revised here and other places. Found in this file we use VELOX_UNSUPPORTED for this case
There was a problem hiding this comment.
Why is no-commit protocol hard-coded here?
There was a problem hiding this comment.
Here is where a system sets the commit strategy it's going to use. CommitStrategy::kNoCommit is set for Presto.
Also discussed with MJ previously, and for PrestoSpark, he will change here to add a condition to set kTaskCommit for PrestoSpark and kNoCommit for Presto.
There was a problem hiding this comment.
= default or remove this and let compiler auto-generate
There was a problem hiding this comment.
remove commented out code
There was a problem hiding this comment.
nit: consider shortening
columns.emplace_back(BaseVector::createConstant(commitInfo.numWrittenRows(), 1, pool))
There was a problem hiding this comment.
fragmentsVector and commitContextVector are used only in the 'else' branch. Consider moving them there.
There was a problem hiding this comment.
add period at the end of the sentence; ditto other places
There was a problem hiding this comment.
use BaseVector::create<FlatVector<int64_t>>(...)
ditto other places
gggrace14
force-pushed
the
insert
branch
3 times, most recently
from
b8f071f to
f0d3a97
Compare
mbasmanova
left a comment
mbasmanova
left a comment
There was a problem hiding this comment.
@gggrace14 Looks good to me. Please, make sure all CI is green before landing.
There was a problem hiding this comment.
Is this accessible outside of Meta?
There was a problem hiding this comment.
Since we are inside presto::protocol namespace, "Presto" prefix can be removed.
gggrace14
force-pushed
the
insert
branch
7 times, most recently
from
cca4b30 to
ab21950
Compare
Add PrestoNoCommitWriteProtocol and PrestoTaskCommitWriteProtocol, which extends the write protocols of Hive connector and make commit() return the specific format of outputs expected by Presto from the table writer. Register the two write protocols during server start. The right protocol will be picked up by the table writer, given the CommitStrategy used by the TableWriter.
3 participants
Add PrestoNoCommitWriteProtocol and PrestoTaskCommitWriteProtocol,
which extends the write protocols of Hive connector and make commit()
return the specific format of outputs expected by Presto from
the table writer. Register the two write protocols during server start.
The right protocol will be picked up by the table writer,
given the CommitStrategy used by the TableWriter.