You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org> on 2024/07/02 16:54:29 UTC
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Zoltan Borok-Nagy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21563
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
IMPALA-13194: Fast-serialize position delete records
Currently the serialization of position delete records are very
wasteful. The records contain slots 'file_path' and 'pos'. And
what we do during serialization is the following:
1. Write fixed-size tuple that have a StringValue and a BigInt slot
2. Copy the StringValue's contents after the tuple.
3. Convert the StringValue ptr to be an offset to the string data
So we end up having something like this:
+-------------+--------+----------------+-------------+--------+----------------+-----+
| StringValue | BigInt | File path | StringValue | BigInt | File path | ... |
+-------------+--------+----------------+-------------+--------+----------------+-----+
| ptr, len | 42 | /.../a.parquet | ptr, len | 43 | /.../a.parquet | ... |
+-------------+--------+----------------+-------------+--------+----------------+-----+
This is very redundant to store the file paths that way, and in the
end we will have a huge buffer that we need to compress and send over
the network. Moreover, we copy the file paths in memory twice:
1. From input row batch to the KrpcDataStreamSender::Channel's temporary row batch
2. From the temporary row batch to the outbound row batch (during serialization)
The position delete files store the delete records in ascending order.
This means adjacent records mostly have the same file path. So we could
just buffer the position delete records up to the Channel's capacity,
then serialize the data in a more efficient way.
With this patch, serialized data will look like this:
+----------------+-------------+--------+-------------+--------+-----+
| File path | StringValue | BigInt | StringValue | BigInt | ... |
+----------------+-------------+--------+-------------+--------+-----+
| /.../a.parquet | ptr, len | 43 | ptr, len | 42 | ... |
+----------------+-------------+--------+-------------+--------+-----+
File path, then tuples with the same file path, after that comes the
next file path and tuples associated with that one, and so on.
TODO: measurements
I have an initial measurement with an earlier version:
07:EXCHANGE : 1m ==> 52s
F02:EXCHANGE SENDER: 1m2s ==> 12s
Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
---
M be/src/exec/iceberg-delete-builder.h
M be/src/runtime/CMakeLists.txt
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
A be/src/runtime/outbound-row-batch.cc
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch.cc
M be/src/runtime/string-value.h
8 files changed, 287 insertions(+), 59 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/21563/1
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 6: Code-Review+2
(1 comment)
One small typo, otherwise +2
http://gerrit.cloudera.org:8080/#/c/21563/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/21563/6//COMMIT_MSG@48
PS6, Line 48: :
Nit: unneeded colon.
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 17 Jul 2024 11:04:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21563
to look at the new patch set (#4).
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
IMPALA-13194: Fast-serialize position delete records
Currently the serialization of position delete records are very
wasteful. The records contain slots 'file_path' and 'pos'. And
what we do during serialization is the following:
1. Write fixed-size tuples that have a StringValue and a BigInt slot
2. Copy the StringValue's contents after the tuple.
3. Convert the StringValue ptr to be an offset to the string data
So we end up having something like this:
+-------------+--------+----------------+-------------+--------+----------------+-----+
| StringValue | BigInt | File path | StringValue | BigInt | File path | ... |
+-------------+--------+----------------+-------------+--------+----------------+-----+
| ptr, len | 42 | /.../a.parquet | ptr, len | 43 | /.../a.parquet | ... |
+-------------+--------+----------------+-------------+--------+----------------+-----+
This is very redundant to store the file paths that way, and in the
end we will have a huge buffer that we need to compress and send over
the network. Moreover, we copy the file paths in memory twice:
1. From input row batch to the KrpcDataStreamSender::Channel's temporary row batch
2. From the temporary row batch to the outbound row batch (during serialization)
The position delete files store the delete records in ascending order.
This means adjacent records mostly have the same file path. So we could
just buffer the position delete records up to the Channel's capacity,
then serialize the data in a more efficient way.
With this patch, serialized data will look like this:
+----------------+-------------+--------+-------------+--------+-----+
| File path | StringValue | BigInt | StringValue | BigInt | ... |
+----------------+-------------+--------+-------------+--------+-----+
| /.../a.parquet | ptr, len | 42 | ptr, len | 43 | ... |
+----------------+-------------+--------+-------------+--------+-----+
File path, then tuples with the same file path, after that comes the
next file path and tuples associated with that one, and so on.
TODO: measurements
I have an initial measurement with an earlier version:
07:EXCHANGE : 1m ==> 52s
F02:EXCHANGE SENDER: 1m2s ==> 12s
Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
---
M be/src/exec/iceberg-delete-builder.h
M be/src/runtime/CMakeLists.txt
A be/src/runtime/iceberg-position-delete-collector.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
A be/src/runtime/outbound-row-batch.cc
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch.cc
M be/src/runtime/string-value.h
9 files changed, 395 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/21563/4
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 1:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/21563/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/21563/1//COMMIT_MSG@13
PS1, Line 13: e
Nit: tuples?
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.h
File be/src/runtime/krpc-data-stream-sender.h:
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.h@308
PS1, Line 308: std::unordered_map<Channel*, std::unique_ptr<IcebergPositionDeleteChannel>>
Could add a comment that describes this variable.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@633
PS1, Line 633: unique_ptr
I think it would be cleaner if we returned OutboundRowBatch*. AFAICS KrpcDataStreamSender::Channel::TransmitData() could also take a raw pointer.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@797
PS1, Line 797: if (row_count_ == capacity_) {
Can channel_->RowBatchCapacity() ever be 0 at L775? If it can then this check comes too late. If it can't we can add a DCHECK.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@857
PS1, Line 857: Ubsan::MemSet
What if 'tuple_data_size' is 0? In this case 'tuple_data' may be a nullptr according to https://en.cppreference.com/w/cpp/container/vector/data and I'm not sure if memset with a nullptr could be undefined behaviour.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@943
PS1, Line 943: auto
I think writing the actual type (Channel*) is easier to read.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@945
PS1, Line 945: row_desc_->tuple_descriptors()[0]
Could extract into a variable before the loop.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@1282
PS1, Line 1282: row
Not changed in this patch, but it should be 'tuple'.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/string-value.h
File be/src/runtime/string-value.h:
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/string-value.h@200
PS1, Line 200: inline std::size_t hash_value(const StringValue& v) {
Not changed in this patch, but how does this work with small strings? StringValue::Eq() first converts the values to SimpleStrings to eliminate the difference between small and normal strings.
Or if we only use it with non-small strings, we should add a DCHECK.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/string-value.h@204
PS1, Line 204: struct StringValueHashWrapper {
Do you think we should consider specialising std::hash for StringValue or should we keep this explicit?
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/string-value.h@206
PS1, Line 206: impala::
Do we need this qualifier?
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Jul 2024 11:01:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21563
to look at the new patch set (#7).
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
IMPALA-13194: Fast-serialize position delete records
Currently the serialization of position delete records are very
wasteful. The records contain slots 'file_path' and 'pos'. And
what we do during serialization is the following:
1. Write fixed-size tuples that have a StringValue and a BigInt slot
2. Copy the StringValue's contents after the tuple.
3. Convert the StringValue ptr to be an offset to the string data
So we end up having something like this:
+-------------+--------+----------------+-------------+--------+----------------+-----+
| StringValue | BigInt | File path | StringValue | BigInt | File path | ... |
+-------------+--------+----------------+-------------+--------+----------------+-----+
| ptr, len | 42 | /.../a.parquet | ptr, len | 43 | /.../a.parquet | ... |
+-------------+--------+----------------+-------------+--------+----------------+-----+
This is very redundant to store the file paths that way, and in the
end we will have a huge buffer that we need to compress and send over
the network. Moreover, we copy the file paths in memory twice:
1. From input row batch to the KrpcDataStreamSender::Channel's temporary row batch
2. From the temporary row batch to the outbound row batch (during serialization)
The position delete files store the delete records in ascending order.
This means adjacent records mostly have the same file path. So we could
just buffer the position delete records up to the Channel's capacity,
then serialize the data in a more efficient way.
With this patch, serialized data will look like this:
+----------------+-------------+--------+-------------+--------+-----+
| File path | StringValue | BigInt | StringValue | BigInt | ... |
+----------------+-------------+--------+-------------+--------+-----+
| /.../a.parquet | ptr, len | 42 | ptr, len | 43 | ... |
+----------------+-------------+--------+-------------+--------+-----+
File path, then tuples with the same file path, after that comes the
next file path and tuples associated with that one, and so on.
Measurements:
07:EXCHANGE : 1m ==> 52s
F02:EXCHANGE SENDER: 1m2s ==> 16s
Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
---
M be/src/exec/iceberg-delete-builder.h
M be/src/runtime/CMakeLists.txt
A be/src/runtime/iceberg-position-delete-collector.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
A be/src/runtime/outbound-row-batch.cc
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch.cc
M be/src/runtime/string-value.h
9 files changed, 395 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/21563/7
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/16521/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 Jul 2024 15:15:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 2:
(16 comments)
Thanks for the comments!
http://gerrit.cloudera.org:8080/#/c/21563/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/21563/1//COMMIT_MSG@13
PS1, Line 13: e
> Nit: tuples?
Done
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.h
File be/src/runtime/krpc-data-stream-sender.h:
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.h@308
PS1, Line 308: /// A mapping from Channel to IcebergPositionDeleteChannel. Only used in DIRECTED mode
> Could add a comment that describes this variable.
Done
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@234
PS1, Line 234: private:
> line has trailing whitespace
Done
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@633
PS1, Line 633:
> I think it would be cleaner if we returned OutboundRowBatch*. AFAICS KrpcDa
TransmitData() requires a unique_ptr so it can swap outbound batches.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@760
PS1, Line 760: }
> Can you add some comments about the role of this class?
Added comment. Also moved out most of the contents to IcebergPositionDeleteCollector.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@797
PS1, Line 797: return Status::OK();
> Can channel_->RowBatchCapacity() ever be 0 at L775? If it can then this che
RowBatchCapacity() is always at least 1 (there is a max(1, <expr>) in its body), otherwise the channels couldn't send any rows.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@855
PS1, Line 855: }
> It should compress if the channel is not local.
Ah, thanks for catching this.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@857
PS1, Line 857: (IsDirectedM
> What if 'tuple_data_size' is 0? In this case 'tuple_data' may be a nullptr
Ubsan::MemSet() handles exactly this case.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@943
PS1, Line 943: i64
> I think writing the actual type (Channel*) is easier to read.
Actual type would be 'unique_ptr<impala::KrpcDataStreamSender::Channel>&', I'm not sure if that would be an improvement in readability in this otherwise trivial case.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@945
PS1, Line 945: i64 %seed) #49 {
> Could extract into a variable before the loop.
Done
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@1282
PS1, Line 1282:
> Not changed in this patch, but it should be 'tuple'.
Done
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/outbound-row-batch.h
File be/src/runtime/outbound-row-batch.h:
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/outbound-row-batch.h@72
PS1, Line 72: Status Finalize(int num_tuples_per_row, TrackedString* compression_scratch);
> line has trailing whitespace
Done
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/row-batch.cc
File be/src/runtime/row-batch.cc:
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/row-batch.cc@272
PS1, Line 272: // Switch to using full deduplication in cases where severe size blow-ups are known to
> +1 for moving these to another outbound-row-batch.cc
Thanks, to make it more safe I've replaced it with an OutboundRowBatch::Finalize() method.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/string-value.h
File be/src/runtime/string-value.h:
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/string-value.h@200
PS1, Line 200: inline std::size_t hash_value(const StringValue& v) {
> Not changed in this patch, but how does this work with small strings? Strin
Ptr() returns a pointer to the data, and Len() returns the actual length, so it should work well with small strings.
SimpleStrings wouldn't make any difference as their 'ptr' and 'len' members would have the same value as Ptr() and Len(). SimpleStrings are only used in cases where they make the code more readable or optimal.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/string-value.h@204
PS1, Line 204: std::ostream& operator<<(std::ostream& os, const StringValue& string_value);
> Do you think we should consider specialising std::hash for StringValue or s
Yeah, I think we should. We already do that for boost containers (see hash_value above), so I don't see why we shouldn't for std containers. Done.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/string-value.h@206
PS1, Line 206:
> Do we need this qualifier?
Done
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sun, 14 Jul 2024 15:10:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 Jul 2024 15:45:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10795/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 Jul 2024 15:59:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 Jul 2024 15:35:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 Jul 2024 21:06:37 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@234
PS1, Line 234: friend KrpcDataStreamSender::IcebergPositionDeleteChannel;
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/outbound-row-batch.h
File be/src/runtime/outbound-row-batch.h:
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/outbound-row-batch.h@72
PS1, Line 72: // Sets header of this outbound row batch.
line has trailing whitespace
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Jul 2024 16:55:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 2: Code-Review+1
(4 comments)
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/iceberg-position-delete-collector.h
File be/src/runtime/iceberg-position-delete-collector.h:
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/iceberg-position-delete-collector.h@30
PS2, Line 30: /// serializing them to outbound row batches.
Can you add the layout from the commit message?
Also, it could be mentioned what parts of memory are tracked and what are not.
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/iceberg-position-delete-collector.h@63
PS2, Line 63: Reset
This could be called inside Close and made private.
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/outbound-row-batch.h
File be/src/runtime/outbound-row-batch.h:
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/outbound-row-batch.h@30
PS2, Line 30: KrpcDataStreamSender
Is this really needed?
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/outbound-row-batch.h@68
PS2, Line 68: Prepares
Maybe PrepareForSend() would be clearer? It seem counter-intuitive for Finalize to be called more than once.
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Jul 2024 13:07:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/16522/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 4
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 Jul 2024 15:26:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/16510/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Sun, 14 Jul 2024 15:32:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 7:
(1 comment)
In PS6, conflict resolution was also not perfect in iceberg-delete-builder.h
http://gerrit.cloudera.org:8080/#/c/21563/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/21563/6//COMMIT_MSG@48
PS6, Line 48: s
> Nit: unneeded colon.
vim.. Done.
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 17 Jul 2024 11:43:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@760
PS1, Line 760:
Can you add some comments about the role of this class?
Also, I think that it would be better to move some of the logic to another class (e.g. PositionDeleteCollector) and file - KrpcDataStreamSender is already too large/complex.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@855
PS1, Line 855: bool compress = channel_->IsLocal();
It should compress if the channel is not local.
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/row-batch.cc
File be/src/runtime/row-batch.cc:
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/row-batch.cc@272
PS1, Line 272: RETURN_IF_ERROR(output_batch->TryCompress(compression_scratch, is_compressed));
+1 for moving these to another outbound-row-batch.cc
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 11 Jul 2024 11:47:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/16529/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 17 Jul 2024 12:06:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 17 Jul 2024 11:56:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21563
to look at the new patch set (#3).
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
IMPALA-13194: Fast-serialize position delete records
Currently the serialization of position delete records are very
wasteful. The records contain slots 'file_path' and 'pos'. And
what we do during serialization is the following:
1. Write fixed-size tuples that have a StringValue and a BigInt slot
2. Copy the StringValue's contents after the tuple.
3. Convert the StringValue ptr to be an offset to the string data
So we end up having something like this:
+-------------+--------+----------------+-------------+--------+----------------+-----+
| StringValue | BigInt | File path | StringValue | BigInt | File path | ... |
+-------------+--------+----------------+-------------+--------+----------------+-----+
| ptr, len | 42 | /.../a.parquet | ptr, len | 43 | /.../a.parquet | ... |
+-------------+--------+----------------+-------------+--------+----------------+-----+
This is very redundant to store the file paths that way, and in the
end we will have a huge buffer that we need to compress and send over
the network. Moreover, we copy the file paths in memory twice:
1. From input row batch to the KrpcDataStreamSender::Channel's temporary row batch
2. From the temporary row batch to the outbound row batch (during serialization)
The position delete files store the delete records in ascending order.
This means adjacent records mostly have the same file path. So we could
just buffer the position delete records up to the Channel's capacity,
then serialize the data in a more efficient way.
With this patch, serialized data will look like this:
+----------------+-------------+--------+-------------+--------+-----+
| File path | StringValue | BigInt | StringValue | BigInt | ... |
+----------------+-------------+--------+-------------+--------+-----+
| /.../a.parquet | ptr, len | 42 | ptr, len | 43 | ... |
+----------------+-------------+--------+-------------+--------+-----+
File path, then tuples with the same file path, after that comes the
next file path and tuples associated with that one, and so on.
TODO: measurements
I have an initial measurement with an earlier version:
07:EXCHANGE : 1m ==> 52s
F02:EXCHANGE SENDER: 1m2s ==> 12s
Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
---
M be/src/exec/iceberg-delete-builder.h
M be/src/runtime/CMakeLists.txt
A be/src/runtime/iceberg-position-delete-collector.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
A be/src/runtime/outbound-row-batch.cc
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch.cc
M be/src/runtime/string-value.h
9 files changed, 396 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/21563/3
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 1: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Jul 2024 16:07:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
IMPALA-13194: Fast-serialize position delete records
Currently the serialization of position delete records are very
wasteful. The records contain slots 'file_path' and 'pos'. And
what we do during serialization is the following:
1. Write fixed-size tuples that have a StringValue and a BigInt slot
2. Copy the StringValue's contents after the tuple.
3. Convert the StringValue ptr to be an offset to the string data
So we end up having something like this:
+-------------+--------+----------------+-------------+--------+----------------+-----+
| StringValue | BigInt | File path | StringValue | BigInt | File path | ... |
+-------------+--------+----------------+-------------+--------+----------------+-----+
| ptr, len | 42 | /.../a.parquet | ptr, len | 43 | /.../a.parquet | ... |
+-------------+--------+----------------+-------------+--------+----------------+-----+
This is very redundant to store the file paths that way, and in the
end we will have a huge buffer that we need to compress and send over
the network. Moreover, we copy the file paths in memory twice:
1. From input row batch to the KrpcDataStreamSender::Channel's temporary row batch
2. From the temporary row batch to the outbound row batch (during serialization)
The position delete files store the delete records in ascending order.
This means adjacent records mostly have the same file path. So we could
just buffer the position delete records up to the Channel's capacity,
then serialize the data in a more efficient way.
With this patch, serialized data will look like this:
+----------------+-------------+--------+-------------+--------+-----+
| File path | StringValue | BigInt | StringValue | BigInt | ... |
+----------------+-------------+--------+-------------+--------+-----+
| /.../a.parquet | ptr, len | 42 | ptr, len | 43 | ... |
+----------------+-------------+--------+-------------+--------+-----+
File path, then tuples with the same file path, after that comes the
next file path and tuples associated with that one, and so on.
Measurements:
07:EXCHANGE : 1m ==> 52s
F02:EXCHANGE SENDER: 1m2s ==> 16s
Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Reviewed-on: http://gerrit.cloudera.org:8080/21563
Reviewed-by: Csaba Ringhofer <cs...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/iceberg-delete-builder.h
M be/src/runtime/CMakeLists.txt
A be/src/runtime/iceberg-position-delete-collector.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
A be/src/runtime/outbound-row-batch.cc
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch.cc
M be/src/runtime/string-value.h
9 files changed, 395 insertions(+), 68 deletions(-)
Approvals:
Csaba Ringhofer: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 8
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/row-batch.cc
File be/src/runtime/row-batch.cc:
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/row-batch.cc@266
PS2, Line 266: RETURN_IF_ERROR(output_batch->Finalize(row_desc_->tuple_descriptors().size(),
optional: I think that it would be useful to move Finalize (and compression) outside of Serialize()
two reasons:
- the signature of Serialize() could be simpler by not having compression related arguments
- a separate timer could be added for compression to KrpcDataSender - currently serialize_batch_timer_ contains both serialization and compression, so it is not evident from the profile which one takes more time
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 Jul 2024 05:30:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 3:
(8 comments)
Thanks for the comments!
http://gerrit.cloudera.org:8080/#/c/21563/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/21563/1//COMMIT_MSG@13
PS1, Line 13: e
> Not yet done :)
Now it's really done. Hopefully.. :)
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/iceberg-position-delete-collector.h
File be/src/runtime/iceberg-position-delete-collector.h:
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/iceberg-position-delete-collector.h@30
PS2, Line 30: /// serializing them to outbound row batches. The serialization algorithm
> Can you add the layout from the commit message?
Done
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/iceberg-position-delete-collector.h@63
PS2, Line 63: l_ =
> This could be called inside Close and made private.
Done
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@943
PS1, Line 943: i64
> I think it would be an improvement, for example for reading "ch.get()" - if
Done
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/krpc-data-stream-sender.cc@188
PS2, Line 188: light. This is expect
> It could be private. Right now it's not used anywhere else and I don't like
Done
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/outbound-row-batch.h
File be/src/runtime/outbound-row-batch.h:
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/outbound-row-batch.h@30
PS2, Line 30: MemTracker;
> Is this really needed?
Done
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/outbound-row-batch.h@68
PS2, Line 68: 'compres
> Maybe PrepareForSend() would be clearer? It seem counter-intuitive for Fina
Done
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/row-batch.cc
File be/src/runtime/row-batch.cc:
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/row-batch.cc@266
PS2, Line 266: RETURN_IF_ERROR(output_batch->PrepareForSend(row_desc_->tuple_descriptors().size(),
> optional: I think that it would be useful to move Finalize (and compression
This will be a separate change from you as we discussed offline.
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 Jul 2024 14:51:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 5
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 Jul 2024 15:59:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21563
to look at the new patch set (#2).
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
IMPALA-13194: Fast-serialize position delete records
Currently the serialization of position delete records are very
wasteful. The records contain slots 'file_path' and 'pos'. And
what we do during serialization is the following:
1. Write fixed-size tuple that have a StringValue and a BigInt slot
2. Copy the StringValue's contents after the tuple.
3. Convert the StringValue ptr to be an offset to the string data
So we end up having something like this:
+-------------+--------+----------------+-------------+--------+----------------+-----+
| StringValue | BigInt | File path | StringValue | BigInt | File path | ... |
+-------------+--------+----------------+-------------+--------+----------------+-----+
| ptr, len | 42 | /.../a.parquet | ptr, len | 43 | /.../a.parquet | ... |
+-------------+--------+----------------+-------------+--------+----------------+-----+
This is very redundant to store the file paths that way, and in the
end we will have a huge buffer that we need to compress and send over
the network. Moreover, we copy the file paths in memory twice:
1. From input row batch to the KrpcDataStreamSender::Channel's temporary row batch
2. From the temporary row batch to the outbound row batch (during serialization)
The position delete files store the delete records in ascending order.
This means adjacent records mostly have the same file path. So we could
just buffer the position delete records up to the Channel's capacity,
then serialize the data in a more efficient way.
With this patch, serialized data will look like this:
+----------------+-------------+--------+-------------+--------+-----+
| File path | StringValue | BigInt | StringValue | BigInt | ... |
+----------------+-------------+--------+-------------+--------+-----+
| /.../a.parquet | ptr, len | 43 | ptr, len | 42 | ... |
+----------------+-------------+--------+-------------+--------+-----+
File path, then tuples with the same file path, after that comes the
next file path and tuples associated with that one, and so on.
TODO: measurements
I have an initial measurement with an earlier version:
07:EXCHANGE : 1m ==> 52s
F02:EXCHANGE SENDER: 1m2s ==> 12s
Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
---
M be/src/exec/iceberg-delete-builder.h
M be/src/runtime/CMakeLists.txt
A be/src/runtime/iceberg-position-delete-collector.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
A be/src/runtime/outbound-row-batch.cc
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch.cc
M be/src/runtime/string-value.h
9 files changed, 381 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/21563/2
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 1:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10775/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Jul 2024 11:02:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 6:
PS6 is a rebase to resolve merge conflicts. The conflict was in iceberg-delete-builder.h since we don't have StringValueHashWrapper anymore.
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 17 Jul 2024 10:53:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10800/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 17 Jul 2024 15:04:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 7: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 7
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 17 Jul 2024 20:10:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21563
to look at the new patch set (#6).
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
IMPALA-13194: Fast-serialize position delete records
Currently the serialization of position delete records are very
wasteful. The records contain slots 'file_path' and 'pos'. And
what we do during serialization is the following:
1. Write fixed-size tuples that have a StringValue and a BigInt slot
2. Copy the StringValue's contents after the tuple.
3. Convert the StringValue ptr to be an offset to the string data
So we end up having something like this:
+-------------+--------+----------------+-------------+--------+----------------+-----+
| StringValue | BigInt | File path | StringValue | BigInt | File path | ... |
+-------------+--------+----------------+-------------+--------+----------------+-----+
| ptr, len | 42 | /.../a.parquet | ptr, len | 43 | /.../a.parquet | ... |
+-------------+--------+----------------+-------------+--------+----------------+-----+
This is very redundant to store the file paths that way, and in the
end we will have a huge buffer that we need to compress and send over
the network. Moreover, we copy the file paths in memory twice:
1. From input row batch to the KrpcDataStreamSender::Channel's temporary row batch
2. From the temporary row batch to the outbound row batch (during serialization)
The position delete files store the delete records in ascending order.
This means adjacent records mostly have the same file path. So we could
just buffer the position delete records up to the Channel's capacity,
then serialize the data in a more efficient way.
With this patch, serialized data will look like this:
+----------------+-------------+--------+-------------+--------+-----+
| File path | StringValue | BigInt | StringValue | BigInt | ... |
+----------------+-------------+--------+-------------+--------+-----+
| /.../a.parquet | ptr, len | 42 | ptr, len | 43 | ... |
+----------------+-------------+--------+-------------+--------+-----+
File path, then tuples with the same file path, after that comes the
next file path and tuples associated with that one, and so on.
Measurements:
07:EXCHANGE : 1m ==> 52:s
F02:EXCHANGE SENDER: 1m2s ==> 16s
Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
---
M be/src/exec/iceberg-delete-builder.h
M be/src/runtime/CMakeLists.txt
A be/src/runtime/iceberg-position-delete-collector.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
A be/src/runtime/outbound-row-batch.cc
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/row-batch.cc
M be/src/runtime/string-value.h
9 files changed, 395 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/21563/6
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/21563/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/21563/1//COMMIT_MSG@13
PS1, Line 13: e
> Done
Not yet done :)
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:
http://gerrit.cloudera.org:8080/#/c/21563/1/be/src/runtime/krpc-data-stream-sender.cc@943
PS1, Line 943: i64
> Actual type would be 'unique_ptr<impala::KrpcDataStreamSender::Channel>&',
I think it would be an improvement, for example for reading "ch.get()" - if we know it is a unique_ptr, we know that ".get()" simply returns the pointer and doesn't call an actual method of the channel object.
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/krpc-data-stream-sender.cc
File be/src/runtime/krpc-data-stream-sender.cc:
http://gerrit.cloudera.org:8080/#/c/21563/2/be/src/runtime/krpc-data-stream-sender.cc@188
PS2, Line 188: GetSerializationBatch
It could be private. Right now it's not used anywhere else and I don't like the idea of giving out the owning pointer to external clients.
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 Jul 2024 11:59:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/16458/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Jul 2024 17:20:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 1:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10774/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Jul 2024 19:54:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 1: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/10774/
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Jul 2024 01:10:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13194: Fast-serialize position delete records
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21563 )
Change subject: IMPALA-13194: Fast-serialize position delete records
......................................................................
Patch Set 6:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/16528/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/21563
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6095f318e3d06dedb4197681156b40dd2a326c6f
Gerrit-Change-Number: 21563
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Wed, 17 Jul 2024 11:13:53 +0000
Gerrit-HasComments: No