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.

Sign up for GitHub

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

Jump to bottom

feat: adding JSON scalar #82

Merged
merged 3 commits into from
Aug 22, 2023
Merged

feat: adding JSON scalar #82

merged 3 commits into from
Aug 22, 2023

Conversation

mnorbury
Copy link
Collaborator

fixes: #63

@mnorbury mnorbury linked an issue Aug 22, 2023 that may be closed by this pull request
Add json to scalar interfaces #63
Closed
erezrokah
erezrokah requested changes Aug 22, 2023
View reviewed changes
Copy link
Member

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

Looks great. A couple comments. Once this is merge I'll update my PR to use Jackson

lib/src/main/java/io/cloudquery/scalar/JSON.java

import static io.cloudquery.scalar.ValidationException.NO_CONVERSION_AVAILABLE;

import com.fasterxml.jackson.databind.ObjectMapper;
Copy link
Member

Choose a reason for hiding this comment

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

Should jackson be added as a dependency to build.gradle?

Unrelated, but I used gson in https://github.com/cloudquery/plugin-sdk-java/pull/81/files#diff-655a69127303f6948c0b150902436756156ec7f82640e994c1f552cbdec5bbceR48

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you have a preference. I think I saw some hints that Jackson is slightly faster, which is why I went with it, but I have no solid preference.

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind, whatever uses less boilerplate code. I read that Jackson serializes dates as long number while gson as strings (with their own format), but probably we should worry about that now.

lib/src/main/java/io/cloudquery/scalar/JSON.java Show resolved Hide resolved
erezrokah
erezrokah reviewed Aug 22, 2023
View reviewed changes
lib/build.gradle Outdated
@@ -42,6 +42,9 @@ dependencies {
implementation "org.apache.arrow:arrow-memory-core:12.0.1"
implementation "org.apache.arrow:arrow-vector:12.0.1"

implementation "com.fasterxml.jackson.core:jackson-annotations:2.15.1"
implementation "com.fasterxml.jackson.core:jackson-annotations:2.15.1"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
implementation "com.fasterxml.jackson.core:jackson-annotations:2.15.1"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The second dependency was supposed to be the jackson-core fixed here

Copy link
Member

Choose a reason for hiding this comment

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

Woops deleted one by mistake, let me bring it back

Copy link
Member

Choose a reason for hiding this comment

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

Actually we don't need annotations at the moment, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did add them here in a previous PR, so we do support them for custom JSON field names.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, cool. Let me add it back

@mnorbury mnorbury force-pushed the chore/add-json-to-scalar-interfaces branch from c12e5ec to a8b12bd Compare August 22, 2023 08:36
@mnorbury mnorbury added the automerge Add to automerge PRs once requirements are met label Aug 22, 2023
@erezrokah erezrokah removed the automerge Add to automerge PRs once requirements are met label Aug 22, 2023
@erezrokah erezrokah changed the title chore: adding JSON scalar feat: adding JSON scalar Aug 22, 2023
@erezrokah erezrokah added the automerge Add to automerge PRs once requirements are met label Aug 22, 2023
@kodiakhq kodiakhq bot merged commit fc92542 into main Aug 22, 2023
5 checks passed
@kodiakhq kodiakhq bot deleted the chore/add-json-to-scalar-interfaces branch August 22, 2023 08:50
@cq-bot cq-bot mentioned this pull request Aug 22, 2023
Merged
@erezrokah erezrokah mentioned this pull request Aug 22, 2023
Merged
kodiakhq bot pushed a commit that referenced this pull request Aug 22, 2023
@erezrokah
fix: Add jackson-annotations to build.gradle (#83)
ead7dd9 
I removed this by mistake in #82
kodiakhq bot pushed a commit that referenced this pull request Aug 24, 2023
@cq-bot
chore(main): Release v0.0.1 (#16)
c20589b 
🤖 I have created a release *beep* *boop*
---


## 0.0.1 (2023-08-24)


### Features

* `io.cloudquery.scalar.Binary` implementation ([#20](#20)) ([b9b73d1](b9b73d1))
* `io.cloudquery.scalar.Bool` ([#27](#27)) ([2a91c92](2a91c92)), closes [#26](#26)
* adding basic support for tables ([#19](#19)) ([22b2350](22b2350))
* adding JSON scalar ([#82](#82)) ([fc92542](fc92542)), closes [#63](#63)
* adding Table filterDFS functionaility ([#21](#21)) ([02d8515](02d8515))
* Date scalars ([#36](#36)) ([adc6ba2](adc6ba2)), closes [#34](#34)
* Duration scalar ([#42](#42)) ([7529438](7529438)), closes [#39](#39)
* Encode resources with data ([#88](#88)) ([2c7060f](2c7060f))
* Generics in scalars ([#56](#56)) ([bc7d6e3](bc7d6e3))
* Implement `getTables` ([#71](#71)) ([085c51f](085c51f))
* Implement concurrency and relations resolving ([#91](#91)) ([0a470b7](0a470b7))
* Init logger, add initial MemDB plugin ([#70](#70)) ([20ebb42](20ebb42))
* int/uint/float/string scalars ([#59](#59)) ([39ec6e6](39ec6e6)), closes [#53](#53) [#54](#54) [#58](#58) [#60](#60)
* Resolve CQId, add CQIds to MemDB plugin ([#95](#95)) ([9d7f1bd](9d7f1bd))
* Scalar Timestamp ([#46](#46)) ([4220e92](4220e92)), closes [#44](#44)
* **sync:** Initial insert message support ([#81](#81)) ([bd729bb](bd729bb))
* **sync:** Send migrate messages ([#79](#79)) ([dd2c1a5](dd2c1a5))


### Bug Fixes

* Add `jackson-annotations` to `build.gradle` ([#83](#83)) ([ead7dd9](ead7dd9))
* **deps:** Update dependency com.google.guava:guava to v32 ([#15](#15)) ([ce8028b](ce8028b))
* **deps:** Update dependency io.grpc:grpc-protobuf to v1.57.1 ([#10](#10)) ([bcfa29c](bcfa29c))
* **deps:** Update dependency io.grpc:grpc-services to v1.57.1 ([#11](#11)) ([71c2ea1](71c2ea1))
* **deps:** Update dependency io.grpc:grpc-stub to v1.57.1 ([#12](#12)) ([c65e5d6](c65e5d6))
* **deps:** Update dependency io.grpc:grpc-testing to v1.57.1 ([#13](#13)) ([a7b1fa6](a7b1fa6))
* **deps:** Update plugin org.gradle.toolchains.foojay-resolver-convention to v0.6.0 ([#14](#14)) ([443990c](443990c))
* Flatten tables in getTables gRPC call ([#80](#80)) ([8c9872a](8c9872a))
* Pass options to tables method ([#78](#78)) ([4b77a2f](4b77a2f))


### Miscellaneous Chores

* Release 0.0.1 ([e169dbc](e169dbc))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erezrokah erezrokah erezrokah approved these changes

@candiduslynx candiduslynx Awaiting requested review from candiduslynx

Assignees
No one assigned
Labels
automerge Add to automerge PRs once requirements are met
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add json to scalar interfaces
2 participants
@mnorbury @erezrokah