You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/10/20 21:08:40 UTC
[GitHub] [iceberg] kbendick opened a new pull request #3332: Fix importing orc files double float metrics
kbendick opened a new pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332
This adds tests for the changes to support importing ORC files with floats and doubles in https://github.com/apache/iceberg/pull/3320
This also adds the same fix for the metrics situation for `max`.
I had to update the Spark3 Extensions gradle build file. But given that iceberg-data and iceberg-orc are already added as `implementation` in the top level spark project, I'm wondering if this is ok? Otherwise I can find a different route to test it.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r737697502
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -234,11 +240,18 @@ private static Metrics buildOrcMetrics(final long numOfRows, final TypeDescripti
max = Math.toIntExact((long) max);
}
} else if (columnStats instanceof DoubleColumnStatistics) {
- // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
- // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
- Preconditions.checkNotNull(fieldMetrics,
- "[BUG] Float or double type columns should have metrics being tracked by Iceberg Orc writers");
- max = fieldMetrics.upperBound();
+ if (fieldMetrics != null) {
+ // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
+ // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
+ max = fieldMetrics.upperBound();
+ } else {
+ // imported files will not have metrics that were tracked by Iceberg, so fall back to the file's metrics.
+ max = ((DoubleColumnStatistics) columnStats).getMaximum();
Review comment:
Cool that makes sense. Will fix.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r737653699
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -234,11 +240,18 @@ private static Metrics buildOrcMetrics(final long numOfRows, final TypeDescripti
max = Math.toIntExact((long) max);
}
} else if (columnStats instanceof DoubleColumnStatistics) {
- // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
- // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
- Preconditions.checkNotNull(fieldMetrics,
- "[BUG] Float or double type columns should have metrics being tracked by Iceberg Orc writers");
- max = fieldMetrics.upperBound();
+ if (fieldMetrics != null) {
+ // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
+ // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
+ max = fieldMetrics.upperBound();
+ } else {
+ // imported files will not have metrics that were tracked by Iceberg, so fall back to the file's metrics.
+ max = ((DoubleColumnStatistics) columnStats).getMaximum();
Review comment:
Yes, and the same comments above apply here.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r736975765
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -196,11 +195,17 @@ private static Metrics buildOrcMetrics(final long numOfRows, final TypeDescripti
min = Math.toIntExact((long) min);
}
} else if (columnStats instanceof DoubleColumnStatistics) {
- // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
- // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
- Preconditions.checkNotNull(fieldMetrics,
- "[BUG] Float or double type columns should have metrics being tracked by Iceberg Orc writers");
- min = fieldMetrics.lowerBound();
+ if (fieldMetrics != null) {
+ // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
+ // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
+ min = fieldMetrics.lowerBound();
+ } else {
+ // imported files will not have metrics that were tracked by Iceberg, so fall back to the file's metrics.
+ min = ((DoubleColumnStatistics) columnStats).getMinimum();
+ if (type.typeId() == Type.TypeID.FLOAT) {
+ min = ((Double) min).floatValue();
Review comment:
I agree here. Let's replace any NaN bounds with -Infinity and +Infinity. You can't add a NaN value in a filter now, so this change is safe and a good idea to make sure we don't drop values from bad bounds.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r733175601
##########
File path: spark/v3.0/spark3-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java
##########
@@ -646,6 +656,43 @@ public void duplicateDataUnpartitionedAllowed() {
}
+
Review comment:
Nit: unnecessary newline.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#issuecomment-952429720
@yyanyy @rdblue I updated to handle non-finite metrics from Orc files. Can you please take a look? Thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r733176756
##########
File path: spark/v3.0/spark3-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java
##########
@@ -735,4 +782,35 @@ private void createPartitionedHiveTable() {
partitionedDF.write().insertInto(sourceTableName);
partitionedDF.write().insertInto(sourceTableName);
}
+
+ public List<Record> createOrcFile(File orcFile, int numRows) throws IOException {
+ // Needs to be deleted but depend on the rule to delete the file for us again at the end.
+ if (orcFile.exists()) {
+ orcFile.delete();
+ }
+ final org.apache.iceberg.Schema icebergSchema = new org.apache.iceberg.Schema(
Review comment:
Is it possible to write to ORC without adding field IDs? We shouldn't allow importing files that already have field IDs, so if we disable that later that would cause this test to fail.
What about creating a DataFrame and writing directly to ORC using Spark instead? That would probably be shorter, too.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] yyanyy commented on a change in pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
yyanyy commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r733955753
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -196,11 +195,17 @@ private static Metrics buildOrcMetrics(final long numOfRows, final TypeDescripti
min = Math.toIntExact((long) min);
}
} else if (columnStats instanceof DoubleColumnStatistics) {
- // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
- // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
- Preconditions.checkNotNull(fieldMetrics,
- "[BUG] Float or double type columns should have metrics being tracked by Iceberg Orc writers");
- min = fieldMetrics.lowerBound();
+ if (fieldMetrics != null) {
+ // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
+ // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
+ min = fieldMetrics.lowerBound();
+ } else {
+ // imported files will not have metrics that were tracked by Iceberg, so fall back to the file's metrics.
+ min = ((DoubleColumnStatistics) columnStats).getMinimum();
+ if (type.typeId() == Type.TypeID.FLOAT) {
+ min = ((Double) min).floatValue();
Review comment:
(paste the same comment from the previous PR) wondering if we want to reassign min to negative infinite if the this value tracked by ORC is NaN (and also max), as ORC could include NaN for upper/lower bounds of floating point columns as in the comment above
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3332: Test for fix importing orc files with doubles and floats
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r733149334
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -234,11 +240,17 @@ private static Metrics buildOrcMetrics(final long numOfRows, final TypeDescripti
max = Math.toIntExact((long) max);
}
} else if (columnStats instanceof DoubleColumnStatistics) {
- // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
- // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
- Preconditions.checkNotNull(fieldMetrics,
- "[BUG] Float or double type columns should have metrics being tracked by Iceberg Orc writers");
- max = fieldMetrics.upperBound();
+ if (fieldMetrics != null) {
+ // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
+ // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
+ max = fieldMetrics.upperBound();
+ } else {
+ // imported files will not have metrics that were tracked by Iceberg, so fall back to the file's metrics.
+ max = ((DoubleColumnStatistics) columnStats).getMaximum();
+ if (type.typeId() == Type.TypeID.FLOAT) {
+ max = ((Double) max).floatValue();
+ }
+ }
Review comment:
I tested without this fix and encountered the error, so I'm sure that the test does in fact hit the problem.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3332: Test for fix importing orc files with doubles and floats
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r733148865
##########
File path: spark/v3.0/build.gradle
##########
@@ -105,6 +105,8 @@ project(":iceberg-spark:iceberg-spark3-extensions") {
compileOnly project(path: ':iceberg-bundled-guava', configuration: 'shadow')
compileOnly project(':iceberg-api')
compileOnly project(':iceberg-core')
+ compileOnly project(':iceberg-data')
+ compileOnly project(':iceberg-orc')
Review comment:
These are already listed as `implementation` higher up, so I'm wondering if this is ok as it's `compileOnly` and they wind up in the resulting Spark3 runtime jars anway?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r739411538
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -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;
Review comment:
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:
```java
max = replaceNaN(((DoubleColumnStatistics) columnStats).getMaximum(), Double.POSITIVE_INFINITY);
if (type.typeId() == Type.TypeID.FLOAT) {
max = ((Double) value).floatValue;
}
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r737024751
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -234,11 +240,18 @@ private static Metrics buildOrcMetrics(final long numOfRows, final TypeDescripti
max = Math.toIntExact((long) max);
}
} else if (columnStats instanceof DoubleColumnStatistics) {
- // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
- // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
- Preconditions.checkNotNull(fieldMetrics,
- "[BUG] Float or double type columns should have metrics being tracked by Iceberg Orc writers");
- max = fieldMetrics.upperBound();
+ if (fieldMetrics != null) {
+ // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
+ // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
+ max = fieldMetrics.upperBound();
+ } else {
+ // imported files will not have metrics that were tracked by Iceberg, so fall back to the file's metrics.
+ max = ((DoubleColumnStatistics) columnStats).getMaximum();
Review comment:
Do we need to check this max for NaN as well? I'm assuming yes if that's what ORC is returning by design.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r733175322
##########
File path: spark/v3.0/build.gradle
##########
@@ -105,6 +105,8 @@ project(":iceberg-spark:iceberg-spark3-extensions") {
compileOnly project(path: ':iceberg-bundled-guava', configuration: 'shadow')
compileOnly project(':iceberg-api')
compileOnly project(':iceberg-core')
+ compileOnly project(':iceberg-data')
+ compileOnly project(':iceberg-orc')
Review comment:
Yeah, it should be fine.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r733190840
##########
File path: spark/v3.0/spark3-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java
##########
@@ -735,4 +782,35 @@ private void createPartitionedHiveTable() {
partitionedDF.write().insertInto(sourceTableName);
partitionedDF.write().insertInto(sourceTableName);
}
+
+ public List<Record> createOrcFile(File orcFile, int numRows) throws IOException {
+ // Needs to be deleted but depend on the rule to delete the file for us again at the end.
+ if (orcFile.exists()) {
+ orcFile.delete();
+ }
+ final org.apache.iceberg.Schema icebergSchema = new org.apache.iceberg.Schema(
Review comment:
The problem with using Spark directly (and trying to use the Java API instructions from the ORC website) is that `'org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch` isn't on the classpath, even after my changes.
That's why the current ORC tests in here which use Spark are disabled.
I'll keep looking for a way though.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r736980244
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -196,11 +195,17 @@ private static Metrics buildOrcMetrics(final long numOfRows, final TypeDescripti
min = Math.toIntExact((long) min);
}
} else if (columnStats instanceof DoubleColumnStatistics) {
- // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
- // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
- Preconditions.checkNotNull(fieldMetrics,
- "[BUG] Float or double type columns should have metrics being tracked by Iceberg Orc writers");
- min = fieldMetrics.lowerBound();
+ if (fieldMetrics != null) {
+ // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
+ // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
+ min = fieldMetrics.lowerBound();
+ } else {
+ // imported files will not have metrics that were tracked by Iceberg, so fall back to the file's metrics.
+ min = ((DoubleColumnStatistics) columnStats).getMinimum();
+ if (type.typeId() == Type.TypeID.FLOAT) {
+ min = ((Double) min).floatValue();
Review comment:
That makes sense to me. I'll push a fix ASAP.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r739413295
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -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;
Review comment:
Ah that's a good idea. Wasn't a fan of the double ternary.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r737824822
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -196,11 +195,18 @@ private static Metrics buildOrcMetrics(final long numOfRows, final TypeDescripti
min = Math.toIntExact((long) min);
}
} else if (columnStats instanceof DoubleColumnStatistics) {
- // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
- // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
- Preconditions.checkNotNull(fieldMetrics,
- "[BUG] Float or double type columns should have metrics being tracked by Iceberg Orc writers");
- min = fieldMetrics.lowerBound();
+ if (fieldMetrics != null) {
+ // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
+ // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
+ min = fieldMetrics.lowerBound();
+ } else {
+ // imported files will not have metrics that were tracked by Iceberg, so fall back to the file's metrics.
+ min = ((DoubleColumnStatistics) columnStats).getMinimum();
+ if (type.typeId() == Type.TypeID.FLOAT) {
+ float orcMin = ((Double) min).floatValue();
+ min = Float.isFinite(orcMin) ? orcMin : Float.NEGATIVE_INFINITY;
Review comment:
Good catch.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r737653511
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -196,11 +195,18 @@ private static Metrics buildOrcMetrics(final long numOfRows, final TypeDescripti
min = Math.toIntExact((long) min);
}
} else if (columnStats instanceof DoubleColumnStatistics) {
- // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
- // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
- Preconditions.checkNotNull(fieldMetrics,
- "[BUG] Float or double type columns should have metrics being tracked by Iceberg Orc writers");
- min = fieldMetrics.lowerBound();
+ if (fieldMetrics != null) {
+ // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
+ // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
+ min = fieldMetrics.lowerBound();
+ } else {
+ // imported files will not have metrics that were tracked by Iceberg, so fall back to the file's metrics.
+ min = ((DoubleColumnStatistics) columnStats).getMinimum();
+ if (type.typeId() == Type.TypeID.FLOAT) {
+ float orcMin = ((Double) min).floatValue();
+ min = Float.isFinite(orcMin) ? orcMin : Float.NEGATIVE_INFINITY;
Review comment:
I don't think that `isFinite` is right. The min could be +Infinity if all values are +Infinity. If that were the case, then this would make the stats span all of [-Infinity, +Infinity] rather than just [+Infinity, +Infinity].
Instead, this should use `Float.isNaN`:
```java
min = Float.isNaN(orcMin) ? Float.NEGATIVE_INFINITY : orcMin;
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r733210711
##########
File path: spark/v3.0/spark3-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java
##########
@@ -735,4 +782,35 @@ private void createPartitionedHiveTable() {
partitionedDF.write().insertInto(sourceTableName);
partitionedDF.write().insertInto(sourceTableName);
}
+
+ public List<Record> createOrcFile(File orcFile, int numRows) throws IOException {
+ // Needs to be deleted but depend on the rule to delete the file for us again at the end.
+ if (orcFile.exists()) {
+ orcFile.delete();
+ }
+ final org.apache.iceberg.Schema icebergSchema = new org.apache.iceberg.Schema(
Review comment:
We could also always store an ORC file in `resources` directory, which would enable us to turn on maybe 2-4 more tests that are in this file.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r737651825
##########
File path: orc/src/main/java/org/apache/iceberg/orc/OrcMetrics.java
##########
@@ -196,11 +195,18 @@ private static Metrics buildOrcMetrics(final long numOfRows, final TypeDescripti
min = Math.toIntExact((long) min);
}
} else if (columnStats instanceof DoubleColumnStatistics) {
- // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
- // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
- Preconditions.checkNotNull(fieldMetrics,
- "[BUG] Float or double type columns should have metrics being tracked by Iceberg Orc writers");
- min = fieldMetrics.lowerBound();
+ if (fieldMetrics != null) {
+ // since Orc includes NaN for upper/lower bounds of floating point columns, and we don't want this behavior,
+ // we have tracked metrics for such columns ourselves and thus do not need to rely on Orc's column statistics.
+ min = fieldMetrics.lowerBound();
+ } else {
+ // imported files will not have metrics that were tracked by Iceberg, so fall back to the file's metrics.
+ min = ((DoubleColumnStatistics) columnStats).getMinimum();
+ if (type.typeId() == Type.TypeID.FLOAT) {
Review comment:
We need to do this check for the `else` case as well, replacing `NaN` with the Double infinity.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#issuecomment-952352991
This looks good to me. To unblock the 0.12.1 release, we should get this in and fix the minor issue with the tests later.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#discussion_r733178415
##########
File path: spark/v3.0/spark3-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java
##########
@@ -735,4 +782,35 @@ private void createPartitionedHiveTable() {
partitionedDF.write().insertInto(sourceTableName);
partitionedDF.write().insertInto(sourceTableName);
}
+
+ public List<Record> createOrcFile(File orcFile, int numRows) throws IOException {
+ // Needs to be deleted but depend on the rule to delete the file for us again at the end.
+ if (orcFile.exists()) {
+ orcFile.delete();
+ }
+ final org.apache.iceberg.Schema icebergSchema = new org.apache.iceberg.Schema(
Review comment:
Unfortunately there's no ability to write to ORC with a dataframe from this test area. I had wanted to do that as well, but it doesn't seem to be possible (even after the changes that I added).
I agree about not writing with the IDs already. Let me see if there's another way to write out the file. I was trying a few ways but now that I added the imports, those other ways might work. 👍
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3332: ORC: Fix importing ORC files with float and double columns and test
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3332:
URL: https://github.com/apache/iceberg/pull/3332#issuecomment-954979898
Thanks, @kbendick!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org