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

[MSSQL]DataTypes.STRING should be Varchar or NVarchar ? #6036

Open
p2227 opened this issue Jun 7, 2016 · 14 comments
Open

[MSSQL]DataTypes.STRING should be Varchar or NVarchar ? #6036

p2227 opened this issue Jun 7, 2016 · 14 comments
Labels
P4: nice to have For issues that are not bugs. type: performance For issues and PRs. Things that affect the performance of Sequelize.

Comments

@p2227
Copy link

p2227 commented Jun 7, 2016

version:3.19.3

http://docs.sequelizejs.com/en/latest/api/datatypes/

according to the docs, DataTypes.STRING should be map to Varchar. but i got nvarchar in mssql ?

@janmeier
Copy link
Member

janmeier commented Jun 7, 2016

return 'NVARCHAR(' + this._length + ')';

@janmeier janmeier closed this as completed Jun 7, 2016
@komeilshahmoradi
Copy link

version 4.42.0
I still have this problem
please, how to solve this problem ?

@durango
Copy link
Member

durango commented Dec 31, 2018

@komeilshahmoradi check out this issue #8533 might help guide you on how to extend data types to your liking. I am not sure why SequelizeJS decided to opt for nvarchar but it is, in my opinion, incorrect. I'll ask the team and see why this decision was made and what are our options for the future.

Alternatively, if you don't mind using varchar(255) you can use ENUM but this isn't really ideal

ENUM.prototype.toSql = function() {

Reopening because it is an issue

@durango durango reopened this Dec 31, 2018
@bjagadeesan
Copy link

bjagadeesan commented Jan 22, 2019

version: 4.42.0

I have the same problem. I need to use varchar instead of nvarchar. I execute raw queries through sequelize. The replacements always converts to nvarchar and it is a performance bottleneck for my operation. The regular query with varchar runs in under a second and the one with nvarchar runs anywhere from 30 to 45 seconds.

I tried overriding the tosql() method, but I couldn't get the extended version to be used in the program. Any help on that is much appreciated.

Below is the code .

import {DataTypes} from "sequelize";

DataTypes.STRING.prototype.toSql = function() {
	if (!this._binary) {
		return "VARCHAR(" + this._length + ")";
	} else{
		return "BINARY(" + this._length + ")";
	}
};

export {DataTypes};

But ideally, adding an option to choose between nvarchar and varchar will be much helpful.

@stale
Copy link

stale bot commented Apr 22, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still an issue, just leave a comment 🙂

@stale stale bot added the stale label Apr 22, 2019
@stale stale bot closed this as completed Apr 29, 2019
@SheetalGoel
Copy link

I am using "sequelize": "^5.21.5"
And still this issue causing a major impact on our operations.

@nareshy-quisitive
Copy link

Any update on this issue? Its impacting application performace.

@WikiRik WikiRik added the type: performance For issues and PRs. Things that affect the performance of Sequelize. label Mar 18, 2022
@WikiRik WikiRik reopened this Mar 18, 2022
@WikiRik
Copy link
Member

WikiRik commented Mar 18, 2022

@ephys I think you can answer this

@nareshy-quisitive
Copy link

nareshy-quisitive commented Mar 18, 2022

Issue is

const searchResults = await models.file.findAll({
        where: {
          path: {
            [Op.like]: `%${queryStr}%`
          }
        },
        attributes: ['path', 'uuid']
      })

Above code executing following query
select [uuid],[path] from [omn].[file] where [path] like N'%abc%'
which is taking around 7sec to complete the query. But if I execute the same query by removing 'N' from the query I am getting the same response within 1sec.

Note: path is varchar data type in MSSQL DataBase

@ephys
Copy link
Member

ephys commented Mar 18, 2022

I don't want to change what DataTypes.STRING produces, even in a SemVer MAJOR release, because it would make migrating too difficult. But I would like to deprecate DataTypes.STRING, see #14259

In the meantime, there is a workaround you can use that we just documented. You can use VARCHAR like this in mssql:

image

As for generating 'string' instead of N'string', it will need to wait until we have DataTypes.VARCHAR & DataTypes.VARCHAR.N so we can generate the appropriate string based on the DataType. This part will be tracked in #9107

@nareshy-quisitive
Copy link

@ephys suggestions are not working. We are using the 5.22.4 version. Do we need to upgrade this to the latest version?

@ephys
Copy link
Member

ephys commented Mar 24, 2022

I did a quick test in a SSCCE running with mssql and the following code:

class User extends Model {}

User.init({
  firstName: 'VARCHAR(50)',
}, {
  sequelize,
});

await sequelize.sync({ force: true });

Which resulted in the following SQL:

IF OBJECT_ID('[Users]', 'U') IS NOT NULL DROP TABLE [Users];

IF OBJECT_ID('[Users]', 'U') IS NULL CREATE TABLE [Users] ([id] INTEGER NOT NULL IDENTITY(1,1) , [firstName] VARCHAR(50) NULL, [createdAt] DATETIMEOFFSET NOT NULL, [updatedAt] DATETIMEOFFSET NOT NULL, PRIMARY KEY ([id]));

EXEC sys.sp_helpindex @objname = N'[Users]';

I was running it using Sequelize 6, it may be the case that this is not supported in Sequelize 5. There is a small number of breaking changes in Sequelize 6, I definitely recommend upgrading: https://sequelize.org/master/manual/upgrade-to-v6.html

@nareshy-quisitive
Copy link

nareshy-quisitive commented Apr 12, 2022

I have tested this with the latest version 6.18.0. still facing the same issue
here is my test model class

const config = require('../config/index')
const _ = require('lodash')
const sequelize = new Sequelize(config.get('database'), config.get('username'), config.get('password'), {
  dialect: config.get('dialect'),
  host: config.get('host'),
  logging: _.get(config, 'logging', console.log),
  benchmark: true,
  port: 1433,
  dialectOptions: {
    options: {
      encrypt: true,
      trustServerCertificate: true
    },
  }
})
class File extends Model { }

File.init({
  uuid: {
    allowNull: false,
    autoIncrement: false,
    primaryKey: true,
    type: DataTypes.UUID
  },
  path: {
    type: 'VARCHAR(500)'
  }
}, {
  sequelize,
  freezeTableName: true
});
(async () => {
  await sequelize.sync({ force: true });
  // Code here
})();

module.exports = File

and i am using this model as

const Op = Sequelize.Op;
const file = require('./models/file')
file.findOne({
    where:{
        path:{
            [Op.like]:'%-1001_1101.xml'
        }
    }
}).then((result) => {
    console.log(JSON.stringify(result))
}).catch((e) => {
    console.log(e)
})

This is producing following query

SELECT [uuid], [path], [createdAt], [updatedAt] FROM [File] AS [File] WHERE [File].[path] LIKE N'%-1001_1101.xml' ORDER BY [File].[uuid] OFFSET 0 ROWS FETCH NEXT 1 ROWS ONLY

The issue is with the N char after like in query. How to fix this?

@ephys
Copy link
Member

ephys commented Apr 12, 2022

@nareshy-quisitive That part is tracked over here #9107 but we won't be able to do that until we have a proper non-n varchar type, which I opened a RFC for here #14259

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4: nice to have For issues that are not bugs. type: performance For issues and PRs. Things that affect the performance of Sequelize.
Projects
None yet
Development

No branches or pull requests

10 participants