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

Schema builder #15

Merged
merged 1 commit into from
Apr 27, 2022
Merged

Schema builder #15

merged 1 commit into from
Apr 27, 2022

Conversation

efirs
Copy link
Collaborator

@efirs efirs commented Mar 27, 2022

No description provided.

@efirs efirs force-pushed the schema_support branch 4 times, most recently from a236a99 to a878690 Compare March 27, 2022 17:56
@ovaistariq
Copy link
Collaborator

High level comment - @himank @efirs are we going to have the high level client in this same repo?

@efirs
Copy link
Collaborator Author

efirs commented Mar 28, 2022

Yes, I think it natural to have them co-located in the same repo.
And actually ideally we could have Golang client as a part of TigrisDB repo.
Pulling client as

go get github.com/tigrisdata/tigrisdb/client

look nicer to me.

@ovaistariq
Copy link
Collaborator

Remember that we are not building a "go" only database. We need to make sure all languages have first class support and that the usage is similar.

So it would be unnatural for only the go client to be part of the server repo.

Secondly, tigrisdb allows HTTP access without requiring a special client. Thus, embedding a client in the server repo would not send a coherent message.

Yes, I think it natural to have them co-located in the same repo.

And actually ideally we could have Golang client as a part of TigrisDB repo.

Pulling client as


go get github.com/tigrisdata/tigrisdb/client

look nicer to me.

client/client.go Outdated Show resolved Hide resolved
client/schema.go Outdated Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
client/schema.go Outdated Show resolved Hide resolved
client/schema.go Outdated Show resolved Hide resolved
client/schema.go Outdated
i := 1
if len(pks) > 1 {
if len(pks) > 2 {
return 0, fmt.Errorf("only one colon allowed in the tag")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a comment when this can happen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When user have more then one colon in the tag, for example "trigris:primary_key:a:b". The error message is self-explanatory IMO.
There is a test for it: https://github.com/tigrisdata/tigris-client-go/blob/6d913e6e5187748bdd9208af04e0eb41f25bfc28/schema/schema_test.go#L71

client/schema.go Outdated Show resolved Hide resolved
@himank
Copy link
Collaborator

himank commented Mar 29, 2022

High level comment - @himank @efirs are we going to have the high level client in this same repo?

yes, so low level client and high level client will be in same repo - in go client repo.

@efirs efirs force-pushed the schema_support branch 2 times, most recently from 806ba2c to 7a0cd6f Compare April 24, 2022 08:31
@efirs efirs changed the title Schema support Schema builder Apr 24, 2022
client/database.go Outdated Show resolved Hide resolved
client/database.go Outdated Show resolved Hide resolved
client/database.go Outdated Show resolved Hide resolved
client/database.go Outdated Show resolved Hide resolved
client/database.go Outdated Show resolved Hide resolved
schema/schema.go Outdated Show resolved Hide resolved
schema/schema_test.go Outdated Show resolved Hide resolved
schema/schema_test.go Outdated Show resolved Hide resolved
schema/schema_test.go Outdated Show resolved Hide resolved
schema/schema_test.go Outdated Show resolved Hide resolved
@efirs efirs force-pushed the schema_support branch 3 times, most recently from e72eaf5 to 622c9cb Compare April 26, 2022 20:21
@codecov-commenter
Copy link

Codecov Report

Merging #15 (622c9cb) into main (b118be7) will increase coverage by 4.17%.
The diff coverage is 87.41%.

@@            Coverage Diff             @@
##             main      #15      +/-   ##
==========================================
+ Coverage   80.76%   84.93%   +4.17%     
==========================================
  Files          10       11       +1     
  Lines         863     1082     +219     
==========================================
+ Hits          697      919     +222     
+ Misses        133      123      -10     
- Partials       33       40       +7     
Impacted Files Coverage Δ
client/database.go 27.08% <33.33%> (-5.35%) ⬇️
schema/schema.go 92.24% <92.24%> (ø)
client/client.go 36.84% <100.00%> (+36.84%) ⬆️
driver/driver.go 97.91% <100.00%> (ø)
driver/grpc.go 88.38% <100.00%> (ø)
driver/http.go 77.08% <100.00%> (ø)
driver/internal.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b118be7...622c9cb. Read the comment docs.

himank
himank previously approved these changes Apr 26, 2022
@efirs efirs merged commit c8718e1 into main Apr 27, 2022
@efirs efirs deleted the schema_support branch April 27, 2022 19:28
@github-actions
Copy link

🎉 This PR is included in version 1.0.0-alpha.9 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@github-actions
Copy link

🎉 This PR is included in version 1.0.0-beta.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented May 9, 2023

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

None yet

4 participants