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

Restrict sending PreviewRows fields in table asset from meteor to compass #43

Closed
utsav14nov opened this issue Nov 17, 2023 · 1 comment · Fixed by #46
Closed

Restrict sending PreviewRows fields in table asset from meteor to compass #43

utsav14nov opened this issue Nov 17, 2023 · 1 comment · Fixed by #46
Assignees

Comments

@utsav14nov
Copy link

utsav14nov commented Nov 17, 2023

Problem statement
Currently when MaxPreviewRows value in meteor recipe is 0, meteor fetches 0 rows from BQ and send null value of PreviewRows in table asset. As compass upsert api is being called, it updates the preview rows in compass asset to null. But for keeping preview rows static in compass we do not have any config which doesn't update preview rows into compass and let it be static.

Proposed solution
Compass doesn't update the field which is not being sent into asset. Hence to solve this, we come up with the solution to not to send PreviewRows field in table asset when MaxPreviewRows is set to -1. Hence it will not update the PreviewRows in compass and make it static.

source:
  type: bigquery
  scope: sample-scope
  config:
    project_id: sample-project
    max_preview_rows: -1

28-Nov-2023 [Update]

Even after following the above approach in bigquery extractor, it is not completely solved.
Following code at sink side adds back the unpopulated fields with default values while marshalling back to proto.

	data, err := protojson.MarshalOptions{
		UseProtoNames:   true,
		EmitUnpopulated: true,
	}.Marshal(anyData)

EmitUnpopulates is set to true which adds the unpopulated fields with empty value i.e. null and [] for preview_rows and preview_fields
Hence finally empties value at compass side.

Following are the proposed solutions for this issue: (Preferred approach is 2)

Approach 1

Hard code EmitUnpopulated to false in compass sink like following

	data, err := protojson.MarshalOptions{
		UseProtoNames:   true,
		EmitUnpopulated: false,
	}.Marshal(anyData)

Cons: It will change this configuration for all fields in asset for every every compass sink where this logic may not be needed.

Approach 2

Create a backward compatible sink config i.e. remove_unset_fields to either true or false which defaults to false and keep the negation of it in marshal options like following code

RemoveUnsetFields bool mapstructure:"remove_unset_fields"

	data, err := protojson.MarshalOptions{
		UseProtoNames:   true,
		EmitUnpopulated: !s.config.RemoveUnsetFields,
	}.Marshal(anyData)

Pros: It will be configurable and backward compatible. i.e. when remove_unset_fields is not set , it will default to false and hence finally EmitUnpopulated to true, So no changes required for current deployments.

Cons: It is changing config for all fields available in asset data.

Approach 3

Extra changes in BQ extractor: Unmarshal table asset after removing preview_rows and preview_fields as done above and marshal it back to new proto not having PreviewRows and PreviewFields (Maybe TableV2). Hence final proto would be marshal at sink end according to the new proto sent i.e TableV2 having no unset fields.

Pros: No changes required at sink side
Cons: Extra cost and time to convert and back to new proto, hence extra processing.

@haveiss
Copy link
Collaborator

haveiss commented Nov 28, 2023

Approach 2 is preferable IMHO.

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 a pull request may close this issue.

2 participants