Skip to content

Conversation

@SinghAsDev
Copy link
Contributor

No description provided.

@SinghAsDev
Copy link
Contributor Author

@rdblue would you be able to provide some thoughts on this? Thanks

Copy link
Contributor

@kbendick kbendick left a comment

Choose a reason for hiding this comment

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

You might want to add tests for 3 level lists.

I know that there were some past PRs around 3 level lists, as parquet uses a different structure for them (or maybe certain engines did with certain versions). If you search closed PRs for 3 level you should find it.

@rdblue
Copy link
Contributor

rdblue commented Apr 15, 2022

@SinghAsDev, does reading 2-level lists work after this PR? Could you write an end-to-end test that validates it?

@SinghAsDev
Copy link
Contributor Author

Hi @rdblue @kbendick thanks for taking a look. This change works for both 2-level and 3-level lists. As part of adding support for 2-level lists, we added quite a few tests which work fine with this change. We have been running some workloads with this change at Pinterest as well. I will add an end-to-end test specifically for this edge case.

@rdblue rdblue merged commit c38f7b5 into apache:master May 2, 2022
@rdblue
Copy link
Contributor

rdblue commented May 2, 2022

Thanks, @SinghAsDev!

SinghAsDev added a commit to pinterest/iceberg that referenced this pull request May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants