feat(bigquery): make preview configurable

[StewartJingga]

• new disable_preview config on bigquery extractor

[rohilsurana]

One thought I had was to let preview have its own config struct. I noticed that there is a var previewTotalRows. If in future we could have a need to expose it as a config and more such stuff can be introduced, we can as of now start with a struct for PreviewConfigs and have disable and totalRows as its fields.

Functionally the PR looks good to me.

[StewartJingga]

One thought I had was to let preview have its own config struct. I noticed that there is a var previewTotalRows. If in future we could have a need to expose it as a config and more such stuff can be introduced, we can as of now start with a struct for PreviewConfigs and have disable and totalRows as its fields.

Functionally the PR looks good to me.

Makes sense, but another thought is, should we just use TotalPreviewRows? we can put 0 to disable preview fetching and will default to 30 or other numbers.

Do you think it's better or do you see limitiations with it?

[rohilsurana]

I think that could work as well, so we put a check on the TotalPreviewRows to return if its equal or less than 0, to avoid even triggering the BQ query.

I think we can start with this. It makes it implicit but should be fine.

[StewartJingga]

@rohilsurana updated here. 🙏

[StewartJingga]

@rohilsurana actually we need another flag for disable. We can do like what you propose using nested config here.

reason is, when I put 0 as the config, it will be treated as an empty value and set it to its default value (30 atm). So we would never be able to get 0 as the MaxPreviewRows.

[rohilsurana]

That sounds like a bug of how we are loading the values into config struct, do you maybe want to address it there?
Is this happening using the odpf/salt/config package?

[rohilsurana]

Just checked we are setting the defaults here - https://github.com/odpf/meteor/blob/dd15e2532141bd5fb7d7d0bd591edc4e66266132/utils/config.go#L17-L22

I think we should load the defaults first into the struct and then the configs from configMap.
Is there any specific reason that I am missing to load defaults later?

[StewartJingga]

I think we should load the defaults first into the struct and then the configs from configMap.
Is there any specific reason that I am missing to load defaults later?

@rohilsurana oh there's no specific reason, was just a wrong order and behaviour. Updated the PR with the fix 🙏 .