Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Iceberg predicate pushdown #1561

Merged
merged 1 commit into from
Oct 22, 2019
Merged

Iceberg predicate pushdown #1561

merged 1 commit into from
Oct 22, 2019

Conversation

Praveen2112
Copy link
Member

@Praveen2112 Praveen2112 commented Sep 19, 2019 •
edited
Loading

Part of #1324

@cla-bot cla-bot bot added the cla-signed label Sep 19, 2019
@Praveen2112 Praveen2112 requested a review from lxynov September 19, 2019 11:04
@Praveen2112 Praveen2112 force-pushed the iceberg_predicate_pushdown branch from 5a87168 to 9b3a545 Compare September 19, 2019 14:29
@Praveen2112 Praveen2112 changed the title [WIP]: Iceberg predicate pushdown Iceberg predicate pushdown Sep 20, 2019
@Praveen2112 Praveen2112 force-pushed the iceberg_predicate_pushdown branch 3 times, most recently from 65b06c1 to 4170c47 Compare September 24, 2019 02:20
lxynov
lxynov reviewed Sep 25, 2019
View reviewed changes
presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergMetadata.java Outdated Show resolved Hide resolved
presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergSplitSource.java Outdated
@@ -116,7 +109,7 @@ public void close()
}
}

private ConnectorSplit toIcebergSplit(TupleDomain<HiveColumnHandle> predicate, FileScanTask task)
private ConnectorSplit toIcebergSplit(FileScanTask task)
{
// TODO: We should leverage residual expression and convert that to TupleDomain.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: update/remove TODOs when necessary.

presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergTableHandle.java Outdated Show resolved Hide resolved
presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergSplitManager.java Outdated Show resolved Hide resolved
@Praveen2112 Praveen2112 force-pushed the iceberg_predicate_pushdown branch from 4170c47 to e5fb3e3 Compare October 11, 2019 08:10
@lxynov
Copy link
Member

lxynov commented Oct 16, 2019

@Praveen2112
The logic looks good to me. Two things to note:

  1. We should think about how to return remaining filter to the engine in the future (as a future project).
  2. Did you get a chance to look into what ExpressionConverter and DomainConverter do? Their logic seems duplicate to me. We might want to remove DomainConverter if so.

@Parth-Brahmbhatt I'm curious about why we didn't implement applyFilter in the first PR. Is there any concern about this?

@electrum
Copy link
Member

The original version of the connector from @Parth-Brahmbhatt was written before applyFilter existed. I skipped converting the pushdown part for expediency when updating the PR.

@electrum electrum merged commit 014c1d5 into trinodb:master Oct 22, 2019
@electrum
Copy link
Member

Merged, thanks!

@Praveen2112 Praveen2112 deleted the iceberg_predicate_pushdown branch November 9, 2019 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lxynov lxynov lxynov left review comments

@Parth-Brahmbhatt Parth-Brahmbhatt Parth-Brahmbhatt left review comments

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Praveen2112 @lxynov @electrum @Parth-Brahmbhatt