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

Feature/bigquery jdbc wrapper #2492

Closed
wants to merge 13 commits into from

Conversation

Pubby6
Copy link
Contributor

@Pubby6 Pubby6 commented Dec 1, 2023

What type of PR is this?

Improvement

What does this PR do / why is it needed ?

This PR introduces a new JDBC wrapper for BigQuery, aimed at replacing the existing Simba driver. The key motivation for this enhancement is to provide a more efficient and reliable way of interfacing with BigQuery by utilizing the BigQuery client library.

Which issue(s) this PR fixes:

Other notes for reviewers:

Please review the implementation details, focusing on the integration of the BigQuery client library.
Any feedback on the compatibility and performance improvements compared to the Simba driver would be highly appreciated.
The changes have been thoroughly tested, but additional insights or tests from the reviewers would be valuable.

Does this PR introduce a user-facing change?

No

Copy link

linux-foundation-easycla bot commented Dec 1, 2023

CLA Not Signed

@Pubby6 Pubby6 marked this pull request as ready for review December 1, 2023 21:45
@Pubby6 Pubby6 requested a review from a team as a code owner December 1, 2023 21:45
@@ -0,0 +1,236 @@
<?xml version="1.0"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Update copyright header to mention Google.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually .. Don't think we need this checstyle.xml here. The root checkstyle.xml should suffice

@@ -0,0 +1,99 @@
<project xmlns="http://maven.apache.org/POM/4.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Update copyright header to include Google + Apache License.

Please make this change for all source files

* Copyright 2020 Google LLC
*/

package com.deloitte.legend.engine.bigquery;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be com.google ? Also, this class is technically not a part of the "engine".

Can we rename this to com.google.bigquery.jdbc

@@ -0,0 +1,229 @@
Legend SQL Compatibility Report
Copy link
Contributor

Choose a reason for hiding this comment

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

This test summary md file will become stale as the codebase evolves.

Please remove from the PR. Might be good to attach to the PR as a doc attachment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as the above summary md file. Please remove the junit test result xml.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as the above summary md file. Please remove the junit test result xml.

import java.util.Properties;
import java.util.logging.Logger;

public class BigQueryDriver implements Driver
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we a simple Junit test that makes a connection and asserts that the connection is valid ?

We can provide the credentials and BigQuery endpoint coordinates to connect.

Copy link

github-actions bot commented Dec 2, 2023

Test Results

0 files   -      708  0 suites   - 708   0s ⏱️ - 1h 3m 13s
0 tests  - 11 845  0 ✔️  - 11 423  0 💤  - 422  0 ±0 
0 runs   - 14 840  0 ✔️  - 14 319  0 💤  - 521  0 ±0 

Results for commit 9ff98b8. ± Comparison against base commit f7c44ec.

@Pubby6 Pubby6 requested a review from epsstan December 19, 2023 14:06
@Pubby6 Pubby6 closed this Dec 19, 2023
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.

2 participants