Repository navigation
GH-35757: [C++][Parquet] Build encodings metadata from actual encodings - #35758
Conversation
|
|
| std::vector<format::Encoding::type> thrift_encodings; | ||
| std::vector<format::PageEncodingStats> thrift_encoding_stats; | ||
| // Force Emplace encoding for RL/DL. | ||
| thrift_encodings.push_back(ToThrift(Encoding::RLE)); |
There was a problem hiding this comment.
Is this true? I think BIT_PACKED is still supported.
There was a problem hiding this comment.
arrow/cpp/src/parquet/column_writer.cc
Lines 182 to 186 in 6d2df07
There was a problem hiding this comment.
Oops, I just keep the logic here as before. It's ok to add or remove it :-)
emm.. |
|
Can you expand the PR description to explain a bit better what this is about? I don't understand just by looking at the code. |
|
@pitrou This patch uses |
|
I go through the Parquet-mr's code, and I think they're doing the better: void writeColumnChunk(ColumnDescriptor descriptor,
long valueCount,
CompressionCodecName compressionCodecName,
DictionaryPage dictionaryPage,
BytesInput bytes,
long uncompressedTotalPageSize,
long compressedTotalPageSize,
Statistics<?> totalStats,
ColumnIndexBuilder columnIndexBuilder,
OffsetIndexBuilder offsetIndexBuilder,
BloomFilter bloomFilter,
Set<Encoding> rlEncodings,
Set<Encoding> dlEncodings,
List<Encoding> dataEncodings,
BlockCipher.Encryptor headerBlockEncryptor,
int rowGroupOrdinal,
int columnOrdinal,
byte[] fileAAD) throws IOException {
startColumn(descriptor, valueCount, compressionCodecName);
state = state.write();
if (dictionaryPage != null) {
byte[] dictonaryPageHeaderAAD = null;
if (null != headerBlockEncryptor) {
dictonaryPageHeaderAAD = AesCipher.createModuleAAD(fileAAD, ModuleType.DictionaryPageHeader,
rowGroupOrdinal, columnOrdinal, -1);
}
writeDictionaryPage(dictionaryPage, headerBlockEncryptor, dictonaryPageHeaderAAD);
}
if (bloomFilter != null) {
// write bloom filter if one of data pages is not dictionary encoded
boolean isWriteBloomFilter = false;
for (Encoding encoding : dataEncodings) {
if (encoding != Encoding.RLE_DICTIONARY) {
isWriteBloomFilter = true;
break;
}
}
if (isWriteBloomFilter) {
currentBloomFilters.put(String.join(".", descriptor.getPath()), bloomFilter);
}
}
LOG.debug("{}: write data pages", out.getPos());
long headersSize = bytes.size() - compressedTotalPageSize;
this.uncompressedLength += uncompressedTotalPageSize + headersSize;
this.compressedLength += compressedTotalPageSize + headersSize;
LOG.debug("{}: write data pages content", out.getPos());
currentChunkFirstDataPage = out.getPos();
bytes.writeAllTo(out);
encodingStatsBuilder.addDataEncodings(dataEncodings);
if (rlEncodings.isEmpty()) {
encodingStatsBuilder.withV2Pages();
}
currentEncodings.addAll(rlEncodings);
currentEncodings.addAll(dlEncodings);
currentEncodings.addAll(dataEncodings);
currentStatistics = totalStats;
this.columnIndexBuilder = columnIndexBuilder;
this.offsetIndexBuilder = offsetIndexBuilder;
endColumn();
}Parquet-mr's
For RL/DL, it has the logic below in and in v2: So, I guess, if not has rl/dl, the encoding would not be collected. It's wired that, some when it's For Dictionary, I guess they don't extra handle the dictionary logic. |
| } | ||
| // Force add encoding for RL/DL. | ||
| // TODO(mwish): BIT_PACKED encoding is supported, but it will not be | ||
| // added here. And when RL/DL is empty, it will not be added. |
There was a problem hiding this comment.
Not sure I understand the comment. Why would we add BIT_PACKED if RLE is being used?
There was a problem hiding this comment.
Sorry for didn't make it clear, I've update it to
// FIXME(mwish): BIT_PACKED encoding is supported, but it would not be
// added here even in format v1.
// And when RL/DL is empty, RLE would be added.
There was a problem hiding this comment.
encodings represent only the encodings used, not the encodings supported. So I don't understand why you're mentioning BIT_PACKED here.
There was a problem hiding this comment.
Thats a good question:
void LevelEncoder::Init(Encoding::type encoding, int16_t max_level,
int num_buffered_values, uint8_t* data, int data_size) {
bit_width_ = bit_util::Log2(max_level + 1);
encoding_ = encoding;
switch (encoding) {
case Encoding::RLE: {
rle_encoder_ = std::make_unique<RleEncoder>(data, data_size, bit_width_);
break;
}
case Encoding::BIT_PACKED: {
int num_bytes =
static_cast<int>(bit_util::BytesForBits(num_buffered_values * bit_width_));
bit_packed_encoder_ = std::make_unique<BitWriter>(data, num_bytes);
break;
}
default:
throw ParquetException("Unknown encoding type for levels.");
}
}
I guess we have LevelEncoder that can use BIT_PACKED. But actually it's never used in our code now except for testing
There was a problem hiding this comment.
I've update it again, hope it would not being confusing...
1b76699 to
7f46757
Compare
7f46757 to
91262cd
Compare
|
Thanks a lot @mapleFU ! This is really appreciated. |
Co-authored-by: Gang Wu <[email protected]>
|
Benchmark runs are scheduled for baseline = 5c772af and contender = fee8bf9. fee8bf9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
['Python', 'R'] benchmarks have high level of regressions. |
Rationale for this change
This patch uses
encoding_statsto build the encodings. Currently,ColumnChunk.encodingsis repeated ( May has multiplePLAIN) and having some wired logic by gettingWriterProperties. And if later we want to support Fallback to non "PLAIN" encoding,encoding_statswould be right, butencodingwould be "PLAIN". So this patch usingencoding_statsto build theencoding, and force them to be consistent.What changes are included in this PR?
Are these changes tested?
Already has test
Are there any user-facing changes?
Some behavior would be changed, but I think my change is right :)
encoding_statsto buildencodings#35757