Support Dwrf Sequence Ids in Writer#16037
Merged
rschlussel merged 1 commit intoprestodb:masterfrom May 12, 2021
Merged
Conversation
|
|
arunthirupathi
left a comment
arunthirupathi
left a comment
There was a problem hiding this comment.
Left some minor comments, but the change looks good once they are addressed. I will approve them once they are addressed.
presto-orc/src/main/java/com/facebook/presto/orc/metadata/Stream.java
Outdated
Show resolved
Hide resolved
presto-orc/src/main/java/com/facebook/presto/orc/writer/ByteColumnWriter.java
Outdated
Show resolved
Hide resolved
presto-orc/src/main/java/com/facebook/presto/orc/stream/DecimalOutputStream.java
Outdated
Show resolved
Hide resolved
|
Can you please rebase on the latest master ? This change will have lot of merge conflicts with, 30cd883 |
akhilum
force-pushed
the
Flat-Map-PR-1
branch
2 times, most recently
from
0867ec0 to
ffb9aca
Compare
presto-orc/src/main/java/com/facebook/presto/orc/metadata/DwrfMetadataWriter.java
Outdated
Show resolved
Hide resolved
presto-orc/src/main/java/com/facebook/presto/orc/metadata/DwrfMetadataWriter.java
Outdated
Show resolved
Hide resolved
presto-orc/src/main/java/com/facebook/presto/orc/metadata/OrcMetadataReader.java
Outdated
Show resolved
Hide resolved
presto-orc/src/main/java/com/facebook/presto/orc/metadata/Stream.java
Outdated
Show resolved
Hide resolved
presto-orc/src/main/java/com/facebook/presto/orc/stream/LongOutputStream.java
Outdated
Show resolved
Hide resolved
akhilum
force-pushed
the
Flat-Map-PR-1
branch
2 times, most recently
from
fcf3ba4 to
f79a5d0
Compare
arunthirupathi
approved these changes
May 5, 2021
arunthirupathi
left a comment
arunthirupathi
left a comment
There was a problem hiding this comment.
It looks good once both the comments are addressed.
presto-orc/src/main/java/com/facebook/presto/orc/metadata/DwrfMetadataWriter.java
Outdated
Show resolved
Hide resolved
presto-orc/src/main/java/com/facebook/presto/orc/writer/BooleanColumnWriter.java
Outdated
Show resolved
Hide resolved
akhilum
changed the title
akhilum
force-pushed
the
Flat-Map-PR-1
branch
2 times, most recently
from
ee7515e to
1ce3d07
Compare
arunthirupathi
approved these changes
May 6, 2021
There was a problem hiding this comment.
- stream will always call stream.toString() which will create garbage. Remove the + stream or use format specifier.
Author
There was a problem hiding this comment.
Stream has custom toString already implemented.
Contributor
There was a problem hiding this comment.
nit: use format instead of concat
There was a problem hiding this comment.
It will print the right details (some readable string, instead of some memory address), that is not a concern. Calling toString will create a temporary string and lot of string concatenation. We want to avoid that.
two ways to avoid it are
- Do not include the stream in the error message. less perfereable, as we will not see what stream failed.
- call checkArgument(condition, "some message {}", object). Here object.toString() will be only called condition is false and avoiding the garbage on the hot path.
Author
There was a problem hiding this comment.
Done. Also fixed the checkArgument in toColumnEncoding() to use format instead of concat
akhilum
changed the title
bhhari
reviewed
May 6, 2021
bhhari
reviewed
May 6, 2021
presto-orc/src/main/java/com/facebook/presto/orc/metadata/DwrfMetadataReader.java
Outdated
Show resolved
Hide resolved
arunthirupathi
approved these changes
May 7, 2021
rschlussel
reviewed
May 11, 2021
presto-orc/src/test/java/com/facebook/presto/orc/TestDecryption.java
Outdated
Show resolved
Hide resolved
Sequence Ids are used by Dwrf FlatMap implementations. This commit introduces sequence for DWRF writer. Following PR of flat map implementation will use this sequence id to add multiple streams per column.
rschlussel
approved these changes
May 12, 2021
10 tasks
16 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sequence Ids are used by Dwrf FlatMap implementations. This commit introduces sequence for DWRF writer.
Following PR of flat map implementation will use this sequence id to add multiple streams per column.