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

Replace binary columns with text #3

Merged
merged 3 commits into from
Sep 22, 2016
Merged

Replace binary columns with text #3

merged 3 commits into from
Sep 22, 2016

Conversation

MrTin
Copy link
Contributor

@MrTin MrTin commented Sep 1, 2016

Why?

Binary columns imposes some unwanted restrictions. Using text will make hive compatible to a wider range of databases and versions. When you are using a more efficient database like Postgres, it makes no difference of varchar and text anyway.

Mysql might experience a performance drawback when creating temporary tables but it's not applicable to our use case. Neither from my understanding, do we need to be able to index these columns.

Affected columns

  • contents
  • changes

Developer notes

How do I test this @marcelklehr? I can't see any test suite so I would at least want to do some manul acceptance testing however as my knowledge is somewhat limited working with Node, how would I go about? Is there a way for me to specify in eg. my package.json that I would like to use this repository instead of the npm version?

Other

Related to #1
Closes hivejs/hive#108

@marcelklehr
Copy link
Member

marcelklehr commented Sep 1, 2016

I think npm-link is probably what you want.

@marcelklehr
Copy link
Member

I use local paths in my package.json, though.

@marcelklehr
Copy link
Member

marcelklehr commented Sep 1, 2016

Confirmed to work with postgres! (had some fun creating a 1.6M file ;) )

@MrTin
Copy link
Contributor Author

MrTin commented Sep 2, 2016

Thanks! According to http://sailsjs.org/documentation/concepts/models-and-orm/attributes mediumtext and longtext are supported as well. It's also supported by MySQL and Postgres which covers the most common databases I would say :)

Let me know if you want me to change this. My suggestion is to go with mediumtext as 16M should be plenty of space for a document ;-)

I'll test this out on my machine to verify if this is the culprit of the bug and report back in a bit!

@MrTin
Copy link
Contributor Author

MrTin commented Sep 2, 2016

Confirmed, this is now working perfectly :)

Just need your decision on what column type to go for 👍

@marcelklehr
Copy link
Member

Let's go with mediumtext, then! :)

@MrTin
Copy link
Contributor Author

MrTin commented Sep 22, 2016

Sorry for the slow fix. I just updated this to use mediumtext instead. Should be ready to go unless you would like me to add tests anywhere?

@marcelklehr
Copy link
Member

No problem. Life always gets the better of us ;)
And the thing with tests is: there are none (yet). The first place to add some would probably be in hive-core as hive-ui is rapidly changing, atm.

@marcelklehr marcelklehr merged commit 065c463 into hivejs:master Sep 22, 2016
@MrTin
Copy link
Contributor Author

MrTin commented Sep 23, 2016

Sure does ;) Following with excitement!

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.

2 participants