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

Add clickhouse connector #969

Merged
merged 12 commits into from
Jan 17, 2024
Merged

Add clickhouse connector #969

merged 12 commits into from
Jan 17, 2024

Conversation

pankaj-peerdb
Copy link
Contributor

No description provided.

@pankaj-peerdb pankaj-peerdb changed the title add clickhouse connector Draft: add clickhouse connector Jan 3, 2024
@serprex serprex marked this pull request as draft January 3, 2024 19:06
@pankaj-peerdb pankaj-peerdb changed the title Draft: add clickhouse connector Add clickhouse connector Jan 8, 2024
@pankaj-peerdb pankaj-peerdb changed the base branch from clickhouse-add-protos to main January 8, 2024 08:05
@pankaj-peerdb pankaj-peerdb marked this pull request as ready for review January 8, 2024 08:05
@pankaj-peerdb pankaj-peerdb force-pushed the clickhouse-add-connector branch from 701e2cd to a07b74d Compare January 8, 2024 09:36
@iskakaushik
Copy link
Contributor

add qrep_flow_sf_test.go equivalent for clickhouse

@iskakaushik
Copy link
Contributor

Tested this locally. Promising. Blockers for landing:

  1. Create PEER should take AWS creds as params. Mandated.
  2. Add tests for datatypes, this is covered in existing snowflake tests.
  3. @iskakaushik to add docs about the required permissions on Clickhouse.

}

func (c *ClickhouseConnector) ConsolidateQRepPartitions(config *protos.QRepConfig) error {
c.logger.Info("Consolidating partitions noop")
Copy link
Contributor

Choose a reason for hiding this comment

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

support UPSERT as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore as first target for CDC only needs append.

cleanup

cleanup

cleanup

cleanup

cleanup

remove metadataSchema

cleanup

cleanup

cleanup
@pankaj-peerdb pankaj-peerdb force-pushed the clickhouse-add-connector branch from 127a009 to 0845521 Compare January 16, 2024 12:47
@@ -663,6 +663,16 @@ func (h *FlowRequestHandler) CreatePeer(
}
s3Config := s3ConfigObject.S3Config
encodedConfig, encodingErr = proto.Marshal(s3Config)
case protos.DBType_CLICKHOUSE:

Copy link
Contributor

Choose a reason for hiding this comment

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

remove this new line

_ "github.com/ClickHouse/clickhouse-go/v2"
_ "github.com/ClickHouse/clickhouse-go/v2/lib/driver"

//_ "github.com/ClickHouse/clickhouse-go"
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

logger slog.Logger
}

// creating this to capture array results from clicknhouse.
Copy link
Contributor

Choose a reason for hiding this comment

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

you mentioned array support not being ready, can this be added as a part of array support MR?

return &ClickhouseConnector{
ctx: ctx,
database: database,
//database: conn,
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

dsn := fmt.Sprintf("tcp://%s:%d?username=%s&password=%s", //&database=%s"
config.Host, config.Port, config.User, config.Password) //, config.Database

fmt.Println("connecting...", dsn)
Copy link
Contributor

@iskakaushik iskakaushik Jan 16, 2024

Choose a reason for hiding this comment

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

remove printlns from this file, pass a logger to this, see snowflake connector as an example.

}

// parseTableName parses a table name into schema and table name.
func parseTableName(tableName string) (*tableNameComponents, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

see: ParseSchemaTable method.


func (c *ClickhouseConnector) getTableSchema(tableName string) ([]*sql.ColumnType, error) {
//nolint:gosec
queryString := fmt.Sprintf(`
Copy link
Contributor

Choose a reason for hiding this comment

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

this query can be in one line, reads better

return false, fmt.Errorf("failed to execute query: %w", err)
}

//return count.Int64 > 0, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this comment

}

func (c *ClickhouseConnector) SetupQRepMetadataTables(config *protos.QRepConfig) error {

Copy link
Contributor

Choose a reason for hiding this comment

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

emplty new line - remove

"github.com/PeerDB-io/peer-flow/model"
"github.com/PeerDB-io/peer-flow/model/qvalue"
"github.com/PeerDB-io/peer-flow/shared"
_ "github.com/snowflakedb/gosnowflake"
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need thios?

@iskakaushik iskakaushik merged commit a0a663f into main Jan 17, 2024
7 checks passed
@serprex serprex deleted the clickhouse-add-connector branch July 19, 2024 15:26
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.

3 participants