parquet: Add support for row group pruning on FixedSizeBinary #9646
Conversation
progval
force-pushed
the
fixedsizebinary-pruning
branch
from
acac9ec
to
588c5bc
Compare
progval
force-pushed
the
fixedsizebinary-pruning
branch
from
588c5bc
to
4740dac
Compare
progval
force-pushed
the
fixedsizebinary-pruning
branch
from
4740dac
to
496ba9a
Compare
| Some(DataType::Decimal128(precision, scale)) => { | ||
| Some(ScalarValue::Decimal128( | ||
| Some(from_bytes_to_i128(s.$bytes_func())), | ||
| *precision, | ||
| *scale, | ||
| )) | ||
| } | ||
| Some(DataType::FixedSizeBinary(size)) => { | ||
| Some(ScalarValue::FixedSizeBinary( |
There was a problem hiding this comment.
Should we verify that the resulting bytes have the correct size? As in verify that *size is the same as (s.$bytes_func().to_vec().len()?
If not, I suggest we ignore the error (but don't use the statistics) the same way as is done for non UTF8 values i string statistics. Perhaps we can at least debug when this happens with debug!
Since this data can come from any arbitrary parquet writer, I think it is good practice to validate what is in there
There was a problem hiding this comment.
Oh sure, good catch.
The code for strings is:
let s = std::str::from_utf8(s.$bytes_func())
.map(|s| s.to_string())
.ok();
Some(ScalarValue::Utf8(s))
which looks like it's treated as a NULL, rather than ignored as a statistics value. Should I do the same here?
There was a problem hiding this comment.
Yes, I think converting to NULL is the correct behavior. In this case that results in ignoring the value in terms of range analysis. More details are here if you are curious:
There was a problem hiding this comment.
I see, done. I also added a log statement for strings, for consistency.
Which issue does this PR close?
Closes #9645.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
It should be invisible other than a performance improvement.