Skip to content

adding avro classes#52

Merged
aweisberg merged 3 commits intoprestodb:masterfrom
ugurmeet:origin/avro
Nov 1, 2021
Merged

adding avro classes#52
aweisberg merged 3 commits intoprestodb:masterfrom
ugurmeet:origin/avro

Conversation

@ugurmeet
Copy link
Contributor

Adding avro classes following discussion in #51

@shangxinli
Copy link
Contributor

Do you make any changes to those files? Knowing that would help review.

@ugurmeet
Copy link
Contributor Author

Hi @shangxinli, thanks for looking. So basically I have applied this patch over these files from Hive 3.0 branch. The TypeInfoToSchema.java needed this fix from trino.

@shangxinli
Copy link
Contributor

shangxinli commented Oct 19, 2021

If no change from the Hive branch and your local Presto building work and unit test passing(Particularly those failing tests in the pr, then it should be fine. + @beinan

Copy link
Member

@beinan beinan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me except the last two files need a "new line" at the end. Many thanks! do we still need make a snapshot release to test?

@shangxinli
Copy link
Contributor

I think that would be a better option if we can do it. Thanks @beinan!

ugurmeet and others added 2 commits October 19, 2021 09:14
…nfo.java

Co-authored-by: Beinan <beinan@users.noreply.github.com>
…ema.java

Co-authored-by: Beinan <beinan@users.noreply.github.com>
@beinan
Copy link
Member

beinan commented Oct 19, 2021

I think that would be a better option if we can do it. Thanks @beinan!

We need help from FB team. @aweisberg could you help make a snapshot release for this PR? It's still for the parquet upgrading. Hope this time we could pass all the test. Thanks a lot!

@aweisberg
Copy link
Contributor

I deployed a 3.0.0-7 snapshot based on this branch. I was able to built Presto using it. Please bug me early and often if you need me to do maven things as I sometimes miss the GH emails :-) Even daily is fine.

@ugurmeet
Copy link
Contributor Author

Thanks @beinan , @shangxinli . I have create a pr to test this version with the presto head. I was able to fix build issue and a few tests but some presto-hive test are still failing.
https://github.com/prestodb/presto/pull/16892/checks?check_run_id=3945173738
One failure seems to be in parquetwriter area. @shangxinli , given your parquet expertise, can you look into the test failures.

@shangxinli
Copy link
Contributor

@aweisberg Thanks a lot for help!

@ugurmeet, I see there are two failures in that PR. One of them is from iceberg module and I called out @beinan. The other is in Hive and the failure message is as below. @beinan I don't know who has the expertise on that one. Idea?

Error: Failures:
Error: com.facebook.presto.hive.TestHiveRecoverableExecution.testInsertBucketedTable(com.facebook.presto.hive.TestHiveRecoverableExecution)
Error: Run 1: TestHiveRecoverableExecution.testInsertBucketedTable:191->testRecoverableGroupedExecution:390 » Execution

@beinan
Copy link
Member

beinan commented Oct 25, 2021

@aweisberg Thanks a lot for help!

@ugurmeet, I see there are two failures in that PR. One of them is from iceberg module and I called out @beinan. The other is in Hive and the failure message is as below. @beinan I don't know who has the expertise on that one. Idea?

Error: Failures: Error: com.facebook.presto.hive.TestHiveRecoverableExecution.testInsertBucketedTable(com.facebook.presto.hive.TestHiveRecoverableExecution) Error: Run 1: TestHiveRecoverableExecution.testInsertBucketedTable:191->testRecoverableGroupedExecution:390 » Execution

@shangxinli any further details of this test? are you able to reproduce this one locally?

@zhenxiao could you help? this one blocks both parquet and iceberg upgrading. Thanks!

@ugurmeet
Copy link
Contributor Author

Hi @aweisberg, from the test failures in prestodb/presto#16892 it seems like the 3.0.0-7-SNAPSHOT.jar no longer has my changes in it. The tests were working mostly fine last week with only couple of failures. Today a bunch of them are failing. Any ideas. Thanks

@ugurmeet
Copy link
Contributor Author

@beinan , @aweisberg , should we commit this change and cut a release to make it easy to test. The test failures in prestodb/presto#16892 seem to be increasing likely due to the snapshot jar losing my changes. We can always revert if things don't seem to work out.

@beinan
Copy link
Member

beinan commented Oct 28, 2021

@beinan , @aweisberg , should we commit this change and cut a release to make it easy to test. The test failures in prestodb/presto#16892 seem to be increasing likely due to the snapshot jar losing my changes. We can always revert if things don't seem to work out.

@ugurmeet are you asking to merge this commit and cut a release of 3.0.0-7? I'm ok and since there is no other changes ongoing on this project. @aweisberg what do you think?

@ugurmeet
Copy link
Contributor Author

@beinan , yes that is exactly what I am asking.

@aweisberg
Copy link
Contributor

I can see that the snapshot artifacts were definitely overwritten by something else called facebook-snapshots. I don't know what that is.

What I can do is upload a snapshot version 3.0.0-7-aweisberg-SNAPSHOT which I really doubt anything else will overwrite!

@ugurmeet
Copy link
Contributor Author

Created prestodb/presto#16923 based on the new jar

@ugurmeet
Copy link
Contributor Author

ugurmeet commented Nov 1, 2021

@beinan , @shangxinli all tests are passing now in prestodb/presto#16923. Should we move this along.

@aweisberg aweisberg merged commit 7313a99 into prestodb:master Nov 1, 2021
@aweisberg
Copy link
Contributor

3.0.0-7 should be available in Maven central now.

@shangxinli
Copy link
Contributor

@ugurmeet Thank you so much for working on this! It unblocked the parquet upgrading for Presto for several important projects.

@aweisberg @beinan Thanks for help!

@ugurmeet
Copy link
Contributor Author

ugurmeet commented Nov 1, 2021

Thanks @shangxinli for your comments. @aweisberg, thanks a bunch for merging the changes. Should we cut the 3.0.0-7 release for the jar to be available for prestodb/presto#16923. Let me know if I can help in any way.

@aweisberg
Copy link
Contributor

aweisberg commented Nov 1, 2021

The jar should already be available. I forgot to push the release commits and closed but didn't click release in Nexus. Fixed and it should be available now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants