Hive: Fix predicate pushdown for Date #2254

pvary · 2021-02-19T17:18:55Z

When Hive query SELECT * from date_test WHERE d_date='1998-02-19' contains a date literal as a predicated Iceberg filter fails with the following exception:

Caused by: org.apache.hive.service.cli.HiveSQLException: java.io.IOException: java.lang.IllegalStateException: Not an instance of java.lang.Integer: 2130-01-20
        at org.apache.hive.service.cli.operation.SQLOperation.getNextRowSet(SQLOperation.java:499)
        at org.apache.hive.service.cli.operation.OperationManager.getOperationNextRowSet(OperationManager.java:307)
        at org.apache.hive.service.cli.session.HiveSessionImpl.fetchResults(HiveSessionImpl.java:878)
        at org.apache.hive.service.cli.CLIService.fetchResults(CLIService.java:559)
        at org.apache.hive.service.cli.CLIService.fetchResults(CLIService.java:551)
        at org.apache.iceberg.mr.hive.TestHiveShell.executeStatement(TestHiveShell.java:143)
        ... 62 more
Caused by: java.io.IOException: java.lang.IllegalStateException: Not an instance of java.lang.Integer: 2130-01-20
        at org.apache.hadoop.hive.ql.exec.FetchOperator.getNextRow(FetchOperator.java:521)
        at org.apache.hadoop.hive.ql.exec.FetchOperator.pushRow(FetchOperator.java:428)
        at org.apache.hadoop.hive.ql.exec.FetchTask.fetch(FetchTask.java:147)
        at org.apache.hadoop.hive.ql.Driver.getResults(Driver.java:2208)
        at org.apache.hive.service.cli.operation.SQLOperation.getNextRowSet(SQLOperation.java:494)
        ... 67 more
Caused by: java.lang.IllegalStateException: Not an instance of java.lang.Integer: 2130-01-20
        at org.apache.iceberg.data.GenericRecord.get(GenericRecord.java:123)
        at org.apache.iceberg.Accessors$PositionAccessor.get(Accessors.java:71)
        at org.apache.iceberg.Accessors$PositionAccessor.get(Accessors.java:58)
        at org.apache.iceberg.expressions.BoundReference.eval(BoundReference.java:39)
        at org.apache.iceberg.expressions.Evaluator$EvalVisitor.eq(Evaluator.java:132)
        at org.apache.iceberg.expressions.Evaluator$EvalVisitor.eq(Evaluator.java:52)
        at org.apache.iceberg.expressions.ExpressionVisitors$BoundVisitor.predicate(ExpressionVisitors.java:249)
        at org.apache.iceberg.expressions.ExpressionVisitors.visitEvaluator(ExpressionVisitors.java:346)
        at org.apache.iceberg.expressions.Evaluator$EvalVisitor.eval(Evaluator.java:57)
        at org.apache.iceberg.expressions.Evaluator$EvalVisitor.access$100(Evaluator.java:52)
        at org.apache.iceberg.expressions.Evaluator.eval(Evaluator.java:49)
        at org.apache.iceberg.mr.mapreduce.IcebergInputFormat$IcebergRecordReader.lambda$applyResidualFiltering$0(IcebergInputFormat.java:288)
        at org.apache.iceberg.io.CloseableIterable$3.shouldKeep(CloseableIterable.java:82)
        at org.apache.iceberg.io.FilterIterator.advance(FilterIterator.java:67)
        at org.apache.iceberg.io.FilterIterator.hasNext(FilterIterator.java:50)
        at org.apache.iceberg.mr.mapreduce.IcebergInputFormat$IcebergRecordReader.nextKeyValue(IcebergInputFormat.java:202)
        at org.apache.iceberg.mr.mapred.MapredIcebergInputFormat$MapredIcebergRecordReader.next(MapredIcebergInputFormat.java:104)
        at org.apache.iceberg.mr.mapred.MapredIcebergInputFormat$MapredIcebergRecordReader.next(MapredIcebergInputFormat.java:81)
        at org.apache.hadoop.hive.ql.exec.FetchOperator.getNextRow(FetchOperator.java:488)
        ... 71 more

There are 2 issue:

Literal conversion is problematic as Hive uses Date with local timezone, so we have to fix the conversion
When comparing the Record with the literal GenericRecord.get() expect the value to be as an Integer, but it gets LocalDate instead.

shardulm94 · 2021-02-19T18:12:33Z

The manifest and residual evaluator expect the following java types for each data type

iceberg/api/src/main/java/org/apache/iceberg/types/Type.java

Line 29 in efc4618

enum TypeID {

I think we will need to do something very similar to wrapping the record into an InternalRecordWrapper like here

iceberg/data/src/main/java/org/apache/iceberg/data/GenericReader.java

Line 86 in efc4618

    
           InternalRecordWrapper wrapper = new InternalRecordWrapper(recordSchema.asStruct());

That should handle conversion between types used by iceberg-data v/s the ones used by the evaluators.

pvary · 2021-02-19T19:23:29Z

I think we will need to do something very similar to wrapping the record into an InternalRecordWrapper like here

iceberg/data/src/main/java/org/apache/iceberg/data/GenericReader.java

Line 86 in efc4618

InternalRecordWrapper wrapper = new InternalRecordWrapper(recordSchema.asStruct());

That works! Thanks for the pointer. Updated the PR

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerLocalScan.java

mr/src/main/java/org/apache/iceberg/mr/mapreduce/IcebergInputFormat.java

…they were correct in the first place Also addressed Marton's comment

pvary · 2021-02-22T17:21:24Z

@shardulm94: Could you please review as a committer, so I can merge?
Thanks,
Peter

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerLocalScan.java

mr/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergFilterFactory.java

…mezones

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerTimezone.java

…ly. Not doing anything helps to highlihgt issues faster if there is a problem with it

mr/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergFilterFactory.java

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerTimezone.java

shardulm94

@pvary LGTM! Thanks for fixing this!

pvary · 2021-02-26T11:51:29Z

Thanks @shardulm94 and @marton-bod for the review!

pvary · 2021-02-26T15:07:17Z

@shardulm94, @marton-bod: Added the fix for timestamp.withZone in #2278

edgarRd · 2021-02-26T22:46:07Z

mr/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergFilterFactory.java

+  // We have to use the LocalDateTime to get the micros. See the comment above.
  private static long microsFromTimestamp(Timestamp timestamp) {
-    return DateTimeUtil.microsFromInstant(timestamp.toInstant());
+    return DateTimeUtil.microsFromTimestamp(timestamp.toLocalDateTime());


@pvary This change breaks test:

./gradlew clean :iceberg-mr:test --tests org.apache.iceberg.mr.hive.TestHiveIcebergFilterFactory ... > Task :iceberg-mr:test FAILED org.apache.iceberg.mr.hive.TestHiveIcebergFilterFactory > testTimestampType FAILED java.lang.AssertionError: expected:<1349154977123456> but was:<1349129777123456> at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.failNotEquals(Assert.java:834) at org.junit.Assert.assertEquals(Assert.java:118) at org.junit.Assert.assertEquals(Assert.java:144) at org.apache.iceberg.mr.hive.TestHiveIcebergFilterFactory.assertPredicatesMatch(TestHiveIcebergFilterFactory.java:268) at org.apache.iceberg.mr.hive.TestHiveIcebergFilterFactory.testTimestampType(TestHiveIcebergFilterFactory.java:248) 16 tests completed, 1 failed

When run in non-UTC environments. I assume the test may need to change to adjust to the changes being made in #2278 to handle predicate pushdown for Timestamp.withZone().

I'm surprised this is not caught by the CI checks, but maybe the CI runs in UTC - is there a way that we can run the tests in a few additional Timezones to validate?

Thanks!

Even more surprised to see that, since I run my tests in CET and they are running without problem. What timezone are you using? Are you using stock Hive?

Thanks,
Peter

I'm in PST, if you replace https://github.com/apache/iceberg/blob/master/mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergFilterFactory.java#L240-L242 for:

TimeZone defaultTz = TimeZone.getDefault(); try { TimeZone.setDefault(TimeZone.getTimeZone("America/Los_Angeles")); UnboundPredicate actual = (UnboundPredicate) HiveIcebergFilterFactory.generateFilterExpression(arg); assertPredicatesMatch(expected, actual); } finally { TimeZone.setDefault(defaultTz); }

to set the TimeZone, you should be able to repro - conversely if I use "UTC" instead of "America/Los_Angeles" the test pass.

I'm running the unit test out of the master branch, with:

./gradlew clean :iceberg-mr:test --tests org.apache.iceberg.mr.hive.TestHiveIcebergFilterFactory

Interestingly, if I use "CET" it also fails:

org.apache.iceberg.mr.hive.TestHiveIcebergFilterFactory > testTimestampType FAILED java.lang.AssertionError: expected:<1349154977123456> but was:<1349162177123456> at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.failNotEquals(Assert.java:834) at org.junit.Assert.assertEquals(Assert.java:118) at org.junit.Assert.assertEquals(Assert.java:144) at org.apache.iceberg.mr.hive.TestHiveIcebergFilterFactory.assertPredicatesMatch(TestHiveIcebergFilterFactory.java:266) at org.apache.iceberg.mr.hive.TestHiveIcebergFilterFactory.testTimestampType(TestHiveIcebergFilterFactory.java:246) 16 tests completed, 1 failed

For CET, you can see the difference between the expected value and the actual value is exactly 2 hrs in microseconds (7.2 10^9) - the actual value is ahead of the expected one (which is in UTC)

Maybe intellij made fun of me, or I messed up with my settings. Will definitely check this out soon.

Thanks!

If you have some time, could you please check that with #2278 the tests run correctly on you side?
In the meantime I try to find out what is the process of reverting changes.

Thanks,
Peter

@edgarRd: Pushed the fix here #2283. Could you please confirm, that this issue is fixed?

Thanks,
Peter

Thanks for the fix @pvary - test works now.

This reverts commit 23735d1.

Hive: Fix predicate pushdown for Date (apache#2254)

github-actions bot added core MR labels Feb 19, 2021

Hive: Fix predicate pushdown for Date

204da73

pvary force-pushed the fixdate branch from a29ca9a to 204da73 Compare February 19, 2021 17:32

Using wrapper for the Records

2e819bb

marton-bod reviewed Feb 22, 2021

View reviewed changes

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerLocalScan.java Outdated Show resolved Hide resolved

marton-bod reviewed Feb 22, 2021

View reviewed changes

mr/src/main/java/org/apache/iceberg/mr/mapreduce/IcebergInputFormat.java Show resolved Hide resolved

pvary force-pushed the fixdate branch from 309c2ed to 71c5d72 Compare February 22, 2021 14:44

Added tests for Timetamps, and reverted timestamp related changes as …

6884bc8

…they were correct in the first place Also addressed Marton's comment

pvary force-pushed the fixdate branch from 71c5d72 to 6884bc8 Compare February 22, 2021 14:48

marton-bod approved these changes Feb 22, 2021

View reviewed changes

shardulm94 reviewed Feb 24, 2021

View reviewed changes

Moved Date tests to it's own test case and run them with different ti…

db05fd6

…mezones

pvary force-pushed the fixdate branch from 4e315b0 to db05fd6 Compare February 24, 2021 22:26

shardulm94 reviewed Feb 25, 2021

View reviewed changes

Peter Vary added 2 commits February 25, 2021 12:07

Fixed Shardul's comments

70e21e1

Do not try to revert the timeZone changes as we can not do it complet…

f33a96c

…ly. Not doing anything helps to highlihgt issues faster if there is a problem with it

shardulm94 reviewed Feb 25, 2021

View reviewed changes

mr/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergFilterFactory.java Outdated Show resolved Hide resolved

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerTimezone.java Outdated Show resolved Hide resolved

Few more comments

861f581

marton-bod reviewed Feb 25, 2021

View reviewed changes

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerTimezone.java Outdated Show resolved Hide resolved

marton-bod reviewed Feb 25, 2021

View reviewed changes

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerTimezone.java Outdated Show resolved Hide resolved

marton-bod reviewed Feb 25, 2021

View reviewed changes

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerTimezone.java Outdated Show resolved Hide resolved

Moved the shell/HS2/HMS creation to BeforeClass

e088a5a

shardulm94 approved these changes Feb 26, 2021

View reviewed changes

pvary merged commit 23735d1 into apache:master Feb 26, 2021

pvary deleted the fixdate branch February 26, 2021 11:51

pvary mentioned this pull request Feb 26, 2021

Hive: Fix predicate pushdown for Timestamp.withZone() #2278

Closed

edgarRd reviewed Feb 26, 2021

View reviewed changes

pvary pushed a commit to pvary/iceberg that referenced this pull request Feb 27, 2021

Revert "Hive: Fix predicate pushdown for Date (apache#2254)"

c7ba753

This reverts commit 23735d1.

bkahloon added a commit to bkahloon/iceberg that referenced this pull request Feb 27, 2021

Merge pull request #1 from apache/master

9bc781c

Hive: Fix predicate pushdown for Date (apache#2254)

pvary mentioned this pull request Feb 28, 2021

Hive: Quick fix for broken TestHiveIcebergFilterFactory.testTimestampType test #2283

Merged

aokolnychyi pushed a commit that referenced this pull request Mar 24, 2021

Hive: Fix predicate pushdown for Date (#2254)

1f02595

aokolnychyi added this to the Java 0.11.1 Release milestone Mar 30, 2021

coolderli pushed a commit to coolderli/iceberg that referenced this pull request Apr 26, 2021

Hive: Fix predicate pushdown for Date (apache#2254)

317d6bb

stevenzwu pushed a commit to stevenzwu/iceberg that referenced this pull request Jul 28, 2021

Hive: Fix predicate pushdown for Date (apache#2254)

b72bdb7

Hive: Fix predicate pushdown for Date #2254

Hive: Fix predicate pushdown for Date #2254

Uh oh!

Conversation

pvary commented Feb 19, 2021

Uh oh!

shardulm94 commented Feb 19, 2021

Uh oh!

pvary commented Feb 19, 2021

Uh oh!

Uh oh!

Uh oh!

pvary commented Feb 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shardulm94 left a comment

Choose a reason for hiding this comment

Uh oh!

pvary commented Feb 26, 2021

Uh oh!

pvary commented Feb 26, 2021

Uh oh!

edgarRd Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

pvary Feb 27, 2021

Choose a reason for hiding this comment

Uh oh!

edgarRd Feb 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edgarRd Feb 27, 2021

Choose a reason for hiding this comment

Uh oh!

pvary Feb 27, 2021

Choose a reason for hiding this comment

Uh oh!

pvary Feb 27, 2021

Choose a reason for hiding this comment

Uh oh!

pvary Feb 28, 2021

Choose a reason for hiding this comment

Uh oh!

edgarRd Mar 1, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

edgarRd Feb 27, 2021 •

edited

Loading