Skip to content

Add predicate to ParquetReader constructor in Iceberg#13804

Merged
raunaqmorarka merged 1 commit intotrinodb:masterfrom
charlesjmorgan:cm/pq-iceberg-params
Sep 7, 2022
Merged

Add predicate to ParquetReader constructor in Iceberg#13804
raunaqmorarka merged 1 commit intotrinodb:masterfrom
charlesjmorgan:cm/pq-iceberg-params

Conversation

@charlesjmorgan
Copy link
Copy Markdown
Member

@charlesjmorgan charlesjmorgan commented Aug 23, 2022

Description

Make parquetPredicate parameter Optional in ParquetReader constructor and pass Optional.empty() from Iceberg to ParquetReader constructor. Add requireNonNull to parquetPredicate and columnIndexStore in ParquetReader constructor since they can no longer be null.

Related issues, pull requests, and links

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

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

@electrum
Copy link
Copy Markdown
Member

It seems better to get rid of this constructor entirely: #13584

@charlesjmorgan
Copy link
Copy Markdown
Member Author

It seems better to get rid of this constructor entirely: #13584

@electrum It would be great if we could just get rid of this constructor, but I thought there was a problem with using ColumnIndexStore in parquet-mr?

@erichwang
Copy link
Copy Markdown
Contributor

@electrum It would be great if we could just get rid of this constructor, but I thought there was a problem with using ColumnIndexStore in parquet-mr?

@charlesjmorgan if there is something else requiring this constructor that is annoying to migrate before the constructor can be removed, i think we can do that later. It should be easy enough to do separately without affecting anything here.

Add requireNonNull on parquetPredicate and columnIndexStore in ParquetReader,
pass Optional.empty() from Iceberg for parquetPredicate param
@raunaqmorarka
Copy link
Copy Markdown
Member

Test failure is from #13889

@raunaqmorarka raunaqmorarka merged commit c72e3ba into trinodb:master Sep 7, 2022
@github-actions github-actions bot added this to the 395 milestone Sep 7, 2022
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.

6 participants