You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/05/09 06:23:09 UTC
[GitHub] [arrow] houqp opened a new pull request #7133: ARROW-8744: [Rust] handle channel close in parquet batch iterator
houqp opened a new pull request #7133:
URL: https://github.com/apache/arrow/pull/7133
Once reached end of iteration, calling next on ParquetIterator will
result in an error. This is inconvenient in two ways:
* when shared between multiple threads, only one of the thread will be
able to terminate without error
* sender for response_rx cannot terminate the iteration early and free
up resources. instead, it needs to always wait for signal from
request_tx before closing up the connection
This patch changed the next method to always return None after
connection has been closed by the sender to address the above mentioned
issues.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #7133: ARROW-8744: [Rust] handle channel close in parquet batch iterator
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #7133:
URL: https://github.com/apache/arrow/pull/7133#discussion_r426705792
##########
File path: rust/arrow/src/record_batch.rs
##########
@@ -36,7 +36,7 @@ use crate::error::{ArrowError, Result};
/// serialization and computation functions, possibly incremental.
/// See also [CSV reader](crate::csv::Reader) and
/// [JSON reader](crate::json::Reader).
-#[derive(Clone)]
+#[derive(Clone, Debug)]
Review comment:
I don't have a strong opinion in this either. If you found it useful for debugging then perhaps other users would too. @nevi-me What do you think? Any reason not to enable Debug here? Presumably there is no cost to this unless people choose to print them?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #7133: ARROW-8744: [Rust] handle channel close in parquet batch iterator
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #7133:
URL: https://github.com/apache/arrow/pull/7133#discussion_r426186878
##########
File path: rust/arrow/src/record_batch.rs
##########
@@ -36,7 +36,7 @@ use crate::error::{ArrowError, Result};
/// serialization and computation functions, possibly incremental.
/// See also [CSV reader](crate::csv::Reader) and
/// [JSON reader](crate::json::Reader).
-#[derive(Clone)]
+#[derive(Clone, Debug)]
Review comment:
I'm curious what the debug out would be. Do you have an example?
##########
File path: rust/arrow/src/record_batch.rs
##########
@@ -36,7 +36,7 @@ use crate::error::{ArrowError, Result};
/// serialization and computation functions, possibly incremental.
/// See also [CSV reader](crate::csv::Reader) and
/// [JSON reader](crate::json::Reader).
-#[derive(Clone)]
+#[derive(Clone, Debug)]
Review comment:
I'm curious what the debug output would be. Do you have an example?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on a change in pull request #7133: ARROW-8744: [Rust] handle channel close in parquet batch iterator
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #7133:
URL: https://github.com/apache/arrow/pull/7133#discussion_r425255863
##########
File path: rust/arrow/src/record_batch.rs
##########
@@ -36,7 +36,7 @@ use crate::error::{ArrowError, Result};
/// serialization and computation functions, possibly incremental.
/// See also [CSV reader](crate::csv::Reader) and
/// [JSON reader](crate::json::Reader).
-#[derive(Clone)]
+#[derive(Clone, Debug)]
Review comment:
I found it useful to have this trait for debugging purpose. It is not used in the change I made in this PR. I will remove it if the policy is to avoid adding Debug trait unless necessary.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove closed pull request #7133: ARROW-8744: [Rust] handle channel close in parquet batch iterator
Posted by GitBox <gi...@apache.org>.
andygrove closed pull request #7133:
URL: https://github.com/apache/arrow/pull/7133
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on a change in pull request #7133: ARROW-8744: [Rust] handle channel close in parquet batch iterator
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #7133:
URL: https://github.com/apache/arrow/pull/7133#discussion_r426734661
##########
File path: rust/arrow/src/record_batch.rs
##########
@@ -36,7 +36,7 @@ use crate::error::{ArrowError, Result};
/// serialization and computation functions, possibly incremental.
/// See also [CSV reader](crate::csv::Reader) and
/// [JSON reader](crate::json::Reader).
-#[derive(Clone)]
+#[derive(Clone, Debug)]
Review comment:
I believe it does generate the code regardless even if it's not being used in debug print? if that's the case, it will increase size of binary.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #7133: ARROW-8744: [Rust] handle channel close in parquet batch iterator
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #7133:
URL: https://github.com/apache/arrow/pull/7133#discussion_r426945870
##########
File path: rust/arrow/src/record_batch.rs
##########
@@ -36,7 +36,7 @@ use crate::error::{ArrowError, Result};
/// serialization and computation functions, possibly incremental.
/// See also [CSV reader](crate::csv::Reader) and
/// [JSON reader](crate::json::Reader).
-#[derive(Clone)]
+#[derive(Clone, Debug)]
Review comment:
I think this is fine so went ahead and merged but we can revert the Debug change if anyone objects to this. Thanks for the fix @houqp !
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] houqp commented on a change in pull request #7133: ARROW-8744: [Rust] handle channel close in parquet batch iterator
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #7133:
URL: https://github.com/apache/arrow/pull/7133#discussion_r426196139
##########
File path: rust/arrow/src/record_batch.rs
##########
@@ -36,7 +36,7 @@ use crate::error::{ArrowError, Result};
/// serialization and computation functions, possibly incremental.
/// See also [CSV reader](crate::csv::Reader) and
/// [JSON reader](crate::json::Reader).
-#[derive(Clone)]
+#[derive(Clone, Debug)]
Review comment:
Here is what it looks like:
```
RecordBatch {
schema: Schema {
fields: [
Field {
name: "id",
data_type: Int64,
nullable: true,
dict_id: 0,
dict_is_ordered: false,
},
],
metadata: {},
},
columns: [
PrimitiveArray<Int64>
[
5,
],
],
}
```
Again, I am happy to push a fixup to delete the Debug trait if anyone prefers that. I don't have a strong opinion on this, just thought it could be handy for future developers.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #7133: ARROW-8744: [Rust] handle channel close in parquet batch iterator
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #7133:
URL: https://github.com/apache/arrow/pull/7133#discussion_r425138068
##########
File path: rust/arrow/src/record_batch.rs
##########
@@ -36,7 +36,7 @@ use crate::error::{ArrowError, Result};
/// serialization and computation functions, possibly incremental.
/// See also [CSV reader](crate::csv::Reader) and
/// [JSON reader](crate::json::Reader).
-#[derive(Clone)]
+#[derive(Clone, Debug)]
Review comment:
I don't see where you use the Debug
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7133: ARROW-8744: [Rust] handle channel close in parquet batch iterator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7133:
URL: https://github.com/apache/arrow/pull/7133#issuecomment-626115360
https://issues.apache.org/jira/browse/ARROW-8744
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org