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

Add binary and varbinary SQLDataType #124

Open
wants to merge 1 commit into
base: next
Choose a base branch
from
Open

Add binary and varbinary SQLDataType #124

wants to merge 1 commit into from

Conversation

Mladen-K
Copy link

Faster comparison and sorting of fields using binary and varbinary types in MySQL comparing to char and varchar. In case of binary/varbinary, comparison and sorting is based on the numeric values of the bytes in the values.

Faster comparison and sorting of fields using binary and varbinary types in MySQL comparing to char and varchar. In case of binary/varbinary, comparison and sorting is based on the numeric values of the bytes in the values.
@CLAassistant
Copy link

CLAassistant commented Apr 11, 2018

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@codecov-io
Copy link

codecov-io commented Apr 11, 2018

Codecov Report

Merging #124 into master will decrease coverage by 0.09%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #124     +/-   ##
=========================================
- Coverage   85.25%   85.16%   -0.1%     
=========================================
  Files          43       43             
  Lines        3575     3579      +4     
=========================================
  Hits         3048     3048             
- Misses        527      531      +4
Flag Coverage Δ
#SwiftKuery 85.16% <0%> (-0.1%) ⬇️
Impacted Files Coverage Δ
Sources/SwiftKuery/SQLDataType.swift 85.71% <0%> (-14.29%) ⬇️

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 039375f...f4c1b1d. Read the comment docs.

@EnriqueL8
Copy link
Contributor

Hey @Mladen-K , do the 3 SQL platforms (PostgresSQL, MySQL, and SQLite) that we support use the same notation? What I mean is does the Binary Type in Swift always translate to binary in SQL for the 3 platforms.

@Mladen-K
Copy link
Author

In PostgreSQL, there are both data types, although they are represented as BIT and BIT VARYING. Without testing on working PostgreSQL database, I can't tell for sure wether the generated query will generate required fields with "binary" and "varbinary". SQLite doesn't support binary types...

@EnriqueL8
Copy link
Contributor

I believe that instead of hardcoding a string in SwiftKuery, ideally we should add a case to the QueryBuilder and then override the default binary in Swift-Kuery-PostgreSQL to BIT and the same for varbinary. There is also the need to research how we handle types not being available in all three platforms, and try to take the same approach for SQLite

@EnriqueL8 EnriqueL8 self-requested a review April 17, 2018 15:44
Copy link
Contributor

@EnriqueL8 EnriqueL8 left a comment

Choose a reason for hiding this comment

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

Please see my comment above

@Mladen-K
Copy link
Author

Ok, I'll see what I can do.

@EnriqueL8
Copy link
Contributor

Thanks

@ianpartridge
Copy link
Contributor

@EnriqueL8
Copy link
Contributor

Hey @Mladen-K , any news on this?

@Mladen-K
Copy link
Author

@EnriqueL8, Just haven't had time to check it out. Sure will do in next few days.

@EnriqueL8
Copy link
Contributor

Thanks!

@kilnerm kilnerm changed the base branch from master to next September 20, 2018 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants