- 
                Notifications
    
You must be signed in to change notification settings  - Fork 161
 
feat(go/adbc/driver/bigquery): Add extra table metadata #3469
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
base: main
Are you sure you want to change the base?
Conversation
The driver wasn't returning a lot of the metadata available to the Go SDK. This commit adds (most of) that data.
| 
           Given the discussion by other dbt engineers in #3449 maybe these values should be namespaced?  | 
    
| 
           @lidavidm I was not aware of that discussion 😅 We opened this PR internally to unblock some use cases but happy to change the key names and upstream again if we wanna namespace or change the names in this.  | 
    
| 
           @lidavidm I don't think everything should be namespaced, only keys that are likely to apply to many different drivers/systems.  | 
    
| 
               | 
          ||
| // Encode a value as a JSON string. Returns "" in case the value is empty or if there was | ||
| // an error | ||
| func encodeJson[S ~[]E | ~map[string]E, E any](v S) string { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zeroshade are you happy with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not happy with this. But I'm not good enough at go to know a better way to ensure v is JSON-encodable and can be used with len(v). Happy to change if anyone has better ideas.
This at least compiles tho :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just:
encoded, _ := json.Marshal(v)
return string(encoded)which should return either the encoded value or an empty string if it can't be encoded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't preserve the current behavior:
if len(v) == 0 {
    encoded := ""
} else {
    encoded := json.Marshal(v)
}If we just json.Marshal a value with length 0 we'll get [] instead of empty string.
We could change to simplify, but that would be a breaking change :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@serramatutu can you skip setting the value on metadata altogether when its empty?
Going from
	metadata["Labels"] = encodeJson[map[string]string, string](md.Labels)to something like this:
encodeJson[map[string]string, string](metadata, "Labels", md.Labels)Helping reduce the bloat caused by keys in the metadata map and reducing the number of possible states to "either it exists with something or is not set at all".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think they should be namespaced, vs just dumping a bunch of values directly onto the schema (if the point is to prevent users from accidentally referring to the wrong metadata, which was the justification of namespacing some values in the first place, why doesn't that apply here?)
The driver wasn't returning a lot of the metadata available to the Go SDK.
This commit adds (most of) that data.