Fix writing bloom filter for string columns in orc by ans76 · Pull Request #11982 · trinodb/trino

ans76 · 2022-04-15T12:29:05Z

Description

Is this change a fix, improvement, new feature, refactoring, or other?
fix #11757

Documentation

( ) No documentation is needed.

Release notes

Fix writing bloom filter for String columns in ORC

sopel39 · 2022-04-15T13:45:19Z

lib/trino-orc/src/main/java/io/trino/orc/writer/SliceDictionaryColumnWriter.java

Is there a way to test it?

How it was broken? (improve commie message)

trino/lib/trino-orc/src/main/java/io/trino/orc/OrcWriter.java

Lines 386 to 406 in 2df56a0

// convert any dictionary encoded column with a low compression ratio to direct

dictionaryCompressionOptimizer.finalOptimize(bufferedBytes);

columnWriters.forEach(ColumnWriter::close);

List<OrcDataOutput> outputData = new ArrayList<>();

List<Stream> allStreams = new ArrayList<>(columnWriters.size() * 3);

// get index streams

long indexLength = 0;

for (ColumnWriter columnWriter : columnWriters) {

for (StreamDataOutput indexStream : columnWriter.getIndexStreams(metadataWriter)) {

// The ordering is critical because the stream only contain a length with no offset.

outputData.add(indexStream);

allStreams.add(indexStream.getStream());

indexLength += indexStream.size();

}

for (StreamDataOutput bloomFilter : columnWriter.getBloomFilters(metadataWriter)) {

outputData.add(bloomFilter);

allStreams.add(bloomFilter.getStream());

indexLength += bloomFilter.size();

SliceDictionaryColumnWriter#tryConvertToDirect will clear rowGroups finally in dictionary compression optimization, index streams use the rowGroups after optimization that result in losing bloom filter streams

sopel39 · 2022-04-20T08:51:24Z

@dain could you review this change. I don't have enough context here

Praveen2112

Can we add test to ensure that bloom filter is written for supported datatypes ?

Praveen2112 · 2022-05-20T12:20:16Z

@ans76 Can you please apply the comments.

lib/trino-orc/src/test/java/io/trino/orc/TestWriteBloomFilter.java

Praveen2112 · 2022-05-31T09:23:07Z

lib/trino-orc/src/test/java/io/trino/orc/TestWriteBloomFilter.java

DataProvider can be below the test method

Praveen2112 · 2022-05-31T09:23:37Z

lib/trino-orc/src/test/java/io/trino/orc/TestWriteBloomFilter.java

Can we inline this method ?

Praveen2112 · 2022-05-31T09:23:52Z

lib/trino-orc/src/test/java/io/trino/orc/TestWriteBloomFilter.java

static import for OrcWriteValidationMode

Do we need to check it for all validation modes ?

Praveen2112 · 2022-05-31T09:30:04Z

lib/trino-orc/src/test/java/io/trino/orc/TestWriteBloomFilter.java

Can we specific name for column instead of test

Praveen2112 · 2022-06-06T14:56:51Z

@alexjo2144 and @raunaqmorarka Can you PTAL ?

raunaqmorarka

Please update commit title to
Fix writing bloom filter for string columns in orc
Commit message

Ensures that bloom filters are written when the writer falls back to 
direct encoding due to dictionary becoming too large.

Also, please squash your commits.

raunaqmorarka · 2022-06-07T08:06:22Z

lib/trino-orc/src/test/java/io/trino/orc/TestWriteBloomFilter.java

The triggering conditions for the bug seems to be dictionary becoming too large and the writer falling back to non-dictionary encoding. But I don't see any tweaks to orc writer config (like reducing hive.orc.writer.dictionary-max-memory) which would guarantee that the conditions of the bug are reproduced.
It seems to me that it would be simpler to just test this in TestSliceDictionaryColumnWriter for CHAR, VARCHAR and STRING types and just call tryConvertToDirect and getBloomFilters directly in the unit test.

raunaqmorarka

Please squash your commits, lgtm otherwise

Ensures that bloom filters are written when the writer falls back to direct encoding due to dictionary becoming too large Co-Authored-By: Raunaq Morarka <raunaqmorarka@users.noreply.github.com>

Praveen2112 · 2022-06-22T13:49:15Z

Merged !! Thanks for fixing this

cla-bot bot added the cla-signed label Apr 15, 2022

findepi requested review from Praveen2112, dain and sopel39 April 15, 2022 12:53

sopel39 requested a review from skrzypo987 April 15, 2022 13:44

sopel39 reviewed Apr 15, 2022

View reviewed changes

github-actions bot added the tests:hive label Apr 15, 2022

sopel39 requested review from sopel39 and removed request for sopel39 April 20, 2022 08:50

Praveen2112 approved these changes Apr 22, 2022

View reviewed changes

Praveen2112 reviewed May 23, 2022

View reviewed changes

ans76 requested a review from Praveen2112 May 25, 2022 02:45

Praveen2112 reviewed May 31, 2022

View reviewed changes

Praveen2112 requested review from alexjo2144 and raunaqmorarka May 31, 2022 09:30

raunaqmorarka reviewed Jun 7, 2022

View reviewed changes

ans76 changed the title ~~Fix use DictionaryRowGroup statistics if convert to direct writer~~ Fix writing bloom filter for string columns in orc Jun 7, 2022

ans76 force-pushed the orc-slice-direct-use-dict-row-group-bloom-filter branch 2 times, most recently from 3840d20 to 495b045 Compare June 7, 2022 10:12

ans76 force-pushed the orc-slice-direct-use-dict-row-group-bloom-filter branch 4 times, most recently from 3747c21 to 338d973 Compare June 21, 2022 14:43

raunaqmorarka reviewed Jun 22, 2022

View reviewed changes

Fix writing bloom filter for string columns in orc

aa9b8dd

Ensures that bloom filters are written when the writer falls back to direct encoding due to dictionary becoming too large Co-Authored-By: Raunaq Morarka <raunaqmorarka@users.noreply.github.com>

ans76 force-pushed the orc-slice-direct-use-dict-row-group-bloom-filter branch from 338d973 to aa9b8dd Compare June 22, 2022 07:14

Praveen2112 merged commit 2e050f2 into trinodb:master Jun 22, 2022

github-actions bot added this to the 387 milestone Jun 22, 2022

ans76 deleted the orc-slice-direct-use-dict-row-group-bloom-filter branch June 22, 2022 13:59

colebow mentioned this pull request Jun 22, 2022

Add Trino 387 release notes #12881

Merged

	// convert any dictionary encoded column with a low compression ratio to direct
	dictionaryCompressionOptimizer.finalOptimize(bufferedBytes);

	columnWriters.forEach(ColumnWriter::close);

	List<OrcDataOutput> outputData = new ArrayList<>();
	List<Stream> allStreams = new ArrayList<>(columnWriters.size() * 3);

	// get index streams
	long indexLength = 0;
	for (ColumnWriter columnWriter : columnWriters) {
	for (StreamDataOutput indexStream : columnWriter.getIndexStreams(metadataWriter)) {
	// The ordering is critical because the stream only contain a length with no offset.
	outputData.add(indexStream);
	allStreams.add(indexStream.getStream());
	indexLength += indexStream.size();
	}
	for (StreamDataOutput bloomFilter : columnWriter.getBloomFilters(metadataWriter)) {
	outputData.add(bloomFilter);
	allStreams.add(bloomFilter.getStream());
	indexLength += bloomFilter.size();

Conversation

ans76 commented Apr 15, 2022 • edited by Praveen2112 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Documentation

Release notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sopel39 commented Apr 20, 2022

Uh oh!

Praveen2112 left a comment

Choose a reason for hiding this comment

Uh oh!

Praveen2112 commented May 20, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Praveen2112 commented Jun 6, 2022

Uh oh!

raunaqmorarka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raunaqmorarka left a comment

Choose a reason for hiding this comment

Uh oh!

Praveen2112 commented Jun 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

ans76 commented Apr 15, 2022 •

edited by Praveen2112

Loading