Uh oh!
There was an error while loading. Please reload this page.
Extend insert into support to include Json backed tables - #7212
Conversation
metesynnada
left a comment
There was a problem hiding this comment.
Overall looks good. I appreciate your hard work. However, I have made some comments regarding the changes.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| /// Serialization is assumed to be stateless, i.e. | ||
| /// each RecordBatch can be serialized without any | ||
| /// dependency on the RecordBatches before or after. | ||
| async fn stateless_serialize_and_write_files( |
There was a problem hiding this comment.
It makes sense to consolidate these into a unified approach.
There was a problem hiding this comment.
I agree -- and I think that will mean when we parallelize the logic more all the writers will benefit
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Metehan Yıldırım <100111937+metesynnada@users.noreply.github.com>
| } | ||
| FileWriterMode::PutMultipart => { | ||
| //currently assuming only 1 partition path (i.e. not hivestyle partitioning on a column) | ||
| // Currently assuming only 1 partition path (i.e. not hive-style partitioning on a column) |
alamb
commented
Aug 7, 2023
Thank you -- I quickly skimmed this PR and it looks great @devinjdangelo -- thank you for the review @devinjdangelo . I will take a closer look tomorrow morning. |
alamb
left a comment
There was a problem hiding this comment.
The code looks great to me -- thank you @devinjdangelo and @metesynnada for the review. I tried it out locally and it was 👌 very nice.
It is somewhat awkward at the moment to use this feature as you can't create new tables, only append to existing:
$ mkdir /tmp/my_table
❯ create external table my_table(x int, y int) stored as JSON location '/tmp/my_table';
0 rows in set. Query took 0.002 seconds.
❯ insert into my_table values (1,2), (3, 4);
Error during planning: Cannot append 1 partitions to 0 files!
I filed #7228 to track improving this
| /// Serialization is assumed to be stateless, i.e. | ||
| /// each RecordBatch can be serialized without any | ||
| /// dependency on the RecordBatches before or after. | ||
| async fn stateless_serialize_and_write_files( |
There was a problem hiding this comment.
I agree -- and I think that will mean when we parallelize the logic more all the writers will benefit
| .map_err(|e| DataFusionError::Internal(e.to_string()))?; | ||
| // Read the records in the table | ||
| let batches = session_ctx.sql("select * from t").await?.collect().await?; |
alamb
commented
Aug 8, 2023
Again, thanks again ! |
alamb
commented
Aug 8, 2023
Here is a small follow on to reduce some duplication #7229 |
Which issue does this PR close?
None, but progresses towards the goals of #5076 and #7079. Follow on to #7141.
Rationale for this change
Adds support for
insert into <table>for tables which are backed by Json files.What changes are included in this PR?
JsonSinkin similar fashion toCsvSinkCsvSinkto support code reuse withJsonSinkinsert intoto be easily extensible for additionalFileFormatsandoptionsListingTableAre these changes tested?
Yes
Are there any user-facing changes?
Insert into Json table will work now.