ARROW-9108: [C++][Dataset] Add supports for missing type in Statistics to Scalar conversion#7623
Conversation
fsaintjacques
force-pushed
the
ARROW-9108-parquet-timestamp-statistics
branch
from
5a904d5 to
ac58dec
Compare
jorisvandenbossche
left a comment
jorisvandenbossche
left a comment
There was a problem hiding this comment.
Looks good to me!
(I can also look into rebasing this once the scalar PR is merged)
What happens if a certain row group column has only nulls? Is the min/max then also null, or is it not defined? (might be good to add a test case for this)
nealrichardson
force-pushed
the
ARROW-9108-parquet-timestamp-statistics
branch
from
b961296 to
b277e3b
Compare
|
@jorisvandenbossche #7519 has merged so I just rebased this |
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
jorisvandenbossche
force-pushed
the
ARROW-9108-parquet-timestamp-statistics
branch
from
b277e3b to
c037e0f
Compare
|
OK, I now also enabled the commented-out tests |
It seems we don't write min/max statistics in such a case ( |
|
CI is green. @jorisvandenbossche is this good to merge? |
|
I think @bkietz is still taking a look today? |
| # Don't treat failure to parse/convert a single Scalar as a | ||
| # failure. The min/max will simply be missing for this field. |
There was a problem hiding this comment.
Should we add a test case for this? IIUC this will only arise when a parquet file has statistics of a type which pyarrow_wrap_scalar doesn't support; are we sure we want to hide that error?
There was a problem hiding this comment.
See also related question above: #7623 (comment)
I am not sure we can test it right now, as all scalars are supported in Python.
I suppose the rationale is that as a user experience it can be better to still return the statistics of all other columns, instead of raising due to one column not being recognized.
On the other hand, it indeed suppresses an error that would be useful to see (for us developers).
There was a problem hiding this comment.
IMHO it'd be better to raise an exception. If conversion fails in the future for a statistics scalar which we don't yet support then recognizing that issue will be easier than figuring out why the statistics disappear.
There was a problem hiding this comment.
We should support Scalars of all types, so it should not be possible for this to error, and if it does, it's a bug. IMO we should remove the try/except and move on.
|
Thanks @fsaintjacques ! |
4 participants
No description provided.