Skip to content

Conversation

dspeck1
Copy link

@dspeck1 dspeck1 commented Sep 5, 2024

Add tap support for BigQuery. Includes the geometric conversions from TAP to BigQuery geometric functions. The TAP server example for running this will be provided in a different repository.

@dspeck1 dspeck1 changed the title BigQuery TAP support. BigQuery TAP support Sep 5, 2024
@pdowler
Copy link
Member

pdowler commented Sep 5, 2024

Can you also add this to the .github/workflows/gradle.yml?

Below cadc-tap-server-oracle (which I just fixed so you will need to pull main) and in that same style.

We haven't got to it yet in tap.git (technical debt), but in many repos we also run the checkstyleMain target in CI so if checkstyle passes for your code you can add that and the PR looks even better. Something like this:

    - name: build and test cadc-tap-server-bigquery
      run: cd cadc-tap-server-bigquery && ../gradlew --info clean build javadoc checkstyleMain install

Copy link
Member

@pdowler pdowler left a comment

Choose a reason for hiding this comment

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

A few comments on the build

@dspeck1
Copy link
Author

dspeck1 commented Feb 6, 2025

Hi @pdowler - is there anything else needed on this PR? I believe the build steps were addressed.

@pdowler
Copy link
Member

pdowler commented Feb 6, 2025 via email

@@ -0,0 +1,3 @@
# cadc-tap-server-bigquery 1.0.0

Offers BigQuery support to handle datatype conversions in ADQL. No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some details here to help a developer that wants to build a service using the library and a BigQuery backend?

What JDBC driver do they need to install and configure? (preferrably: class name and maven dependency info to add as a runtime dependency)

Do they put the tap_schema tables and content into BigQuery or a side db? (cadc-tap-server does not currently support them being in separate DBs but we are working on that)

Is it expected to support the optional TAP UPLOAD feature? without a custom ca.nrc.cadc.tap.db.DatabaseDataType it seems like that won't work (at least for all data types)

It looks like this is the ADQL -> {big query dialect} conversion. Even for that, I would provide a single class that encapsulates all of that.

As an example, the youcat module in this repo is a fully functioning TAP service for PostgreSQL. In the src/main/resources you will see the config file for various plugins needed to make that work and some of those are in youcat, some are in cadc-tap-server-pg (or other libs), and some are the defaults from cadc-tap-server. Admittedly, those might not have the optimal organization as some things in youcat could possibly be moved up to the pg specific lib, but it shows what's needed.

Here, it would be good to document the provided implementation classes (like the implementation of TapQuery interface that encapsulates the ADQL query processing) and which DatabaseDataType implementation must/should be used, and any others as outlined in that example PluginFactory.properties

import ca.nrc.cadc.tap.TapSelectItem;

public class BqFormatFactory extends DefaultFormatFactory {

Copy link
Member

Choose a reason for hiding this comment

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

Without a Format object for these column data types, any query that selects a point, polygon, etc will fail to write output. I think this is incomplete and was intended to be added but not?

@@ -0,0 +1,33 @@
plugins {
id "java"
Copy link
Member

Choose a reason for hiding this comment

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

We have updated builds (../opencadc.gradle) to work with gradle 6-8 so

  • merge master in if you haven't done that recently
  • plugins: java-library, maven-publish, checkstyle
  • do not use the old maven plugin

The included ../opencadc.gradle configures the maven-publish plugin to do the old install task so usage is the same.

Also, below you can update sourceCompatibility to 11... probably need to by 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.

2 participants