Skip to content

Conversation

@Gowthami03B
Copy link
Contributor

No description provided.

@Gowthami03BGowthami03B changed the title files metadata tableAdd Files metadata tableApr 18, 2024
@Gowthami03BGowthami03B mentioned this pull request Apr 18, 2024
8 tasks
Copy link
Collaborator

@sungwysungwy left a comment

Choose a reason for hiding this comment

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

Hi @Gowthami03B this looks great. I have one request to add the optional snapshot_id argument to support time traveling between different snapshots Iceberg metadata table.

@sungwy
Copy link
Collaborator

Hi @HonahX could we get your help in triggering this workflow to see if the CI succeeds?

@Gowthami03BGowthami03B requested a review from geruhMay 12, 2024 22:11
Copy link
ContributorAuthor

@Gowthami03BGowthami03B left a comment

Choose a reason for hiding this comment

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

Hello @HonahX

Can I get a re-trigger again (the checks succeeds in my PR to my fork, so I am confident)?
Also requesting re-review here , I addressed the comments, hoping we could close this :)

@kevinjqliukevinjqliu mentioned this pull request May 14, 2024
39 tasks
Copy link
Contributor

@HonahXHonahX left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for adding this @Gowthami03B.

@Gowthami03BGowthami03B requested a review from geruhMay 22, 2024 19:20
@Gowthami03BGowthami03Bforce-pushed the metadata-files-table branch from 42d091d to 334cf94CompareMay 22, 2024 21:04
@Fokko
Copy link
Contributor

Sorry for now following up on this @Gowthami03B Could you rebase so we can get this in? Thanks!

@Fokko
Copy link
Contributor

@Gowthami03B gentle ping, this is the last metadata table, and we would love to include this into the release! 🙌

@Gowthami03B
Copy link
ContributorAuthor

@Fokko@kevinjqliu@amogh-jahagirdar Can I get a re-review here please? Want to close this asap for the release timeline :)

Copy link
Contributor

@kevinjqliukevinjqliu left a comment

Choose a reason for hiding this comment

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

LGTM!

r? @Fokko / @HonahX

@HonahX
Copy link
Contributor

LGTM too. @Gowthami03B Thanks for working on this! Thanks everyone for reviewing. Let's get this last metadata table in!

@HonahXHonahX merged commit 254a701 into apache:mainJul 4, 2024
@DieHertz
Copy link

DieHertz commented Sep 25, 2024

Hi guys, sorry if it's not the right place to ask this question.
Do you know of a viable way to speed up table.inspect.files() for large tables?
Maybe something in mind that I could implement and contribute to upstream.

I haven't profiled yet but I guess the gist of the issue is manifest.fetch_manifest_entry being called synchronously and sequentially in a loop.
Offloading this call to a thread-based executor doesn't help much, probably because of GIL, and a process-based executor is harder to implement because of unpicklable types involved.

As of now pyspark's .files metatable collection can be done considerably quicker than pyiceberg's

@kevinjqliu
Copy link
Contributor

I think there's definitely room for improvement. @DieHertz do you mind opening an issue for this?

@DieHertz
Copy link

Will do

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@Gowthami03B@sungwy@Fokko@HonahX@DieHertz@kevinjqliu@geruh@amogh-jahagirdar