Skip to content

GH-35757: [C++][Parquet] Build encodings metadata from actual encodings - #35758

Merged
pitrou merged 14 commits into
apache:mainfrom
mapleFU:parquet/using-page-stats-to-build-encoding
May 31, 2023
Merged

pitrou merged 14 commits into
apache:mainfrom
mapleFU:parquet/using-page-stats-to-build-encoding

Conversation

@mapleFU

@mapleFU mapleFU commented May 25, 2023 •

Copy link
Copy Markdown
Member

Rationale for this change

This patch uses encoding_stats to build the encodings. Currently, ColumnChunk.encodings is repeated ( May has multiple PLAIN ) and having some wired logic by getting WriterProperties. And if later we want to support Fallback to non "PLAIN" encoding, encoding_stats would be right, but encoding would be "PLAIN". So this patch using encoding_stats to build the encoding, 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 :)

@mapleFU
mapleFU requested a review from wjones127 as a code owner May 25, 2023 05:49
@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #35757 has been automatically assigned in GitHub to PR creator.

Comment thread cpp/src/parquet/metadata.cc Outdated
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));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this true? I think BIT_PACKED is still supported.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oops, I just keep the logic here as before. It's ok to add or remove it :-)

Comment thread cpp/src/parquet/metadata.cc Resolved
Comment thread cpp/src/parquet/metadata.cc Resolved
@mapleFU

mapleFU commented May 25, 2023

Copy link
Copy Markdown
Member Author
=== sccache stats after the build ===
sccache: error: Timed out waiting for server startup

emm..

@pitrou

pitrou commented May 25, 2023

Copy link
Copy Markdown
Member

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.

@mapleFU

mapleFU commented May 25, 2023

Copy link
Copy Markdown
Member Author

@pitrou This patch uses encoding_stats to build the encodings. Currently, ColumnChunk.encodings is repeated ( May has multiple PLAIN ) and having some wired logic by getting WriterProperties. And if later we want to support Fallback to non "PLAIN" encoding, encoding_stats would be right, but encoding would be "PLAIN". So this patch using encoding_stats to build the encoding, and force them to be consistent. I also remove some duplicated encoding here.

Comment thread cpp/src/parquet/metadata.cc Resolved
Comment thread cpp/src/parquet/metadata.cc Resolved
Comment thread cpp/src/parquet/metadata.cc Resolved
Comment thread cpp/src/parquet/metadata_test.cc Resolved
Comment thread cpp/src/parquet/metadata_test.cc Resolved
@mapleFU

mapleFU commented May 27, 2023

Copy link
Copy Markdown
Member Author

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 ColumnChunkPageWriteStore will record that:

  1. When WritePage is called, what rl/dl/data encoding is used
  2. They will be collected in a Set, and be written to column chunk

For RL/DL, it has the logic below in ColumnWriterV1:

  private ValuesWriter newColumnDescriptorValuesWriter(int maxLevel) {
    if (maxLevel == 0) {
      return new DevNullValuesWriter();
    } else {
      return new RunLengthBitPackingHybridValuesWriter(
          getWidthFromMaxInt(maxLevel), MIN_SLAB_SIZE, pageSizeThreshold, allocator);
    }
  }

and in v2:

  @Override
  ValuesWriter createRLWriter(ParquetProperties props, ColumnDescriptor path) {
    return path.getMaxRepetitionLevel() == 0 ? NULL_WRITER : new RLEWriterForV2(props.newRepetitionLevelEncoder(path));
  }

  @Override
  ValuesWriter createDLWriter(ParquetProperties props, ColumnDescriptor path) {
    return path.getMaxDefinitionLevel() == 0 ? NULL_WRITER : new RLEWriterForV2(props.newDefinitionLevelEncoder(path));
  }

So, I guess, if not has rl/dl, the encoding would not be collected. It's wired that, some when it's DevNullValuesWritter, the level is BIT_PACKED:

  @Override
  public Encoding getEncoding() {
    return BIT_PACKED;
  }

For Dictionary, I guess they don't extra handle the dictionary logic.

@mapleFU

mapleFU commented May 31, 2023

Copy link
Copy Markdown
Member Author

@pitrou @wgtmac I've pasted the parquet-mr logic here

Comment thread cpp/src/parquet/metadata_test.cc Resolved
Comment thread cpp/src/parquet/metadata_test.cc Resolved
Comment thread cpp/src/parquet/metadata_test.cc Resolved
Comment thread cpp/src/parquet/metadata.cc Resolved
Comment thread cpp/src/parquet/metadata.cc Resolved
@mapleFU
mapleFU requested a review from pitrou May 31, 2023 12:32
Comment thread cpp/src/parquet/metadata.cc Outdated
}
// 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure I understand the comment. Why would we add BIT_PACKED if RLE is being used?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

encodings represent only the encodings used, not the encodings supported. So I don't understand why you're mentioning BIT_PACKED here.

@mapleFU mapleFU May 31, 2023 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've update it again, hope it would not being confusing...

@mapleFU
mapleFU force-pushed the parquet/using-page-stats-to-build-encoding branch from 1b76699 to 7f46757 Compare May 31, 2023 13:20
@mapleFU
mapleFU force-pushed the parquet/using-page-stats-to-build-encoding branch from 7f46757 to 91262cd Compare May 31, 2023 13:28
@mapleFU
mapleFU requested a review from pitrou May 31, 2023 14:15
Comment thread cpp/src/parquet/metadata.cc Resolved
@pitrou

pitrou commented May 31, 2023

Copy link
Copy Markdown
Member

Thanks a lot @mapleFU ! This is really appreciated.

Comment thread cpp/src/parquet/metadata.cc Resolved
Co-authored-by: Gang Wu <[email protected]>
@pitrou pitrou changed the title GH-35757: [C++][Parquet] using page-encoding-stats to build encodings GH-35757: [C++][Parquet] Build encodings metadata from actual encodings May 31, 2023
@pitrou
pitrou merged commit fee8bf9 into apache:main May 31, 2023
@ursabot

ursabot commented Jun 1, 2023

Copy link
Copy Markdown

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.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.91% ⬆️0.21%] test-mac-arm
[Finished ⬇️1.3% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.6% ⬆️0.03%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] fee8bf9c ec2-t3-xlarge-us-east-2
[Finished] fee8bf9c test-mac-arm
[Finished] fee8bf9c ursa-i9-9960x
[Finished] fee8bf9c ursa-thinkcentre-m75q
[Finished] 5c772af5 ec2-t3-xlarge-us-east-2
[Finished] 5c772af5 test-mac-arm
[Finished] 5c772af5 ursa-i9-9960x
[Finished] 5c772af5 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot

ursabot commented Jun 1, 2023

Copy link
Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++][Parquet] ColumnChunkMetadata: using encoding_stats to build encodings

4 participants