Upgrade to Hive 3.0.0 by imjalpreet · Pull Request #15659 · prestodb/presto

imjalpreet · 2021-02-01T11:03:39Z

This PR includes the base changes required for upgrading to Hive 3. This will help in supporting and bringing a number of interesting features in Hive 3 to Presto in the future.

This depends on https://github.com/prestodb/presto-hive-apache/releases/tag/3.0.0-2 and the following PR in tempto (prestodb/tempto#272)

depends on https://github.com/facebookexternal/presto-facebook/pull/1414

aweisberg · 2021-02-08T16:52:18Z

If there are test failures introduced by a commit it's better to have the fixes in the commit and not separate. Otherwise there are failing commits on master that interfere with bisecting.

This seems to have introduced another dependency related failure. It's caught in the FB integration builds.

[WARNING] Rule 0: org.apache.maven.plugins.enforcer.RequireUpperBoundDeps failed with message:
Failed while enforcing RequireUpperBoundDeps. The error(s) are [
Require upper bound dependencies error for com.facebook.presto.hive:hive-apache:1.2.2-2 paths to dependency are:
+-com.facebook.presto:presto-namespace:0.248-SNAPSHOT
  +-com.facebook.presto:presto-hive-metastore:0.248-SNAPSHOT
    +-com.facebook.presto.hive:hive-apache:1.2.2-2 (managed) <-- com.facebook.presto.hive:hive-apache:3.0.0-2
, 
Require upper bound dependencies error for org.apache.thrift:libthrift:0.9.1 paths to dependency are:
+-com.facebook.presto:presto-namespace:0.248-SNAPSHOT
  +-com.facebook.presto:presto-hive-metastore:0.248-SNAPSHOT
    +-org.apache.thrift:libthrift:0.9.1 (managed) <-- org.apache.thrift:libthrift:0.9.3
]

aweisberg · 2021-02-08T17:08:26Z

I'll try and get Tempto through since it is one of the dependencies.

imjalpreet · 2021-02-09T08:19:00Z

@aweisberg Yes once the tempto changes are through I will update this PR(squash commits as well) and make sure all the test cases pass before the code is reviewed.

Regarding the Facebook Integration build failure, it looks like there are some additional plugins in FB like presto-namespace which will also have to be modified for Hive 3 Upgrade. Currently, this additional plugin is using some dependencies which have been upgraded in this PR due to which we are seeing the errors. I am not sure if this plugin (presto-namespace) is open-sourced.

aweisberg · 2021-02-09T15:08:22Z

Thanks I missed that. I thought presto-namespace was one of the OSS repos. Still trying to figure out how to release Tempto.

aweisberg · 2021-02-09T16:27:06Z

So the issue is that we can't release Tempto because it's under io.prestodb which isn't controlled by the Linux Foundation.

We either need to find out who has the credentials for updating io.prestodb and get them to provide access or we need to refactor Tempto so we can upload it to com.facebook.presto.

I can ask around about updating io.prestodb but it could take a while, and I can't guarantee it will happen. I broached the subject of refactoring it under com.facebook.presto and so far no one has objected.

If you want to do the refactor of Tempto it would unblock this PR otherwise we have to wait on trying to get the credentials back. Sorry we are getting blocked on something so basic.

imjalpreet · 2021-02-09T19:19:43Z

@aweisberg Thanks for looking into this. I can take up the refactoring task and raise another PR once it's ready.

aweisberg · 2021-02-09T21:19:23Z

Great, so the package we want is com.facebook.prestodb.tempto and find ./ -type f -exec sed -i '' -e 's/io.prestodb/com.facebook.prestodb/g' {} \; can replace anything that refactoring in IntelliJ doesn't manage to do.

aweisberg · 2021-02-10T19:47:14Z

Asking Sonatype if we can get access to io.prestodb.tempto

aweisberg · 2021-02-11T22:31:46Z

I was able to upload the snapshot artifacts. Can you rebase your PR and update it to use the 1.51-SNAPSHOT artifact and we will see if the tests complete?

imjalpreet · 2021-02-12T15:28:32Z

@aweisberg I had made the respective changes. The product tests have passed but the tests for presto-main module seem to have failed. It doesn't look like that is because of the code changes in this PR, looks like it was because of some crash while the tests were running. Maybe if you can re-trigger the module, it would pass successfully.

aweisberg

Only two minor change requests. The next step is for me to deploy this on a cluster and shadow some traffic to sanity check. I don't want this to get reverted during the release process.

aweisberg · 2021-02-12T16:56:07Z

presto-orc/src/test/java/com/facebook/presto/orc/OrcTester.java

Maybe keep the refactor from the Trino PR? trinodb/trino@238f4b6#diff-23fd4cecda2fe420dc9a4fea750e785439046225ed9f013dee3bc123f1c9c2e5R1202

Just to keep the two code bases more consistent.

@aweisberg I have made the refactor consistent with Trino as you requested.

aweisberg · 2021-02-12T16:56:23Z

presto-orc/src/main/java/com/facebook/presto/orc/metadata/statistics/BloomFilter.java

Is there a reason we don't take the latest version of BloomFilter from Trino?

@aweisberg The latest version was not taken with the idea of making the changes gradually to understand the impact. The class being used in trino is forked from org.apache.orc.util.BloomFilter and we had been referring to org.apache.hive.common.util.BloomFilter.

Let me check the impact and the changes required and get back.

@aweisberg I reviewed the BloomFilter Class being used in Trino. As I mentioned earlier, one of the major change is they have forked from org.apache.orc.util.BloomFilter and post that they have made some optimisations on top of that. A couple of enhancements and optimisations include changes in ORC Bloom filter hash, added support for ORC UTF8 bloom filters, real type in ORC bloom filter and a few more.

Would it be better to have separate PRs for these enhancements?

I would have been fine with that as well :-) Thanks for doing it now. I am still trying to figure out why I can't publish Tempto. It really does seem to be uploading without error.

aweisberg · 2021-02-17T22:48:13Z

It's released! At long last our national nightmare is over.

Bad news, there are some shading issues I think. FB integration is failing on duplicate-finder-maven-plugin which I think is finding legitimate issues even outside the FB integration. We should probably enable the duplicate finder on Presto Hive https://pastebin.com/AHxAnJNF

Still looking into this to make sure we aren't pulling in org.apache.parquet directly by mistake.

imjalpreet · 2021-02-18T13:47:47Z

It's released! At long last our national nightmare is over.

That's great news.

Bad news, there are some shading issues I think. FB integration is failing on duplicate-finder-maven-plugin which I think is finding legitimate issues even outside the FB integration. We should probably enable the duplicate finder on Presto Hive https://pastebin.com/AHxAnJNF

Sure I will also try to have a look into the same.

aweisberg

LGTM. I verified this on a cluster. Turns out the issue was that the Tempto 1.51-SNAPSHOT was probably just unmodified 1.50 since I was failing it update the artifacts. Switching to the 1.51 release artifact made the dependency issues go away.

rschlussel

Just 2 questions, otherwise looks good.

rschlussel · 2021-02-25T15:17:41Z

...e-metastore/src/main/java/com/facebook/presto/hive/metastore/thrift/ThriftMetastoreUtil.java

What's the reason for the changes for decimal?

Constructor argument order is reversed in Hive 3. Trino also just switches the parameters around.

rschlussel · 2021-02-25T15:18:09Z

presto-raptor/src/test/java/com/facebook/presto/raptor/storage/TestShardWriter.java

why was this test removed?

Short answer, because that is what Trino did trinodb/trino@238f4b6#diff-82a51d3cf239ee341a4eab63c8c88c82367b389a9652cbaa067631d131a60930

Long answer... I don't know. I'll bring it back and see what happens when I run it.

The test still passes. So I guess we should keep it!

aweisberg

@imjalpreet can you bring back TestShardWriter.testWriterZeroRows? I am not sure why it was removed in Trino, but I brought it back and the test still passes.

aweisberg · 2021-02-25T15:45:40Z

presto-raptor/src/test/java/com/facebook/presto/raptor/storage/TestShardWriter.java

The test still passes. So I guess we should keep it!

Fix TestOrcBatchPageSourceMemoryTracking unit test failures Fork BloomFilter class from org.apache.hive.common.util.BloomFilter Fix presto-orc plugin unit test failures Co-authored-by: David Phillips <david@acz.org>

imjalpreet · 2021-02-25T22:16:19Z

@aweisberg I have reverted that test removal as requested. I will try to understand why was it removed in Trino.

aweisberg

Thank you @imjalpreet

rschlussel · 2021-03-01T15:53:30Z

Ignoring travis failure since we're migrating from travis. Thanks for the contribution!

aweisberg · 2021-03-08T20:05:15Z

I was mistaken the joda upgrade was in #15649, hah I should know!

imjalpreet · 2021-03-09T05:45:56Z

I was mistaken the joda upgrade was in #15649, hah I should know!

Ah okay, no issues!

aweisberg · 2021-03-10T19:02:54Z

This did end up failing on at least one different issue. CONTAINS on an array was returning false and never true when it should and the # of input rows counted by the aggregation for that query also didn't match 0.247.

I am trying to factor out a reproducer now.

aweisberg · 2021-03-10T22:01:40Z

Narrowing it down some, the issue with Hive 3 is that arrays of varchar are coming back in queries as an array containing a single varchar which is a , separated list of the array contents. I haven't factored that out into a reproducer yet.

You can tell from the CLI because instead of seeing a space after the , between each entry it's just one long run on.

aweisberg · 2021-03-10T22:29:37Z

Still can't reproduce, but I noticed that the format of the table that creates this issue is TEXTFILE. I can create my own TEXTFILE format table with an array it in, and select it out, but I don't see that the values in the array being merged :-(

aweisberg · 2021-03-10T22:53:11Z

LOL https://forums.aws.amazon.com/thread.jspa?threadID=264065

aweisberg · 2021-03-10T23:04:23Z

https://github.com/trinodb/trino/pull/1733/files
https://github.com/trinodb/trino/pull/1321/files

Release cut is Friday

imjalpreet · 2021-03-11T05:38:27Z

@aweisberg thanks for the detailed investigation. A bug due to correcting a misspelled property that's a first. I will try to get in the fix before the release cut most probably by today evening.

highker · 2021-03-11T06:33:33Z

Thanks @imjalpreet putting this up. While working on the fix, please take a look at #15589. It depends which PR merges first. We will need to add materialized view support for Hive 3.0

imjalpreet · 2021-03-11T13:55:23Z

@highker Sure, I will have a look at the materialized view PR.

aweisberg · 2021-03-11T14:16:38Z

@highker I am not 100% following. Do we need to deliver both at the same time? If we only release the Hive 3 upgrade is that ok?

highker · 2021-03-11T17:20:38Z

@aweisberg, @imjalpreet, for example, we need to model materialized views as managed tables today (#15589 (comment)). This can be changed after Hive 3.0

imjalpreet mentioned this pull request Feb 8, 2021

Upgrade Hive dependency to 3.0.0-2 prestodb/tempto#272

Merged

imjalpreet marked this pull request as draft February 8, 2021 09:29

aweisberg self-requested a review February 8, 2021 16:52

imjalpreet force-pushed the HiveUpgrade branch 2 times, most recently from 57acf75 to ce9dfbc Compare February 12, 2021 13:56

imjalpreet marked this pull request as ready for review February 12, 2021 16:17

aweisberg suggested changes Feb 12, 2021

View reviewed changes

imjalpreet force-pushed the HiveUpgrade branch from ce9dfbc to 115ac54 Compare February 16, 2021 13:34

imjalpreet force-pushed the HiveUpgrade branch 2 times, most recently from 7f64dcc to 8456314 Compare February 25, 2021 14:01

aweisberg approved these changes Feb 25, 2021

View reviewed changes

aweisberg requested a review from a team February 25, 2021 15:06

rschlussel reviewed Feb 25, 2021

View reviewed changes

aweisberg suggested changes Feb 25, 2021

View reviewed changes

Update to Hive 3.0.0

64fb3db

Fix TestOrcBatchPageSourceMemoryTracking unit test failures Fork BloomFilter class from org.apache.hive.common.util.BloomFilter Fix presto-orc plugin unit test failures Co-authored-by: David Phillips <david@acz.org>

imjalpreet force-pushed the HiveUpgrade branch from 8456314 to 64fb3db Compare February 25, 2021 22:11

rschlussel approved these changes Feb 26, 2021

View reviewed changes

aweisberg approved these changes Feb 26, 2021

View reviewed changes

rschlussel merged commit fd32fea into prestodb:master Mar 1, 2021

arhimondr mentioned this pull request Mar 12, 2021

Add release notes for 0.249 #15831

Merged

13 tasks

ajaygeorge mentioned this pull request Mar 15, 2021

[TEST] Add release notes for 0.249 #15835

Closed

9 tasks

Conversation

imjalpreet commented Feb 1, 2021 • edited by aweisberg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aweisberg commented Feb 8, 2021

Uh oh!

aweisberg commented Feb 8, 2021

Uh oh!

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

Uh oh!

Uh oh!

aweisberg commented Feb 9, 2021

Uh oh!

aweisberg commented Feb 9, 2021

Uh oh!

imjalpreet commented Feb 9, 2021

Uh oh!

aweisberg commented Feb 9, 2021

Uh oh!

aweisberg commented Feb 10, 2021

Uh oh!

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

Uh oh!

Uh oh!

imjalpreet commented Feb 12, 2021

Uh oh!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

imjalpreet commented Feb 18, 2021

Uh oh!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

rschlussel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

rschlussel commented Mar 1, 2021

Uh oh!

aweisberg commented Mar 8, 2021

Uh oh!

imjalpreet commented Mar 9, 2021

Uh oh!

aweisberg commented Mar 10, 2021

imjalpreet commented Feb 1, 2021 •

edited by aweisberg

Loading

imjalpreet commented Feb 9, 2021 •

edited

Loading

aweisberg commented Feb 11, 2021 •

edited

Loading

aweisberg commented Feb 17, 2021 •

edited

Loading

imjalpreet commented Feb 25, 2021 •

edited

Loading

aweisberg commented Mar 10, 2021 •

edited

Loading