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

Added option to pass indexType for MySQL dialect #2890

Merged
merged 3 commits into from Nov 10, 2018

Conversation

@douglasjunior
Copy link
Contributor

@douglasjunior douglasjunior commented Nov 5, 2018

I followed the MySQL Reference.

CREATE [UNIQUE | FULLTEXT | SPATIAL] INDEX index_name
    [index_type]
    ON tbl_name (key_part,...)
    [index_option]
    [algorithm_option | lock_option] ...

key_part: {col_name [(length)] | (expr)} [ASC | DESC]

index_option:
    KEY_BLOCK_SIZE [=] value
  | index_type
  | WITH PARSER parser_name
  | COMMENT 'string'
  | {VISIBLE | INVISIBLE}

index_type:
    USING {BTREE | HASH}

algorithm_option:
    ALGORITHM [=] {DEFAULT | INPLACE | COPY}

lock_option:
    LOCK [=] {DEFAULT | NONE | SHARED | EXCLUSIVE}

No changes are required in types/knex.d.ts.

I'm not sure if I need to write tests for this, I could not find where the MySQL schema tests are written.

Steps to reproduce:

  1. Add my fork as dependency in package.json:
"knex": "git+https://github.com/douglasjunior/knex.git#custom"
  1. Create a migration file:
exports.up = (knex, Promise) => {
    return  knex.schema.createTable('my_table', table => {
        table.bigIncrements('id');

        table.string('my_long_text_column', 1000).notNullable();

        table.index('my_long_text_column', null, 'FULLTEXT'); // <= here
    });
};

exports.down = (knex, Promise) => {
    return knex.schema.dropTableIfExists('my_table');
};
  1. Run npx knex migrate:latest.

  2. See the generated schema:

CREATE TABLE `my_table` (
  `id` bigint(20) unsigned NOT NULL AUTO_INCREMENT,
  `my_long_text_column` varchar(1000) NOT NULL,
  PRIMARY KEY (`id`),
  FULLTEXT KEY `my_table_my_long_text_column_index` (`nome_original`)
) 

Related to #1840

@@ -194,14 +194,13 @@ assign(TableCompiler_MySQL.prototype, {
})
);
},
index(columns, indexName) {
index(columns, indexName, indexType) {
Copy link
Collaborator

@kibertoad kibertoad Nov 5, 2018

can you also add test for this? probably just a unit test for query generation would suffice

Copy link
Collaborator

@kibertoad kibertoad Nov 5, 2018

and there probably are existing tests for pg already

Copy link
Contributor Author

@douglasjunior douglasjunior Nov 5, 2018

The format of the tests is confusing to me. Many tests are failing and I do not know why. https://travis-ci.org/tgriesser/knex/jobs/450867352 (The tests fail even before the change)

I'll look at the postgre tests to see if I can understand.

Copy link
Contributor Author

@douglasjunior douglasjunior Nov 5, 2018

Done.

Copy link
Collaborator

@kibertoad kibertoad Nov 5, 2018

    it('test adding index', function() {
      tableSql = client
        .schemaBuilder()
        .table('users', function() {
          this.index(['foo', 'bar'], 'baz');
        })
        .toSQL();

      equal(1, tableSql.length);
      expect(tableSql[0].sql).to.equal(
        'alter table `users` add index `baz`(`foo`, `bar`)'
      );
    });

This is an example of a test failing. I assume structure of an index statement changed even when index type is not specified, which is actually probably not something that we want - maybe excessive space somewhere?..

Copy link
Collaborator

@kibertoad kibertoad Nov 5, 2018

@douglasjunior Feel free to ignore Oracle tests failing, they've been like that for a while already. But MySQL ones should be passing.

Copy link
Contributor Author

@douglasjunior douglasjunior Nov 5, 2018

Now I think I get it, please, could you check it again?

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Nov 5, 2018

@douglasjunior Thank you very much! Would you consider creating PR for https://github.com/knex/documentation to mention MySQL support for .index()?

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Nov 5, 2018

@tgriesser LGTM, what do you think?

@douglasjunior
Copy link
Contributor Author

@douglasjunior douglasjunior commented Nov 5, 2018

@douglasjunior Thank you very much! Would you consider creating PR for https://github.com/knex/documentation to mention MySQL support for .index()?

Sure.

@douglasjunior
Copy link
Contributor Author

@douglasjunior douglasjunior commented Nov 5, 2018

@kibertoad kibertoad added the ready label Nov 6, 2018
@kibertoad kibertoad merged commit 931c157 into knex:master Nov 10, 2018
2 checks passed
mwilliammyers added a commit to voxjar/knex that referenced this issue Dec 12, 2018
* Added option to pass indexType for MySQL dialect.

* Added test for: test adding index with type

* Removing extra space when no type is provided.
@glensc
Copy link

@glensc glensc commented Aug 23, 2021

Filed a request for actual index_type:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants