You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Rachelint (via GitHub)" <gi...@apache.org> on 2024/07/08 00:35:51 UTC
Re: [PR] Improve stats convert performance for Binary/String arrays [datafusion]
Rachelint commented on code in PR #11319:
URL: https://github.com/apache/datafusion/pull/11319#discussion_r1667817843
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -392,51 +393,73 @@ macro_rules! get_statistics {
})
},
DataType::Binary => Ok(Arc::new(BinaryArray::from_iter(
- [<$stat_type_prefix ByteArrayStatsIterator>]::new($iterator).map(|x| x.map(|x| x.to_vec())),
+ [<$stat_type_prefix ByteArrayStatsIterator>]::new($iterator)
))),
DataType::LargeBinary => Ok(Arc::new(LargeBinaryArray::from_iter(
- [<$stat_type_prefix ByteArrayStatsIterator>]::new($iterator).map(|x| x.map(|x|x.to_vec())),
- ))),
- DataType::Utf8 => Ok(Arc::new(StringArray::from_iter(
- [<$stat_type_prefix ByteArrayStatsIterator>]::new($iterator).map(|x| {
- x.and_then(|x| {
- let res = std::str::from_utf8(x).map(|s| s.to_string()).ok();
- if res.is_none() {
- log::debug!("Utf8 statistics is a non-UTF8 value, ignoring it.");
- }
- res
- })
- }),
+ [<$stat_type_prefix ByteArrayStatsIterator>]::new($iterator)
))),
+ DataType::Utf8 => {
+ let iterator = [<$stat_type_prefix ByteArrayStatsIterator>]::new($iterator);
+ let mut builder = StringBuilder::new();
+ for x in iterator {
+ let Some(x) = x else {
+ builder.append_null(); // no statistics value
+ continue;
+ };
+
+ let Ok(x) = std::str::from_utf8(x) else {
+ log::debug!("Utf8 statistics is a non-UTF8 value, ignoring it.");
+ builder.append_null();
+ continue;
+ };
+
+ builder.append_value(x);
+ }
+ Ok(Arc::new(builder.finish()))
+ },
DataType::LargeUtf8 => {
- Ok(Arc::new(LargeStringArray::from_iter(
- [<$stat_type_prefix ByteArrayStatsIterator>]::new($iterator).map(|x| {
- x.and_then(|x| {
- let res = std::str::from_utf8(x).map(|s| s.to_string()).ok();
- if res.is_none() {
- log::debug!("LargeUtf8 statistics is a non-UTF8 value, ignoring it.");
- }
- res
- })
- }),
- )))
- }
- DataType::FixedSizeBinary(size) => Ok(Arc::new(FixedSizeBinaryArray::from(
- [<$stat_type_prefix FixedLenByteArrayStatsIterator>]::new($iterator).map(|x| {
- x.and_then(|x| {
- if x.len().try_into() == Ok(*size) {
- Some(x)
- } else {
- log::debug!(
- "FixedSizeBinary({}) statistics is a binary of size {}, ignoring it.",
- size,
- x.len(),
- );
- None
- }
- })
- }).collect::<Vec<_>>(),
- ))),
+ let iterator = [<$stat_type_prefix ByteArrayStatsIterator>]::new($iterator);
+ let mut builder = LargeStringBuilder::new();
+ for x in iterator {
+ let Some(x) = x else {
+ builder.append_null(); // no statistics value
+ continue;
+ };
+
+ let Ok(x) = std::str::from_utf8(x) else {
+ log::debug!("Utf8 statistics is a non-UTF8 value, ignoring it.");
+ builder.append_null();
+ continue;
+ };
+
+ builder.append_value(x);
+ }
+ Ok(Arc::new(builder.finish()))
+ },
+ DataType::FixedSizeBinary(size) => {
+ let iterator = [<$stat_type_prefix FixedLenByteArrayStatsIterator>]::new($iterator);
+ let mut builder = FixedSizeBinaryBuilder::new(*size);
+ for x in iterator {
+ let Some(x) = x else {
+ builder.append_null(); // no statistics value
+ continue;
+ };
+
+ // ignore invalid values
+ if x.len().try_into() != Ok(*size){
+ log::debug!(
+ "FixedSizeBinary({}) statistics is a binary of size {}, ignoring it.",
+ size,
+ x.len(),
+ );
+ builder.append_null();
+ continue;
+ }
+
+ builder.append_value(x).expect("ensure to append successfully here, because size have been checked before");
Review Comment:
> Rather than using `expect` it is likely possible to check the value of `builder.append_value` and if it is an error append null. But perhaps that is overly confusing. This formulation is quite clear and explicit
:thinking: Yes, I `append_null` before. And I read the codes of `append_value` after, I found the only reason to return Err is to append a value with different width that has been checked and ensured actually.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org