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

feat: Add JSON type schema #1796

Merged
merged 7 commits into from
Jul 24, 2024
Merged

feat: Add JSON type schema #1796

merged 7 commits into from
Jul 24, 2024

Conversation

erezrokah
Copy link
Member

@erezrokah erezrokah commented Jul 17, 2024

Summary

Part of cloudquery/cloudquery#2023 Still WIP and depends on cloudquery/cloudquery-api-go#196


Use the following steps to ensure your PR is ready to be reviewed

  • Read the contribution guidelines 🧑‍🎓
  • Run go fmt to format your code 🖊
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests 🧪
  • Ensure the status checks below are successful ✅

@github-actions github-actions bot added the feat label Jul 17, 2024
transformers/struct.go Outdated Show resolved Hide resolved
transformers/struct.go Outdated Show resolved Hide resolved
schema/column.go Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Jul 17, 2024

⏱️ Benchmark results

Comparing with 7955c53

  • Glob-8 ns/op: 91.89 ⬇️ 2.50% decrease vs. 7955c53

@github-actions github-actions bot added feat and removed feat labels Jul 17, 2024
transformers/struct.go Outdated Show resolved Hide resolved
@erezrokah erezrokah marked this pull request as ready for review July 17, 2024 14:04
@erezrokah erezrokah requested a review from a team as a code owner July 17, 2024 14:04
@erezrokah erezrokah requested a review from bbernays July 17, 2024 14:04
@bbernays
Copy link
Contributor

Adding @marianogappa as a reviewer to this because I don't have the bandwidth to dive into this and give it the proper level of attention

schema/column.go Outdated Show resolved Hide resolved
transformers/struct.go Outdated Show resolved Hide resolved
transformers/struct_test.go Show resolved Hide resolved
@@ -111,17 +118,11 @@ func (t *structTransformer) addColumnFromField(field reflect.StructField, parent
return nil
}

columnType, err := t.typeTransformer(field)
columnType, err := t.getColumnType(field)
Copy link
Member Author

Choose a reason for hiding this comment

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

Extracted getColumnType to a dedicated function

@erezrokah
Copy link
Member Author

Sorry for the review noise, I basically re-did the PR without using TransformWithStruct as it didn't provide me the output I wanted from the schema. Please see updated tests that I believe look better now and always return a valid JSON

Copy link
Contributor

@marianogappa marianogappa left a comment

Choose a reason for hiding this comment

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

Some comments

@@ -46,6 +46,8 @@ type Column struct {

// PrimaryKeyComponent is a flag that indicates if the column is used as part of the input to calculate the value of `_cq_id`.
PrimaryKeyComponent bool `json:"primary_key_component"`

TypeSchema string `json:"type_schema,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

All the other fields here have docs. I think a comment is warranted on this one too 🤔

transformers/struct.go Outdated Show resolved Hide resolved
transformers/struct.go Show resolved Hide resolved
transformers/struct.go Outdated Show resolved Hide resolved
transformers/struct_test.go Show resolved Hide resolved
transformers/struct_test.go Show resolved Hide resolved
@kodiakhq kodiakhq bot merged commit dbc534b into main Jul 24, 2024
8 checks passed
@kodiakhq kodiakhq bot deleted the feat/add_json_type_schema branch July 24, 2024 11:20
kodiakhq bot pushed a commit that referenced this pull request Jul 24, 2024
🤖 I have created a release *beep* *boop*
---


## [4.52.0](v4.51.0...v4.52.0) (2024-07-24)


### Features

* Add JSON type schema ([#1796](#1796)) ([dbc534b](dbc534b))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
kodiakhq bot pushed a commit that referenced this pull request Jul 24, 2024
#### Summary

I missed this in #1796. Actually added the `normalizePointer` for this reason just forgot to use it for map and slice pointers 

---
kodiakhq bot pushed a commit to cloudquery/cloudquery that referenced this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants