[C++][Parquet] Misleading `GeoStatistics::dimension_empty` docstring

[pitrou]

Describe the bug, including details regarding any error messages, version, and platform.

The docstring for the new GeoStatistics::dimension_empty method is confusing as it contains a double negative (" zero non-NaN values were encountered").

Component(s)

C++

[paleolimbot]

Thanks for opening!

The current text reads:

arrow/cpp/src/parquet/geospatial/statistics.h

Lines 148 to 163 in baf97fd

	/// \brief Dimension emptiness in XYZM order
	///
	/// True for a given dimension if and only if zero non-NaN values were encountered
	/// in that dimension and dimension_valid() is true for that dimension.
	///
	/// When calculating statistics, zero or more of these values may be true because
	/// this implementation calculates bounds for all dimensions; however, it may be
	/// true that zero coordinates were encountered in a given dimension. For example,
	/// dimension_empty() will return four true values if Update() was not called
	/// or if Update() was called with only null values. If Update() was provided
	/// one or more geometries with X and Y dimensions but not Z or M dimensions,
	/// dimension_empty() will return true, true, false, false.
	///
	/// For statistics read from a Parquet file, dimension_empty() will always contain
	/// false values because there is no mechanism to communicate an empty interval
	/// in the Thrift metadata.

I am almost positive this is correct - zero finite (i.e., non-NaN) values is empty (true); if there are any finite values, it is non-empty (false). I feel like I'm missing something obvious here!

[pitrou]

Ah, no, I actually misread the docstring and missed the "zero" which amounts to a double negative.

1. How about changing "zero non-NaN values were encountered in that dimension" to "all values in that dimension are NaN"?
2. Why does dimension_empty() return false if dimension_valid() is false? Wouldn't it be easier for the user if dimension_empty() also returned true in that case?

[paleolimbot]

It's a great point that it's confusing!

How about changing "zero non-NaN values were encountered in that dimension" to "all values in that dimension are NaN"?

There also could have been no values in a given dimension (e.g., the example I give in the docstring where Update() was never called). I'll open a PR with a clarification shortly.

Why does dimension_empty() return false if dimension_valid() is false? Wouldn't it be easier for the user if dimension_empty() also returned true in that case?

I think this is more user-friendly as is...if you have a query rectangle, you can do if (dimension_empty(0) || dimension_empty(1)) skipThisRowGroup() without checking validity. I opened #46267 for just exposing a method that checks for intersection with a rectangle to handle those details for the common case 🙂

[pitrou]

I think this is more user-friendly as is...if you have a query rectangle, you can do if (dimension_empty(0) || dimension_empty(1)) skipThisRowGroup() without checking validity.

Can you? I think it's the reverse:

 /// True for a given dimension if and only if zero non-NaN values were encountered 
 /// in that dimension and dimension_valid() is true for that dimension. 

[wgtmac]

/// For statistics read from a Parquet file, dimension_empty() will always contain
/// false values because there is no mechanism to communicate an empty interval
/// in the Thrift metadata.

If dimension_empty() always returns false from deserialized thrift metadata, is it better to make it an internally writer-only state?