clee704 commented on code in PR #50183:
URL: https://github.com/apache/arrow/pull/50183#discussion_r3415966515
##########
cpp/src/parquet/statistics.cc:
##########
@@ -330,6 +351,17 @@ struct CompareHelper<Float16LogicalType,
/*is_signed=*/true> {
using ::std::optional;
+// A usable min/max pair always satisfies min <= max. The reverse ordering
+// (min > max) is produced only by the inverted DefaultMin()/DefaultMax() seeds
+// -- left in place when no valid, non-NaN value was observed -- or by an
+// inverted caller-supplied range; in either case there is no statistic to
emit.
+// Testing the ordering keeps this independent of the specific seed values, so
it
+// cannot drift from DefaultMin()/DefaultMax().
+template <typename T>
+bool IsInvalidMinMax(const T& min, const T& max) {
+ return max < min;
+}
Review Comment:
`IsInvalidMinMax` already has internal linkage: it sits inside the
file-level anonymous namespace (opened at statistics.cc:50, closed at :1042)
that also encloses its sibling helpers (`CompareHelper`, `Float16Constants`,
`CleanStatistic`), none of which are marked `static` either. So no
anonymous-namespace wrapper or `static` needs to be added.
Confirmed in the compiled object — `nm` reports the symbol as local
(lowercase `t`), demangled as `parquet::(anonymous
namespace)::IsInvalidMinMax<...>`:
```
t bool parquet::(anonymous namespace)::IsInvalidMinMax<double>(double
const&, double const&)
```
There is therefore no external-linkage symbol and no cross-translation-unit
multiple-definition risk.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
