-
Notifications
You must be signed in to change notification settings - Fork 344
feat(datafusion): Support INSERT INTO partitioned tables
#1827
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
Conversation
liurenjie1024
left a comment
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.
Thanks @CTTY for this pr!
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_insert_into_partitioned() -> Result<()> { |
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.
Could we move this test to newly added sqllogictests? I think we should be able to read/write now.
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.
I think this makes sense, I'll work on adding sqllogictests for INSERT INTO overall.
This test case still has its value as it validates the inserted data files are put under the partitioned path correctly. I'll create a tracking issue as a follow up
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.
Created #1835
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.
I think the overall direction is to remove integration tests like this, and move them in sqllogictests. But we could delete this after we fix #1835 .
This test case still has its value as it validates the inserted data files are put under the partitioned path correctly.
I'm not convinced. Such detailed check could be done in ut of low level apis rather in integration tests.
liurenjie1024
left a comment
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.
Thanks @CTTY for this pr!
Which issue does this PR close?
INSERT INTOpartitioned data for DataFusion #1828insert_intoforIcebergTableProvider#1540What changes are included in this PR?
IcebergWriteExecto support partitioned dataAre these changes tested?
Added an ut