Skip to content

Use byte arrays for encoding short decimals in native hive parquet writer#12658

Merged
findepi merged 6 commits intotrinodb:masterfrom
raunaqmorarka:pq-writer-decimal
Jun 10, 2022
Merged

Use byte arrays for encoding short decimals in native hive parquet writer#12658
findepi merged 6 commits intotrinodb:masterfrom
raunaqmorarka:pq-writer-decimal

Conversation

@raunaqmorarka
Copy link
Copy Markdown
Member

@raunaqmorarka raunaqmorarka commented Jun 2, 2022 •
edited
Loading

Description

Apache Hive fails to read short decimals encoded as INT32/INT64 by the optimized parquet hive writer in Trino.
This PR changes the native writer to use fixed length byte arrays for encoding short decimals when writing parquet files in hive connector. This makes the output readable by Apache Hive.

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

Fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Parquet writer

How would you describe this change to a non-technical end user or system administrator?

Makes output of optimized parquet writer for short decimals compatible with Apache Hive.

Related issues, pull requests, and links

#6377
#10486

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# Section
* Fixes compatibility of files produced by optimized parquet writer for short decimal columns in Trino hive connnector with Apache Hive. ({issue}`12658`)

@cla-bot cla-bot bot added the cla-signed label Jun 2, 2022
@raunaqmorarka raunaqmorarka force-pushed the pq-writer-decimal branch 3 times, most recently from 86d68d6 to 756fc60 Compare June 2, 2022 16:51
skrzypo987
skrzypo987 approved these changes Jun 3, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@skrzypo987 skrzypo987 left a comment

Choose a reason for hiding this comment

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

LGTM % comments

@raunaqmorarka raunaqmorarka requested a review from skrzypo987 June 3, 2022 06:54
findepi
findepi reviewed Jun 6, 2022
View reviewed changes
@findepi
Copy link
Copy Markdown
Member

findepi commented Jun 6, 2022

I skipped over the actual writers, assuming @skrzypo987 took a look at them. Thanks @skrzypo987 .

@alexjo2144 can you PTAL too?

@raunaqmorarka raunaqmorarka requested a review from findepi June 6, 2022 20:06
findepi
findepi reviewed Jun 9, 2022
View reviewed changes
...parquet/src/main/java/io/trino/parquet/writer/valuewriter/BinaryShortDecimalValueWriter.java Outdated
Comment on lines 58 to 61
Copy link
Copy Markdown
Member

@findepi findepi Jun 9, 2022

Choose a reason for hiding this comment

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

Maybe unfold the loop with a switch on numBytes

trino/lib/trino-orc/src/main/java/io/trino/orc/stream/BooleanInputStream.java

Line 193 in 55df60d

switch (remaining) {

Copy link
Copy Markdown
Contributor

@findinpath findinpath Jun 10, 2022

Choose a reason for hiding this comment

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

@findepi what is the gain of doing this change?

Copy link
Copy Markdown
Member

@findepi findepi Jun 10, 2022

Choose a reason for hiding this comment

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

i expect them to perform differently.

findinpath
findinpath reviewed Jun 9, 2022
View reviewed changes
plugin/trino-hive/src/test/java/io/trino/plugin/hive/parquet/AbstractTestParquetReader.java Outdated
Copy link
Copy Markdown
Contributor

@findinpath findinpath Jun 9, 2022

Choose a reason for hiding this comment

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

This code snipped appears to be repeating (with slight modifications).
Consider creating a method for code reuse.

@findepi findepi force-pushed the pq-writer-decimal branch from 8b78956 to 62239be Compare June 9, 2022 20:31
@findepi
Copy link
Copy Markdown
Member

findepi commented Jun 9, 2022

(rebased because of a conflict)

@raunaqmorarka raunaqmorarka force-pushed the pq-writer-decimal branch 2 times, most recently from 8aa5304 to 9ad449b Compare June 10, 2022 06:47
@raunaqmorarka raunaqmorarka requested a review from findepi June 10, 2022 06:49
@raunaqmorarka raunaqmorarka requested a review from skrzypo987 June 10, 2022 06:49
@raunaqmorarka
Calculate long decimal length based on precision in parquet
7d1e544 
Avoids wasting space by always using 16 bytes even though
lower precision values can be stored using fewer bytes.
findepi
findepi reviewed Jun 10, 2022
View reviewed changes
@raunaqmorarka raunaqmorarka requested a review from findepi June 10, 2022 08:38
@raunaqmorarka raunaqmorarka requested a review from findepi June 10, 2022 09:41
@findepi
Copy link
Copy Markdown
Member

findepi commented Jun 10, 2022

Build is green (https://github.com/trinodb/trino/runs/6828591664?check_suite_focus=true). squashing and merging.

@raunaqmorarka @findepi
Use byte array encoding for decimals in hive parquet optimized writer
6229d51 
Delta Lake will continue to use integer encoding for short decimals.
This change fixes compatbility with Apache Hive which expects
decimals to be encoded as fixed length byte arrays.
@findepi findepi force-pushed the pq-writer-decimal branch from 3f5ab6b to 6229d51 Compare June 10, 2022 14:33
@findepi findepi merged commit f94ea19 into trinodb:master Jun 10, 2022
@raunaqmorarka raunaqmorarka deleted the pq-writer-decimal branch June 10, 2022 14:35
@findepi findepi mentioned this pull request Jun 10, 2022
Closed
@github-actions github-actions bot added this to the 386 milestone Jun 10, 2022
@colebow colebow mentioned this pull request Jun 10, 2022
Merged
@findepi findepi mentioned this pull request Jun 13, 2022
Merged
@raunaqmorarka raunaqmorarka mentioned this pull request Jun 24, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@qqibrow qqibrow Awaiting requested review from qqibrow

@skrzypo987 skrzypo987 Awaiting requested review from skrzypo987

@findepi findepi Awaiting requested review from findepi

1 more reviewer

@findinpath findinpath findinpath left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Milestone

386

Development

Successfully merging this pull request may close these issues.

4 participants

@raunaqmorarka @findepi @findinpath @skrzypo987