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 provisional workaround to support CDC #1039 #1042

Merged
merged 2 commits into from
Dec 25, 2022
Merged

Conversation

Fazzani
Copy link
Contributor

@Fazzani Fazzani commented Dec 25, 2022

Description

This is only a workaround to the bug #1039

now it is possible to load datatables with the CDC option activated

Related Issue(s)

@Fazzani
Copy link
Contributor Author

Fazzani commented Dec 25, 2022

Hello,
I'm not an expert in Rust. Please check this workaround carefully #1042

By the way @herry13, I have used your struct AddCDCFile

Thx

Copy link
Collaborator

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

Thank you for adding the table and the test in Python.

The one main thing I'd like to see is a test in Rust. It would be appropriate here: https://github.com/delta-io/delta-rs/blob/main/rust/tests/read_delta_test.rs

rust/src/table_state.rs Show resolved Hide resolved
python/tests/test_table_read.py Outdated Show resolved Hide resolved
rust/src/action/mod.rs Outdated Show resolved Hide resolved
@Fazzani
Copy link
Contributor Author

Fazzani commented Dec 25, 2022

thank you @wjones127 for the review!
All feedback are fixed!

@Fazzani Fazzani requested review from wjones127 and removed request for rtyler, xianwill, houqp, mosyp, fvaleye and roeap December 25, 2022 20:17
Copy link
Collaborator

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

Thank you @Fazzani. This looks good now.

@Fazzani
Copy link
Contributor Author

Fazzani commented Dec 25, 2022

@wjones127
why it doesn't merged?

@wjones127 wjones127 merged commit 03151e7 into delta-io:main Dec 25, 2022
@wjones127
Copy link
Collaborator

@wjones127 why it doesn't merged?

I was waiting for all CI jobs to complete. Merged now. Thanks again!

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.

2 participants