kbendick commented on pull request #3332: URL: https://github.com/apache/iceberg/pull/3332#issuecomment-954913614
Ah that's good idea. Wasn't a big fan of the multiple ternaries. On Fri, Oct 29, 2021 at 10:20 AM Ryan Blue ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java > <https://github.com/apache/iceberg/pull/3332#discussion_r739411538>: > > > @@ -262,6 +269,16 @@ private static Metrics buildOrcMetrics(final long numOfRows, final TypeDescripti > return Optional.ofNullable(Conversions.toByteBuffer(type, truncateIfNeeded(Bound.UPPER, type, max, metricsMode))); > } > > + // ORC uses NaN in its metrics for floating point numbers (float and double). > + // To avoid storing NaN in the Iceberg metrics, NaN is normalized to +/- Infinity for max / min respectively. > + private static Object normalizeFloatingPointColumnsIfNeeded(Bound bound, Type type, double value) { > + if (type.typeId() == Type.TypeID.DOUBLE) { > + return Double.isNaN(value) ? (bound == Bound.UPPER ? Double.POSITIVE_INFINITY : Double.NEGATIVE_INFINITY) : value; > > What about a simpler method, replaceNaN(double value, double replacement)? > Then you just need the ternary check here and you can customize with > Double.NEGATIVE_INFINITY in the call rather than passing a Bound. > > Also, you only need one since the bound is always a double to begin with. > So you can replace the logic with: > > max = replaceNaN(((DoubleColumnStatistics) columnStats).getMaximum(), Double.POSITIVE_INFINITY); > if (type.typeId() == Type.TypeID.FLOAT) { > max = ((Double) value).floatValue; > } > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <https://github.com/apache/iceberg/pull/3332#pullrequestreview-793286489>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ACLAXEQNIVK7XX2UOEYFGFLUJLJUDANCNFSM5GMSUBDA> > . > Triage notifications on the go with GitHub Mobile for iOS > <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> > or Android > <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. > > -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
