-
Notifications
You must be signed in to change notification settings - Fork 14
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
Iceberg sync #771
Iceberg sync #771
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, especially seeing how it is a smaller change than I anticipated.
@@ -609,3 +609,70 @@ async fn test_sync_custom_store( | |||
|
|||
Ok(()) | |||
} | |||
|
|||
#[tokio::test] | |||
async fn test_sync_iceberg_custom_store( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like test_sync_custom_store
and this test share the setup/results, maybe we can just parametrize the former to do exercise this routine for both formats (we can ditch the local
parametrization in it since it won't work for Iceberg)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setup, verification and parametrization are all subtly different so I'm not sure it's worth it.
Support Iceberg tables as sync targets