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.

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

Add support iceberg parquet predicate pushdown with iceberg column id #19066

Merged

Conversation

Heltman
Copy link
Contributor

@Heltman Heltman commented Sep 18, 2023

Description

Fixes #18855. IcebergPageSourceProvider.getParquetTupleDomain get domain by column name. When column renamed in iceberg, this result is empty. We should use iceberg column id to match parquet column instead.

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Iceberg
* Fix predicate pushdown in parquet for renamed columns in Iceberg tables. (`18855`)

@cla-bot cla-bot bot added the cla-signed label Sep 18, 2023
@github-actions github-actions bot added the iceberg Iceberg connector label Sep 18, 2023
@Heltman Heltman requested a review from findinpath September 18, 2023 12:17
@Heltman
Copy link
Contributor Author

Heltman commented Sep 18, 2023

Do we have a test to check this problem? I found we don't have enough test about iceberg nested type and schema evolution. @findinpath

Copy link
Member

@homar homar left a comment

Choose a reason for hiding this comment

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

Change looks good but please add a test, for example to BaseIcebergConnectorTest that actually checks the behaviour.

Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Jan 10, 2024
@mosabua
Copy link
Member

mosabua commented Jan 10, 2024

👋 @Heltman @findinpath and @homar could you please collaborate on the test needs for this PR to move it forward.

@findinpath
Copy link
Contributor

findinpath commented Jan 11, 2024

i reached out over Slack to @Heltman and plan on continuing the effort on this bugfix. Thank you @Heltman for your contribution.

@findinpath
Copy link
Contributor

@Heltman pls add a test for your changes / reuse the test I suggested and also rebase on master.

Thank you very much for your contribution.

@raunaqmorarka raunaqmorarka force-pushed the iceberg-parquet-predicate-pushdown-by-id branch from 2427995 to 4b7f6da Compare January 11, 2024 08:44
@raunaqmorarka raunaqmorarka force-pushed the iceberg-parquet-predicate-pushdown-by-id branch from 4b7f6da to 05f81ee Compare January 11, 2024 08:52
@raunaqmorarka raunaqmorarka merged commit 65e9c64 into trinodb:master Jan 11, 2024
42 checks passed
@github-actions github-actions bot added this to the 436 milestone Jan 11, 2024
@Heltman
Copy link
Contributor Author

Heltman commented Jan 11, 2024

Sorry I don't check email. I'm glad this issue is resolved. Thanks for @findinpath and all members.

@mosabua
Copy link
Member

mosabua commented Jan 11, 2024

Awesome work on completing this together @findinpath @Heltman and @raunaqmorarka

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

Successfully merging this pull request may close these issues.

predicate doesn't take effect after renaming Iceberg table's column
5 participants