feat: Implement basic transformer (add/remove/obfuscate columns) #18704

marianogappa · 2024-07-26T10:32:13Z

This adds a plugins/transformer/basic folder with a basic transformer plugin that enables the following 3 transformations:

adding a literal string column
removing columns
obfuscating string columns

~~Temporary limitation:~~
- I discovered yesterday that schema changes don't work due to the previous PR (which enables transformation support in sync_v3) not transforming the MigrateTable message. I'll fix that today. Obfuscation works, though.

Because this is early days and we might end up reusing behaviour on different transformers (which means we might want to lift some of this to the sdk), I separated concerns into different packages and tested them separately.

The design looks like this:

And then for each transformer within this plugin:

Given this spec:

---
kind: source
spec:
  name: "xkcd"
  path: "cloudquery/xkcd"
  version: "v1.0.6"
  tables: ['*']
  destinations:
    - "postgresql"
  spec:
---
kind: transformer # new transformer kind
spec:
  name: "basic"
  path: "localhost:7777"
  registry: "grpc"
  spec:
    transformations:
      - kind: obfuscate_columns
        tables: ["*"]
        columns: ["safe_title", "title"]
---
kind: destination
spec:
  name: "postgresql"
  path: "cloudquery/postgresql"
  registry: "cloudquery"
  version: "v8.0.7"
  write_mode: "overwrite-delete-stale"
  migrate_mode: forced
  transformers: # new transformer field
    - "basic" # example transformer

  spec:
    connection_string: "postgresql://mariano.gappa:@localhost:5432/xkcd?sslmode=disable"

It runs successfully in normal elapsed time:

✓ Code/test-xkcd-postgres-sync  $ cli sync cloudquery-config                                                                
Loading spec(s) from cloudquery-config
Starting sync for: xkcd (cloudquery/xkcd@v1.0.6) -> [postgresql (cloudquery/postgresql@v8.0.7)]
Sync completed successfully. Resources: 2962, Errors: 0, Warnings: 0, Time: 15s

And obfuscates title & safe_title columns:

UPDATE: all functionality works; here's an example spec:

---
kind: transformer # new transformer kind
spec:
  name: "basic"
  path: "localhost:7777"
  registry: "grpc"
  spec:
    transformations:
      - kind: obfuscate_columns
        tables: ["*"]
        columns: ["safe_title", "title"]
      - kind: remove_columns
        tables: ["*"]
        columns: ["transcript", "news"]
      - kind: add_column
        tables: ["*"]
        name: "source"
        value: "xkcd"

And the result (same elapsed time):

erezrokah

The plugin code looks great. Added a few comments.
For the migrate message bit we probably want it to be a protocol message and not record metadata.

Also where does the plugin sets the cq:extension:message_type I couldn't find it.

Regardless, I think the plugin's code (not the CLI part) can go into the main branch once the comments are resolved

erezrokah · 2024-07-30T09:29:45Z

plugins/transformer/basic/resources/plugin/plugin.go

+// Don't move this file to a different package, it's used by Go releaser to embed the version in the binary.
+var (
+	Name    = "basic"
+	Kind    = "destination" // breaks otherwise


Let's update this to transformer supported in https://github.com/cloudquery/plugin-sdk/releases/tag/v4.54.0

erezrokah · 2024-07-30T09:32:07Z

cli/cmd/sync_v3.go

+			// This could either be an `Insert` or a `MigrateTable` message. To answer this,
+			// we look to see if the schema's metadata has a MetadataMessageTypeKey key
+			msgType := "insert"
+			if isMigrateTableRecord(record) {


This should probably be part of the protocol not part of the data

This change belongs to the parent PR. Let's discuss it there.

For context, we changed the protocol after an offline discussion.

erezrokah · 2024-07-30T09:36:16Z

plugins/transformer/basic/.gitignore

@@ -0,0 +1 @@
+basic


Maybe call it toolkit? Not sure what's a better name for it

I'm not married to basic, but I chose a name that clarified that advanced transformations would never be in scope, because as we know it so far we cannot add Datafusion within Go in a newer version. I'm not strongly opposed to toolkit but MacGyver would laugh in our face 🤷

I think basic is okay for now. We can always change it later by releasing it under a different name

plugins/transformer/basic/client/client.go

erezrokah · 2024-07-30T09:41:43Z

plugins/transformer/basic/client/spec/spec.go

+}
+
+func (s *Spec) Validate() error {
+	for _, t := range s.TransformationSpecs {


Non blocking, it would be nice to collect all errors and report them instead of only the first one each time.
You can use errors.Join for that

Used errors.Join now.

erezrokah · 2024-07-30T09:44:28Z

plugins/transformer/basic/client/transformers/transformers.go

+func NewFromSpec(sp spec.TransformationSpec) (*Transformer, error) {
+	tr := &Transformer{matcher: tablematcher.New(sp.Tables)}
+
+	fns := map[string]TransformationFn{


Would a switch/case be more suitable here?

Used switch.

plugins/transformer/basic/client/tablematcher/table_matcher.go

erezrokah · 2024-07-30T09:54:48Z

plugins/transformer/basic/client/recordupdater/record_updater.go

+	mem := memory.NewGoAllocator()
+	bld := array.NewStringBuilder(mem)


Suggested change

mem := memory.NewGoAllocator()

bld := array.NewStringBuilder(mem)

bld := array.NewStringBuilder(memory.DefaultAllocator)

And everywhere else should give the same result

TIL. Updated everywhere.

erezrokah · 2024-07-30T10:01:09Z

plugins/transformer/basic/client/recordupdater/record_updater.go

+	return bld.NewStringArray()
+}
+
+func (*RecordUpdater) _obfuscateColumn(column arrow.Array) arrow.Array {


What's the reason for _obfuscateColumn and not obfuscateColumn?

Originally RecordUpdater had ObfuscateColumn rather than ObfuscateColumns. Name clash. But I took out the _ now.

erezrokah · 2024-07-30T12:00:35Z

This can be merged directly to main I believe

plugins/transformer/basic/README.md

Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>

Co-authored-by: Kemal <223029+disq@users.noreply.github.com>

erezrokah

Code looks good but I think that in order to package/publish it we only need a docs directory with an overview.md file.

See example in https://github.com/cloudquery/cloudquery/tree/main/plugins/source/xkcd/docs

marianogappa · 2024-07-31T15:53:01Z

@erezrokah yep added a sample docs and the package command works. I'm just having trouble with publishing to local; some auth problem. I can't debug the failing SQL query because the SQL uses some variables that I guess only exist in the server's connection.

erezrokah · 2024-07-31T16:05:15Z

plugins/transformer/basic/docs/overview.md

+  path: "localhost:7777" # TODO change this when it's published
+  registry: "grpc" # TODO change this when it's published


Suggested change

path: "localhost:7777" # TODO change this when it's published

registry: "grpc" # TODO change this when it's published

path: cloudquery/basic

version: VERSION_TRANSFORMER_BASIC

This is our convention, the VERSION_TRANSFORMER_BASIC will be replaced during published with the version being published so the docs always have the latest version.

erezrokah

We have a convention to create _configuration.md file that can be embedded in other documents.

See https://github.com/cloudquery/cloudquery/blob/main/plugins/source/xkcd/docs/_configuration.md

Also

cloudquery/plugins/source/xkcd/docs/overview.md

Line 16 in cf45e5a

:configuration

The Cloud also uses this _configuration.md file if exists for pages like https://hub.cloudquery.io/export-data/airtable/spangenberg_arrowflight#source-configuration

hermanschaaf

Looks great! 🚀🚀🚀

hermanschaaf · 2024-07-31T16:27:49Z

plugins/transformer/basic/client/recordupdater/record_updater.go

+	}
+
+	oldRecord := r.record.Columns()
+	newColumns := make([]arrow.Array, 0, len(oldRecord)-1)


Doesn't matter too much, but should the cap here be len(oldRecord)-len(colIndices) ?

hermanschaaf · 2024-07-31T16:29:30Z

plugins/transformer/basic/client/recordupdater/record_updater.go

+	return r.record, nil
+}
+
+func (r *RecordUpdater) ObfuscateColumns(columnNames []string) (arrow.Record, error) {


This is great for now, but I can see how in the future we may also need to handle fields inside JSON columns, for example.

I think before that we might need to obfuscate non-string fields too. And we can implement anything we want; the question is where is the line where bespoke transformations end and Datafusion-based transformations start.

erezrokah · 2024-07-31T18:15:23Z

Just realized this PR is missing the steps from https://github.com/cloudquery/cloudquery/blob/cf45e5a2d6225fccce9248919f1c280e5db286b6/contributing/adding_a_new_plugin_to_cq_monorepo.md

marianogappa · 2024-08-01T10:52:18Z

Steps added

🤖 I have created a release *beep* *boop* --- ## 1.0.0 (2024-08-05) ### Features * Implement basic transformer (add/remove/obfuscate columns) ([#18704](#18704)) ([704102f](704102f)) * Support table name changes on basic transformer. ([#18833](#18833)) ([67d3701](67d3701)) ### Bug Fixes * **deps:** Update module github.com/cloudquery/cloudquery-api-go to v1.12.0 ([#18448](#18448)) ([a5850e1](a5850e1)) * **deps:** Update module github.com/cloudquery/plugin-sdk/v2 to v4 ([#18822](#18822)) ([bfd8765](bfd8765)) * **deps:** Update module github.com/cloudquery/plugin-sdk/v2 to v4 ([#18842](#18842)) ([39beee4](39beee4)) * **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.57.0 ([#18821](#18821)) ([ef364a2](ef364a2)) * **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.57.1 ([#18830](#18830)) ([605c202](605c202)) * **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.58.0 ([#18839](#18839)) ([6b57bca](6b57bca)) * **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.58.1 ([#18852](#18852)) ([4320340](4320340)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

marianogappa added 7 commits July 23, 2024 11:39

Implement support for transformations in CLI.

815b178

Keep plugins folder as it is in main branch.

192d4bf

Remove test transformer.

ebdad9e

Remove incorrect rebase changes.

23ec33c

Merge branch 'main' into mariano/transformations-support

826f02f

Add transformerpipeline tests.

beabe30

Merge branch 'main' into mariano/transformations-support

088df8a

marianogappa requested review from erezrokah and hermanschaaf July 26, 2024 10:32

cq-bot added the area/cli label Jul 26, 2024

marianogappa added 2 commits July 26, 2024 15:30

Also transform MigrateTable message, so that schemas can change.

c1266e6

Merge branch 'main' into mariano/transformations-support

6b7009e

erezrokah reviewed Jul 30, 2024

View reviewed changes

cq-bot removed the area/cli label Jul 30, 2024

vercel bot deployed to Preview July 30, 2024 11:24 View deployment

erezrokah approved these changes Jul 30, 2024

View reviewed changes

erezrokah marked this pull request as ready for review July 30, 2024 12:00

erezrokah requested review from a team and savme and removed request for a team July 30, 2024 12:00

disq reviewed Jul 30, 2024

View reviewed changes

plugins/transformer/basic/README.md Outdated Show resolved Hide resolved

cq-bot added the needs-area label Jul 31, 2024

marianogappa and others added 7 commits July 31, 2024 12:08

Implement review comments. Use TransformSchema method.

4da8ca9

Merge branch 'main' into mariano/transformations-support

22febc7

Run go mod tidy.

6deb9b2

feat: Add basic transformer plugin.

84b3256

Rename function for clarity.

8fc3731

Also transform MigrateTable message, so that schemas can change.

c106934

Update plugins/transformer/basic/client/client.go

d0b15a3

Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>

Update plugins/transformer/basic/README.md

bc42ff3

Co-authored-by: Kemal <223029+disq@users.noreply.github.com>

marianogappa requested a review from erezrokah July 31, 2024 12:31

erezrokah approved these changes Jul 31, 2024

View reviewed changes

Add sample docs to ba able to publish.

46f83f8

erezrokah reviewed Jul 31, 2024

View reviewed changes

hermanschaaf approved these changes Jul 31, 2024

View reviewed changes

marianogappa added 4 commits August 1, 2024 10:42

Fix panic when spec is empty.

6428509

Follow docs conventions.

a3eca50

Fix outdated logic for calculating slice cap.

c2ff2e8

Add plugin to CQ Monorepo steps.

2ce76a2

marianogappa requested a review from a team August 1, 2024 10:18

cq-bot added the area/ci label Aug 1, 2024

Add transformer plugins to wait_for_required_workflows.

2e95627

erezrokah approved these changes Aug 1, 2024

View reviewed changes

Base automatically changed from mariano/transformations-support to main August 1, 2024 15:37

cq-bot added the area/cli label Aug 1, 2024

Merge branch 'main' into mariano/transformations-support-transformer

94edcfc

cq-bot removed the area/cli label Aug 1, 2024

marianogappa added area/new-transformer-plugin automerge Automatically merge once required checks pass and removed needs-area labels Aug 1, 2024

vercel bot deployed to Preview August 1, 2024 15:51 View deployment

kodiakhq bot merged commit 704102f into main Aug 1, 2024

kodiakhq bot deleted the mariano/transformations-support-transformer branch August 1, 2024 15:51

cq-bot mentioned this pull request Aug 1, 2024

chore(main): Release plugins-transformer-basic v1.0.0 #18820

Merged

		mem := memory.NewGoAllocator()
		bld := array.NewStringBuilder(mem)

	mem := memory.NewGoAllocator()
	bld := array.NewStringBuilder(mem)
	bld := array.NewStringBuilder(memory.DefaultAllocator)

		path: "localhost:7777" # TODO change this when it's published
		registry: "grpc" # TODO change this when it's published

feat: Implement basic transformer (add/remove/obfuscate columns) #18704

feat: Implement basic transformer (add/remove/obfuscate columns) #18704

Uh oh!

Conversation

marianogappa commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erezrokah commented Jul 30, 2024

Uh oh!

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

Uh oh!

marianogappa commented Jul 31, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

Uh oh!

hermanschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erezrokah commented Jul 31, 2024

Uh oh!

marianogappa commented Aug 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

marianogappa commented Jul 26, 2024 •

edited

Loading