Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for select query failure in presto-bigquery #23957

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

nishithakbhaskaran
Copy link
Contributor

@nishithakbhaskaran nishithakbhaskaran commented Nov 6, 2024

Description

Fix for select query failure in presto-bigquery

Motivation and Context

#23951

Impact

Test Plan

Tested the fix and the select query works fine.
image

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==
BigQuery Connector Changes
* Fix for BigQuery ``SELECT``. :pr:`23957`

Copy link

linux-foundation-easycla bot commented Nov 6, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: nishithakbhaskaran (dca4893)

@nishithakbhaskaran nishithakbhaskaran force-pushed the bigquery-fix branch 2 times, most recently from 1ba3170 to 1341202 Compare November 6, 2024 08:02
@nishithakbhaskaran nishithakbhaskaran marked this pull request as ready for review November 6, 2024 09:15
@nishithakbhaskaran nishithakbhaskaran requested a review from a team as a code owner November 6, 2024 09:15
Copy link
Member

@imjalpreet imjalpreet left a comment

Choose a reason for hiding this comment

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

For context, can you please share the dependency tree before and after this change?

Also, to confirm, the only class that was causing the issue was CallCredentials2?

@tdcmeehan tdcmeehan self-assigned this Nov 6, 2024
@nishithakbhaskaran
Copy link
Contributor Author

@imjalpreet Please see the dependency tree before and after the fix
dependencies-after.txt
dependencies-before.txt

Presto <grpc.version>1.64.0</grpc.version> was updated. The issue was java.lang.NoClassDefFoundError: io/grpc/CallCredentials2 as CallCredentials2.java was removed from the grpc-api:1.64.0 version.

In presto-bigquery below grpc dependencies are of an older version.So we tried updating them as well to 1.64.0
io.grpc:grpc-auth:jar:1.64.0:compile
io.grpc:grpc-alts:jar:1.64.0:compile
io.grpc:grpc-grpclb:jar:1.64.0:compile

@steveburnett
Copy link
Contributor

Suggest revising the release note entry as follows:

== RELEASE NOTES ==
BigQuery Connector Changes
* Fix for BigQuery ``SELECT``. :pr:`23957`

@nishithakbhaskaran
Copy link
Contributor Author

@steveburnett Updated the release note entry as suggested.

@imjalpreet
Copy link
Member

Presto <grpc.version>1.64.0</grpc.version> was updated. The issue was java.lang.NoClassDefFoundError: io/grpc/CallCredentials2 as CallCredentials2.java was removed from the grpc-api:1.64.0 version.

Yes, it was removed after ~ 1.41.x.

In presto-bigquery below grpc dependencies are of an older version.So we tried updating them as well to 1.64.0

Yes thanks, just wanted to confirm the same from the dependency tree.

@@ -358,6 +376,9 @@
<exclude>com.fasterxml.jackson.core:jackson-core</exclude>
<exclude>javax.annotation:javax.annotation-api</exclude>
<exclude>com.fasterxml.jackson.core:jackson-databind</exclude>
<exclude>com.google.auth:google-auth-library-oauth2-http</exclude>
<exclude>com.google.auth:google-auth-library-credentials</exclude>
<exclude>org.conscrypt:conscrypt-openjdk-uber</exclude>
Copy link
Member

Choose a reason for hiding this comment

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

Is this a new transitive dependency due to the version upgrade? Is it also included in Presto as part of any other dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@imjalpreet Added those exclusion to resolve Require upper bound dependencies error in bigquery pom.xml . Those were included in Presto as part of other dependency.

Copy link
Member

Choose a reason for hiding this comment

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

Can you please share the error we saw when we didn't exclude it? I am curious since I can see the same dependency in both the dependency trees.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

logs.docx

@imjalpreet Please see the above log file.

Copy link
Member

Choose a reason for hiding this comment

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

@nishithakbhaskaran Thank you for sharing the logs. Can we assess how complex it would be to fix these errors instead of simply excluding them from the check?

Let's try upgrading all three components to their latest required versions based on the error log and see if any new issues arise.

Upgrade com.google.auth:google-auth-library-oauth2-http to 1.22.0

Add com.google.auth:google-auth-library-credentials to dependencyManagement with version 1.22.0

Add org.conscrypt:conscrypt-openjdk-uber to dependencyManagement with version 2.5.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@imjalpreet I have updated the same as suggested, but after that it shows lot more dependency with Require upper bound dependencies error in the bigquery pom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @imjalpreet @tdcmeehan Addressed the comments. Along with that I need to handle some other upperbound dependency failures in the dependencymanagement tag.

@nishithakbhaskaran nishithakbhaskaran force-pushed the bigquery-fix branch 2 times, most recently from 3f48dea to b91cd5b Compare December 11, 2024 13:23
@tdcmeehan tdcmeehan added the from:IBM PR from IBM label Dec 11, 2024
@prestodb-ci prestodb-ci requested review from a team and jp-sivaprasad and removed request for a team December 11, 2024 13:42
@nishithakbhaskaran nishithakbhaskaran force-pushed the bigquery-fix branch 4 times, most recently from fd45aac to a73cbd9 Compare December 12, 2024 10:14
Copy link
Member

@imjalpreet imjalpreet left a comment

Choose a reason for hiding this comment

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

@nishithakbhaskaran Thanks for the changes! LGTM now.

I just want to confirm if we tested the SELECT query with BigQuery after making these final changes.

@nishithakbhaskaran
Copy link
Contributor Author

@imjalpreet Sure, I have verified the select query after the changes and it is working as expected.

presto> SELECT * FROM bigquery.team_lakehouse_engine_1.emp;
 id |  name  
----+--------
  1 | lukman 
(1 row)

@tdcmeehan tdcmeehan merged commit f9bd6e3 into prestodb:master Jan 8, 2025
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:IBM PR from IBM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants