Skip to content

Fix Parquet predicate pushdown for smallint, tinyint#131

Merged
findepi merged 6 commits intotrinodb:masterfrom
findepi:findepi/parquet-predicates
Feb 5, 2019
Merged

Fix Parquet predicate pushdown for smallint, tinyint#131
findepi merged 6 commits intotrinodb:masterfrom
findepi:findepi/parquet-predicates

Conversation

@findepi
Copy link
Member

@findepi findepi commented Feb 1, 2019

Fixes #123

This is refactoring commit. It is supposed not to change code behavior
regardless of anything, unless introduced short-circuiting prevents an
exception from being thrown.
This commit changes code behavior in the unlikely edge case where:

1. Domain constructed from file stats is empty (`isNone()`)
2. we did not skip the file/stripe based on `numberOfRows == 0`
   condition.
3. there is no effective predicate for a column

Previously, the code would construct realize the `Domain` constructed
from file stats `isNone()`. Now, the `Domain` construction will not be
attempted, as the code short-circuits when there no effective predicate
for a column. This change is based on assumption that the `Domain`
constructed from file stats cannot `isNone()` in any practical
situation.
When creating `Domain` from file stats, we need to know actual Presto
Type of the column, as Types must match for `Domain#intersect`.

Instead of reconstructing the type based on the type in the file, which
is not always possible, as Parquet does not distinguish between INTEGER,
SMALLINT etc, use the Type passed with effective predicate.
@cla-bot cla-bot bot added the cla-signed label Feb 1, 2019
@findepi findepi requested a review from kokosing February 1, 2019 12:34
@findepi
Copy link
Member Author

findepi commented Feb 1, 2019

@zhenxiao

@findepi
Copy link
Member Author

findepi commented Feb 1, 2019

@nishantrayan I saw yours prestodb/presto#11118 , which fixes ParquetTypeUtils#getPrestoType method, but i think a different approach is possible. In this PR, I remove ParquetTypeUtils#getPrestoType method instead of fixing it. Would you like to take a look?

Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

Reviewed internally

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

Development

Successfully merging this pull request may close these issues.

2 participants