Remove FileWriterMode and ListingTableInsertMode (#7994) #8017

tustvold · 2023-11-01T07:53:32Z

Which issue does this PR close?

Relates to #7994
Closes #7991

Rationale for this change

Following #8021 append semantics are handled by a separate TableProvider, this allows removing the corresponding logic from ListingTable.

Further cleanups will likely follow, but this is all that is necessary to unblock #8029

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

tustvold · 2023-11-15T15:12:48Z

datafusion/core/src/datasource/file_format/write/mod.rs

-/// `AsyncPutWriter` is an object that facilitates asynchronous writing to object stores.
-/// It is specifically designed for the `object_store` crate's `put` method and sends
-/// whole bytes at once when the buffer is flushed.
-pub struct AsyncPutWriter {


This wasn't actually hooked up anywhere (and was actually incorrect and wouldn't have worked anyway #7991)

Indeed I double checked and it appears to be dead code (unused and untested):

https://github.com/search?q=repo%3Aapache%2Farrow-datafusion%20AsyncPutWriter&type=code

tustvold · 2023-11-15T15:14:04Z

datafusion/core/src/datasource/file_format/write/mod.rs

@@ -229,16 +116,6 @@ impl<W: AsyncWrite + Unpin + Send> AsyncWrite for AbortableWrite<W> {
    }
 }

-/// An enum that defines different file writer modes.
-#[derive(Debug, Clone, Copy)]
-pub enum FileWriterMode {


Given FileWriterMode::Put is not implemented by anything, and we are removing FileWriterMode::Append, it seemed simplest to just remove this. This also frees up implementations to choose an adaptive IO strategy, where they can determine if Put is appropriate

tustvold · 2023-11-15T15:25:31Z

datafusion/sqllogictest/test_files/insert_to_external.slt

@@ -267,8 +267,6 @@ INSERT INTO single_file_test values (4, 5), (6, 7);
 query II
 select * from single_file_test;
 ----
-1 2


This is a change worth highlighting, inserting into a ListingTable backed by a single CSV or JSON file now overwrites. We could also make this error

I think it should be an error. It would also be really good to call out how to get "append" semantics (which I believe requires using CREATE UNBOUNDED TABLE ...)?

tustvold · 2023-11-15T15:27:28Z

FYI @devinjdangelo @alamb @ozankabak @metesynnada

tustvold · 2023-11-15T15:30:09Z

datafusion/core/src/datasource/listing/table.rs

-    ///Data is appended as new files in existing TablePaths
-    AppendNewFiles,
-    ///Throw an error if insert into is attempted on this table
-    Error,


It is worth highlighting this also removes ListingTableInsertMode::Error, I wasn't sure that this was important to preserve but happy to restore if people feel strongly. @devinjdangelo

Removing error is fine by me. A formal access control mechanism would be better than the error mode option.

I do think there is some value to append mode for ListingTable. Specifically it enables tables backed by a single file where inserts append to that file.

This is a relatively niche usecase so I'm not bothered if it is removed.

Specifically it enables tables backed by a single file where inserts append to that file.

I believe this use-case is now covered by StreamTable unless I am missing something here?

That could be (I haven't dug into StreamTable yet). The usecase for append I'm thinking of isn't really streaming related. More like a batch job that appends to one file or a fixed set of files on disk.

StreamTable will do the former, but cannot, yet, handle multiple files. We could add this but I would rather scream test if anyone is actually doing this

It would be good to hear about a usecase of appending to a set of existing files -- it is not a usecase I am familiar with but also my background is in analytic database where it is more common to make new files (and then "compact/rewrite" them as subsequent step) rather than modifying existing files

alamb

Thank you for this (and #8021) @tustvold -- I think this looks like a nice alighnment to separate object_store backed ListingTables and the more flexible StreamingTable.

I do think we should make writing into an existing file error (rather than change the behavior from append to overwrite) as it is a significant change that I think could result in dataloss for existing users who expected the old behavior.

I also don't think we should merge it until @ozankabak and @metegenez have a chance to weigh in and approve, given the potential overlap between what I believe they are working on and the changes in this PR.

alamb · 2023-11-16T13:40:30Z

datafusion/core/src/datasource/file_format/write/mod.rs

-/// `AsyncPutWriter` is an object that facilitates asynchronous writing to object stores.
-/// It is specifically designed for the `object_store` crate's `put` method and sends
-/// whole bytes at once when the buffer is flushed.
-pub struct AsyncPutWriter {


Indeed I double checked and it appears to be dead code (unused and untested):

https://github.com/search?q=repo%3Aapache%2Farrow-datafusion%20AsyncPutWriter&type=code

alamb · 2023-11-16T13:42:41Z

datafusion/core/src/datasource/file_format/write/mod.rs

 /// A wrapper struct with abort method and writer
 pub(crate) struct AbortableWrite<W: AsyncWrite + Unpin + Send> {
    writer: W,
-    mode: AbortMode,
+    multipart: MultiPart,
 }

 impl<W: AsyncWrite + Unpin + Send> AbortableWrite<W> {
    /// Create a new `AbortableWrite` instance with the given writer, and write mode.


the doc comments may also need updating

alamb · 2023-11-16T13:55:17Z

datafusion/core/src/datasource/listing/table.rs

-    ///Data is appended as new files in existing TablePaths
-    AppendNewFiles,
-    ///Throw an error if insert into is attempted on this table
-    Error,


It would be good to hear about a usecase of appending to a set of existing files -- it is not a usecase I am familiar with but also my background is in analytic database where it is more common to make new files (and then "compact/rewrite" them as subsequent step) rather than modifying existing files

alamb · 2023-11-16T13:56:54Z

datafusion/sqllogictest/test_files/insert_to_external.slt

@@ -267,8 +267,6 @@ INSERT INTO single_file_test values (4, 5), (6, 7);
 query II
 select * from single_file_test;
 ----
-1 2


I think it should be an error. It would also be really good to call out how to get "append" semantics (which I believe requires using CREATE UNBOUNDED TABLE ...)?

ozankabak · 2023-11-16T16:04:04Z

Thank you, we will take a look tomorrow

alamb

Thanks @tustvold -- this PR is good to go from my perspective, but just to be over communicative I do not think we should merge it until review and approval from @ozankabak / @metesynnada

tustvold · 2023-11-16T17:33:00Z

datafusion/core/src/datasource/stream.rs

@@ -283,7 +283,12 @@ struct StreamWrite(Arc<StreamConfig>);

 impl DisplayAs for StreamWrite {
    fn fmt_as(&self, _t: DisplayFormatType, f: &mut Formatter) -> std::fmt::Result {
-        write!(f, "{self:?}")
+        f.debug_struct("StreamWrite")


Without this change you end up with an absolutely massive display line

ozankabak · 2023-11-16T18:24:44Z

Thanks @tustvold -- this PR is good to go from my perspective, but just to be over communicative I do not think we should merge it until review and approval from @ozankabak / @metesynnada

Thanks, we will circle back tomorrow morning. I took a quick look and it looks good to me, but I want to consult with @metesynnada just to be sure.

tustvold · 2023-11-17T18:48:33Z

Do you require further time? Happy to wait, just checking where we stand

ozankabak

Thank you @tustvold, AFAICT this is good to go from our perspective. In case we are overlooking anything, we can send a quick follow-on PR. But all seems good now 🚀

tustvold closed this Nov 1, 2023

github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Nov 1, 2023

This was referenced Nov 1, 2023

Decouple Streaming Use-Case from File IO Abstractions #7994

Closed

Update arrow 49.0.0 and object_store 0.8.0 #8029

Merged

alamb mentioned this pull request Nov 9, 2023

Introduce a way to represent constrained statistics / bounds on values in Statistics #8078

Open

tustvold reopened this Nov 15, 2023

tustvold force-pushed the remove-listing-table-append branch from 1d7e8bd to 76624a6 Compare November 15, 2023 15:08

github-actions bot removed the sqllogictest SQL Logic Tests (.slt) label Nov 15, 2023

tustvold added the api change Changes the API exposed to users of the crate label Nov 15, 2023

tustvold changed the title ~~POC: Remove ListingTable Append Support (#7994)~~ Remove ListingTable WriterMode Support (#7994) Nov 15, 2023

tustvold mentioned this pull request Nov 15, 2023

Fix AsyncPutWriter (#7991) #7992

Closed

tustvold commented Nov 15, 2023

View reviewed changes

Remove FileWriterMode Support (apache#7994)

1e86301

tustvold force-pushed the remove-listing-table-append branch from 76624a6 to 1e86301 Compare November 15, 2023 15:23

tustvold changed the title ~~Remove ListingTable WriterMode Support (#7994)~~ Remove FileWriterMode (#7994) Nov 15, 2023

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Nov 15, 2023

tustvold commented Nov 15, 2023

View reviewed changes

Don't ignore test

8e1c2d2

tustvold marked this pull request as ready for review November 15, 2023 15:27

tustvold commented Nov 15, 2023

View reviewed changes

tustvold changed the title ~~Remove FileWriterMode (#7994)~~ Remove FileWriterMode and ListingTableInsertMode (#7994) Nov 15, 2023

alamb reviewed Nov 16, 2023

View reviewed changes

Error on insert to single file

7a5d876

alamb approved these changes Nov 16, 2023

View reviewed changes

Improve DisplayAs

0ae3d9e

tustvold commented Nov 16, 2023

View reviewed changes

ozankabak approved these changes Nov 17, 2023

View reviewed changes

tustvold merged commit 325a3fb into apache:main Nov 17, 2023
22 checks passed

tustvold mentioned this pull request Dec 14, 2023

Remove External Table Backwards Compatibility Options #8547

Closed

matthewgapp mentioned this pull request Jan 11, 2024

matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove FileWriterMode and ListingTableInsertMode (#7994) #8017

Remove FileWriterMode and ListingTableInsertMode (#7994) #8017

tustvold commented Nov 1, 2023 •

edited

Loading

tustvold Nov 15, 2023

alamb Nov 16, 2023

tustvold Nov 15, 2023

tustvold Nov 15, 2023 •

edited

Loading

alamb Nov 16, 2023

tustvold commented Nov 15, 2023

tustvold Nov 15, 2023

devinjdangelo Nov 15, 2023

tustvold Nov 16, 2023

devinjdangelo Nov 16, 2023

tustvold Nov 16, 2023

alamb Nov 16, 2023

alamb left a comment

alamb Nov 16, 2023

alamb Nov 16, 2023

alamb Nov 16, 2023

alamb Nov 16, 2023

ozankabak commented Nov 16, 2023

alamb left a comment •

edited

Loading

tustvold Nov 16, 2023

ozankabak commented Nov 16, 2023

tustvold commented Nov 17, 2023

ozankabak left a comment

Remove FileWriterMode and ListingTableInsertMode (#7994) #8017

Remove FileWriterMode and ListingTableInsertMode (#7994) #8017

Conversation

tustvold commented Nov 1, 2023 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold commented Nov 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozankabak commented Nov 16, 2023

alamb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozankabak commented Nov 16, 2023

tustvold commented Nov 17, 2023

ozankabak left a comment

Choose a reason for hiding this comment

tustvold commented Nov 1, 2023 •

edited

Loading

tustvold Nov 15, 2023 •

edited

Loading

alamb left a comment •

edited

Loading