You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "tustvold (via GitHub)" <gi...@apache.org> on 2023/11/01 07:53:35 UTC
[PR] POC: Remove ListingTable Append Support (#7994) [arrow-datafusion]
tustvold opened a new pull request, #8017:
URL: https://github.com/apache/arrow-datafusion/pull/8017
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Relates to #7994
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
This is an extremely hacky POC to see how tightly coupled ListingTable is with append semantics and therefore how tractable it would be to split this into a separate TableProvider. The good news is it does not appear to be particularly tightly coupled, and so I am confident this is likely tractable
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove FileWriterMode (#7994) [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #8017:
URL: https://github.com/apache/arrow-datafusion/pull/8017#discussion_r1394365001
##########
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
Review Comment:
This is a change worth highlighting, inserting into a ListingTable backed by a single CSV or JSON file now overwrites
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove FileWriterMode (#7994) [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #8017:
URL: https://github.com/apache/arrow-datafusion/pull/8017#discussion_r1394365001
##########
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
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove FileWriterMode and ListingTableInsertMode (#7994) [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #8017:
URL: https://github.com/apache/arrow-datafusion/pull/8017#discussion_r1395463490
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -214,33 +214,6 @@ impl ListingTableConfig {
}
}
-#[derive(Debug, Clone)]
-///controls how new data should be inserted to a ListingTable
-pub enum ListingTableInsertMode {
- ///Data should be appended to an existing file
- AppendToFile,
- ///Data is appended as new files in existing TablePaths
- AppendNewFiles,
- ///Throw an error if insert into is attempted on this table
- Error,
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove FileWriterMode and ListingTableInsertMode (#7994) [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #8017:
URL: https://github.com/apache/arrow-datafusion/pull/8017#issuecomment-1816923380
Do you require further time? Happy to wait, just checking where we stand
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove FileWriterMode and ListingTableInsertMode (#7994) [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8017:
URL: https://github.com/apache/arrow-datafusion/pull/8017#discussion_r1395710136
##########
datafusion/core/src/datasource/file_format/write/mod.rs:
##########
@@ -19,128 +19,32 @@
//! write support for the various file formats
use std::io::Error;
-use std::mem;
use std::pin::Pin;
use std::sync::Arc;
use std::task::{Context, Poll};
use crate::datasource::file_format::file_compression_type::FileCompressionType;
-use crate::datasource::physical_plan::FileMeta;
use crate::error::Result;
use arrow_array::RecordBatch;
-use datafusion_common::{exec_err, DataFusionError};
+use datafusion_common::DataFusionError;
use async_trait::async_trait;
use bytes::Bytes;
use futures::future::BoxFuture;
-use futures::ready;
-use futures::FutureExt;
use object_store::path::Path;
-use object_store::{MultipartId, ObjectMeta, ObjectStore};
+use object_store::{MultipartId, ObjectStore};
use tokio::io::AsyncWrite;
pub(crate) mod demux;
pub(crate) mod orchestration;
-/// `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 {
Review Comment:
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
<img width="1570" alt="Screenshot 2023-11-16 at 8 40 03 AM" src="https://github.com/apache/arrow-datafusion/assets/490673/1f7e0dd1-3d19-4665-a051-abb2dd6af297">
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -214,33 +214,6 @@ impl ListingTableConfig {
}
}
-#[derive(Debug, Clone)]
-///controls how new data should be inserted to a ListingTable
-pub enum ListingTableInsertMode {
- ///Data should be appended to an existing file
- AppendToFile,
- ///Data is appended as new files in existing TablePaths
- AppendNewFiles,
- ///Throw an error if insert into is attempted on this table
- Error,
Review Comment:
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
##########
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
Review Comment:
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 ...`)?
##########
datafusion/core/src/datasource/file_format/write/mod.rs:
##########
@@ -163,45 +67,28 @@ impl MultiPart {
}
}
-pub(crate) enum AbortMode {
- Put,
- Append,
- MultiPart(MultiPart),
-}
-
/// 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.
Review Comment:
the doc comments may also need updating
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove ListingTable WriterMode Support (#7994) [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #8017:
URL: https://github.com/apache/arrow-datafusion/pull/8017#discussion_r1394348699
##########
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 {
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove FileWriterMode and ListingTableInsertMode (#7994) [arrow-datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on PR #8017:
URL: https://github.com/apache/arrow-datafusion/pull/8017#issuecomment-1814755465
Thank you, we will take a look tomorrow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove FileWriterMode (#7994) [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #8017:
URL: https://github.com/apache/arrow-datafusion/pull/8017#discussion_r1394371652
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -214,33 +214,6 @@ impl ListingTableConfig {
}
}
-#[derive(Debug, Clone)]
-///controls how new data should be inserted to a ListingTable
-pub enum ListingTableInsertMode {
- ///Data should be appended to an existing file
- AppendToFile,
- ///Data is appended as new files in existing TablePaths
- AppendNewFiles,
- ///Throw an error if insert into is attempted on this table
- Error,
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] POC: Remove ListingTable Append Support (#7994) [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold closed pull request #8017: POC: Remove ListingTable Append Support (#7994)
URL: https://github.com/apache/arrow-datafusion/pull/8017
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove FileWriterMode and ListingTableInsertMode (#7994) [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #8017:
URL: https://github.com/apache/arrow-datafusion/pull/8017#discussion_r1396099301
##########
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")
Review Comment:
Without this change you end up with an absolutely massive display line
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove FileWriterMode and ListingTableInsertMode (#7994) [arrow-datafusion]
Posted by "devinjdangelo (via GitHub)" <gi...@apache.org>.
devinjdangelo commented on code in PR #8017:
URL: https://github.com/apache/arrow-datafusion/pull/8017#discussion_r1394996546
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -214,33 +214,6 @@ impl ListingTableConfig {
}
}
-#[derive(Debug, Clone)]
-///controls how new data should be inserted to a ListingTable
-pub enum ListingTableInsertMode {
- ///Data should be appended to an existing file
- AppendToFile,
- ///Data is appended as new files in existing TablePaths
- AppendNewFiles,
- ///Throw an error if insert into is attempted on this table
- Error,
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove FileWriterMode and ListingTableInsertMode (#7994) [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #8017:
URL: https://github.com/apache/arrow-datafusion/pull/8017
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove FileWriterMode and ListingTableInsertMode (#7994) [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #8017:
URL: https://github.com/apache/arrow-datafusion/pull/8017#discussion_r1395001364
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -214,33 +214,6 @@ impl ListingTableConfig {
}
}
-#[derive(Debug, Clone)]
-///controls how new data should be inserted to a ListingTable
-pub enum ListingTableInsertMode {
- ///Data should be appended to an existing file
- AppendToFile,
- ///Data is appended as new files in existing TablePaths
- AppendNewFiles,
- ///Throw an error if insert into is attempted on this table
- Error,
Review Comment:
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove FileWriterMode (#7994) [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #8017:
URL: https://github.com/apache/arrow-datafusion/pull/8017#issuecomment-1812746779
FYI @devinjdangelo @alamb @ozankabak @metesynnada
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove FileWriterMode and ListingTableInsertMode (#7994) [arrow-datafusion]
Posted by "devinjdangelo (via GitHub)" <gi...@apache.org>.
devinjdangelo commented on code in PR #8017:
URL: https://github.com/apache/arrow-datafusion/pull/8017#discussion_r1395037988
##########
datafusion/core/src/datasource/listing/table.rs:
##########
@@ -214,33 +214,6 @@ impl ListingTableConfig {
}
}
-#[derive(Debug, Clone)]
-///controls how new data should be inserted to a ListingTable
-pub enum ListingTableInsertMode {
- ///Data should be appended to an existing file
- AppendToFile,
- ///Data is appended as new files in existing TablePaths
- AppendNewFiles,
- ///Throw an error if insert into is attempted on this table
- Error,
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove FileWriterMode and ListingTableInsertMode (#7994) [arrow-datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on PR #8017:
URL: https://github.com/apache/arrow-datafusion/pull/8017#issuecomment-1815003570
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove ListingTable WriterMode Support (#7994) [arrow-datafusion]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #8017:
URL: https://github.com/apache/arrow-datafusion/pull/8017#discussion_r1394347042
##########
datafusion/core/src/datasource/file_format/write/mod.rs:
##########
@@ -19,128 +19,32 @@
//! write support for the various file formats
use std::io::Error;
-use std::mem;
use std::pin::Pin;
use std::sync::Arc;
use std::task::{Context, Poll};
use crate::datasource::file_format::file_compression_type::FileCompressionType;
-use crate::datasource::physical_plan::FileMeta;
use crate::error::Result;
use arrow_array::RecordBatch;
-use datafusion_common::{exec_err, DataFusionError};
+use datafusion_common::DataFusionError;
use async_trait::async_trait;
use bytes::Bytes;
use futures::future::BoxFuture;
-use futures::ready;
-use futures::FutureExt;
use object_store::path::Path;
-use object_store::{MultipartId, ObjectMeta, ObjectStore};
+use object_store::{MultipartId, ObjectStore};
use tokio::io::AsyncWrite;
pub(crate) mod demux;
pub(crate) mod orchestration;
-/// `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 {
Review Comment:
This wasn't actually hooked up anywhere (and was actually incorrect and wouldn't have worked anyway #7991)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org