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

Bigquery support #106

Merged
merged 8 commits into from Dec 11, 2018

Conversation

Projects
None yet
3 participants
@snowhork
Copy link
Collaborator

snowhork commented Oct 17, 2018

I want to support Bigquery as DataSource.

I introduce DataSourceAdapters::Base class, and adapters of existing data_source(pg, mysql2, redshift) inherit this base class.

I think when someone introduces new data_source, new data_source adapter should inherit this base class.

please review, @hogelog

@snowhork snowhork force-pushed the reproio:bigquery-support branch from 2e15025 to 4efdacd Oct 17, 2018

@snowhork

This comment has been minimized.

Copy link
Collaborator

snowhork commented Oct 17, 2018

This is screenshot (project_id: bigquery-public-data dataset: bitcoin_blockchain)
cf. https://bigquery.cloud.google.com/dataset/bigquery-public-data:bitcoin_blockchain

image

This is blocks table.
cf. https://bigquery.cloud.google.com/table/bigquery-public-data:bitcoin_blockchain.blocks

image

@snowhork snowhork force-pushed the reproio:bigquery-support branch from 4efdacd to 0d245be Oct 17, 2018

@snowhork

This comment has been minimized.

Copy link
Collaborator

snowhork commented Oct 17, 2018

Hakiri warns about sql injection, but this code doesn't seem different from origin code..


has_many :ignored_tables

has_one :database_memo, class_name: "DatabaseMemo", foreign_key: :name, primary_key: :name, dependent: :destroy
has_one :bigquery_config, dependent: :destroy
accepts_nested_attributes_for :bigquery_config, update_only: true

This comment has been minimized.

@hogelog

hogelog Nov 3, 2018

Owner

Sorry, I don't like accepts_nested_attributes_for.
I don't want to merge this change...

This comment has been minimized.

@snowhork

snowhork Nov 6, 2018

Collaborator

ok. I'll remove it.

class BigqueryConfig < ApplicationRecord
validates :project_id, :dataset, presence: true

self.primary_key = :data_source_id

This comment has been minimized.

@hogelog

hogelog Nov 3, 2018

Owner

Why do you want to change primary key?
I think it is safe to leave the primary key as default.

This comment has been minimized.

@snowhork

snowhork Nov 6, 2018

Collaborator

bigquery_configs.data_source_id is unique and not null. I think it can be used as primary key.
You don't prefer? If not, I will fix.

@hogelog

This comment has been minimized.

Copy link
Owner

hogelog commented Nov 4, 2018

After #110, many codes are conflicted.
Resolve conflicts please 🙏

Now, I noticed Hakiri CI is failed.
Tweaking config/brakeman.ignore could suppress this warning.

@snowhork snowhork force-pushed the reproio:bigquery-support branch from 0d245be to 73f988e Nov 7, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 7, 2018

Coverage Status

Coverage decreased (-1.8%) to 95.165% when pulling c927abb on reproio:bigquery-support into bf1904e on hogelog:master.

@snowhork snowhork force-pushed the reproio:bigquery-support branch from c9c2133 to 03bd75b Nov 7, 2018

@snowhork

This comment has been minimized.

Copy link
Collaborator

snowhork commented Nov 7, 2018

@hogelog I have resolved conflicts and fixed your points.
Coverage decreased, but it detected meaningless code, for example, raise NotImplementedError etc. Is it OK?

@hogelog
Copy link
Owner

hogelog left a comment

This PR looks good for me!

But I commented on some concerned points. @snowhork

@@ -0,0 +1,19 @@
module DataSourceAdapters
class << self

This comment has been minimized.

@hogelog

hogelog Dec 9, 2018

Owner

I'm using "def self.adapters; ..." style (not "class << self; def ..." style) in this project, so I want you to arrange like that.

This comment has been minimized.

@snowhork

snowhork Dec 11, 2018

Collaborator

I've fixed. 7d1364c


require 'data_source_adapters/postgresql_adapter'
require 'data_source_adapters/mysql2_adapter'
require 'data_source_adapters/redshift_adapter'

This comment has been minimized.

@hogelog

hogelog Dec 9, 2018

Owner

Is this config file required?
These files are located under app/models, so it seems to be autoloaded without this config.

This comment has been minimized.

@hogelog

hogelog Dec 9, 2018

Owner

How about this change? reproio@ac92651

This comment has been minimized.

@snowhork

snowhork Dec 11, 2018

Collaborator

OK. It's more preferable.

@hogelog
Copy link
Owner

hogelog left a comment

Great work 🎉

@hogelog hogelog merged commit 5456f28 into hogelog:master Dec 11, 2018

1 of 3 checks passed

Hakiri Security warnings were found.
Details
coverage/coveralls Coverage decreased (-1.8%) to 95.165%
Details
semaphoreci The build passed on Semaphore.
Details

@abicky abicky deleted the reproio:bigquery-support branch Dec 19, 2018

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