You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "rluvaton (via GitHub)" <gi...@apache.org> on 2025/02/20 15:24:05 UTC
[PR] feat: add to concat different data types error message the data types [arrow-rs]
rluvaton opened a new pull request, #7166:
URL: https://github.com/apache/arrow-rs/pull/7166
# Which issue does this PR close?
N/A
# Rationale for this change
Better debugging experience
# What changes are included in this PR?
Only added the unique data types in the concat message and updated the tests
# Are there any user-facing changes?
yes, they will see more helpful error message
--
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] feat: add to concat different data types error message the data types [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #7166:
URL: https://github.com/apache/arrow-rs/pull/7166#discussion_r1968118413
##########
arrow-select/src/concat.rs:
##########
@@ -223,8 +223,29 @@ pub fn concat(arrays: &[&dyn Array]) -> Result<ArrayRef, ArrowError> {
let d = arrays[0].data_type();
if arrays.iter().skip(1).any(|array| array.data_type() != d) {
+ // Get all the unique data types
+ let input_data_types = {
+ let unique = arrays
+ .iter()
+ .map(|array| array.data_type())
+ .collect::<HashSet<&DataType>>();
+
+ // Allow unused mut as we need it for tests
+ #[allow(unused_mut)]
+ let mut names = unique
+ .into_iter()
+ .map(|dt| format!("{dt}"))
+ .collect::<Vec<_>>();
+
+ // Only sort in tests to make the error message is deterministic
Review Comment:
Perhaps we could just use a BTreeSet? It will be slightly slower, but having non-deterministic error messages I think would be surprising for people.
--
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] feat: add to concat different data types error message the data types [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #7166:
URL: https://github.com/apache/arrow-rs/pull/7166
--
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] feat: add to concat different data types error message the data types [arrow-rs]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on code in PR #7166:
URL: https://github.com/apache/arrow-rs/pull/7166#discussion_r1983241347
##########
arrow-select/src/concat.rs:
##########
@@ -223,8 +223,29 @@ pub fn concat(arrays: &[&dyn Array]) -> Result<ArrayRef, ArrowError> {
let d = arrays[0].data_type();
if arrays.iter().skip(1).any(|array| array.data_type() != d) {
+ // Get all the unique data types
+ let input_data_types = {
+ let unique = arrays
+ .iter()
+ .map(|array| array.data_type())
+ .collect::<HashSet<&DataType>>();
+
+ // Allow unused mut as we need it for tests
+ #[allow(unused_mut)]
+ let mut names = unique
+ .into_iter()
+ .map(|dt| format!("{dt}"))
+ .collect::<Vec<_>>();
+
+ // Only sort in tests to make the error message is deterministic
Review Comment:
So I kept the HashSet but just for tracking unique values, and now the error message have the data type in the order of the input which is deterministic and better so people can get a sense about where the input exists
--
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] feat: add to concat different data types error message the data types [arrow-rs]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on PR #7166:
URL: https://github.com/apache/arrow-rs/pull/7166#issuecomment-2703669728
> I wonder if we need to incorporate some sort of cardinality limit here, e.g. similar to what we do when printing long arrays. I think this could potentially lead to long error messages, which in turn can lead to application hangs that are hard to diagnose.
>
> WDYT?
I've added a limit of 10
--
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] feat: add to concat different data types error message the data types [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #7166:
URL: https://github.com/apache/arrow-rs/pull/7166#issuecomment-2679200026
I wonder if we need to incorporate some sort of cardinality limit here, e.g. similar to what we do when printing long arrays. I think this could potentially lead to long error messages, which in turn can lead to application hangs that are hard to diagnose.
WDYT?
--
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