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

feat(mariadb): implementation for MariaDB #10192

Merged
merged 14 commits into from Dec 3, 2018
Merged

Conversation

@rusher
Copy link
Contributor

@rusher rusher commented Nov 22, 2018

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Implementation of MariaDB.
This differs from MySQL mostly for 3 things:

  • the driver is different (mariadb
  • use of schema
  • MariaDB JSON implementation differ from MySQL

Breaking change is the use of schemas. MySQL/MariaDB have schemas, but no "default" public schema.
Current implementation for MySQL will create a table with name mySchema_myTable, using schema, that is mySchema.myTable.
I haven't corrected that for MySQL since it will break every existing application, but for new MariaDB implementation, it seems appropriate to use a better base with real schemas.

Tests have been updated accordingly (defaut method that drop all schemas is replace to keep default schema that is needed for MariaDB)

This differ from MySQL mostly for 3 things:
- driver is different (mariadb)
- use of schema
- MariaDB JSON implementation differ from MySQL
Copy link
Member

@SimonSchick SimonSchick left a comment

Bunch of nits, looks good over all.

lib/dialects/abstract/query-generator.js Outdated Show resolved Hide resolved
lib/dialects/mariadb/connection-manager.js Outdated Show resolved Hide resolved
lib/dialects/mariadb/connection-manager.js Outdated Show resolved Hide resolved
lib/dialects/mariadb/query-generator.js Outdated Show resolved Hide resolved
test/integration/dialects/mariadb/dao.test.js Outdated Show resolved Hide resolved
test/integration/query-interface.test.js Show resolved Hide resolved
test/support.js Outdated Show resolved Hide resolved
@codecov
Copy link

@codecov codecov bot commented Nov 26, 2018

Codecov Report

Merging #10192 into master will decrease coverage by 0.06%.
The diff coverage is 92.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10192      +/-   ##
==========================================
- Coverage   96.34%   96.28%   -0.07%     
==========================================
  Files          63       68       +5     
  Lines        9420     9796     +376     
==========================================
+ Hits         9076     9432     +356     
- Misses        344      364      +20
Impacted Files Coverage Δ
...dialects/abstract/query-generator/helpers/quote.js 100% <ø> (+5.55%) ⬆️
lib/dialects/abstract/connection-manager.js 90.26% <100%> (+0.26%) ⬆️
lib/model.js 96.77% <100%> (ø) ⬆️
lib/dialects/mariadb/index.js 100% <100%> (ø)
lib/utils.js 98.27% <100%> (ø) ⬆️
lib/dialects/mysql/query-generator.js 97.9% <100%> (+0.01%) ⬆️
lib/data-types.js 94.94% <100%> (+0.01%) ⬆️
lib/dialects/mariadb/connection-manager.js 100% <100%> (ø)
lib/query-interface.js 91.95% <100%> (+2.38%) ⬆️
lib/dialects/mysql/query-interface.js 100% <100%> (ø) ⬆️
... and 13 more

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 f2cafcc...7fe7cd1. Read the comment docs.

Correcting padding, simplify implementation according to remarks
Use Set when possible, benchmarks showing better performance
Copy link
Member

@SimonSchick SimonSchick left a comment

Looks better now, some minor nits.

lib/dialects/mariadb/query.js Outdated Show resolved Hide resolved
lib/dialects/mariadb/query.js Outdated Show resolved Hide resolved
lib/dialects/mariadb/query.js Show resolved Hide resolved
Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

A few comments for now

lib/dialects/abstract/query-generator.js Outdated Show resolved Hide resolved
lib/dialects/mariadb/query-generator.js Outdated Show resolved Hide resolved
lib/dialects/mariadb/query-interface.js Outdated Show resolved Hide resolved
@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Nov 28, 2018

This looks almost ready @rusher , Please extend documentation so it references mariadb

I'll do some testing locally in the evening, we can hopefully merge this tomorrow

@rusher
Copy link
Contributor Author

@rusher rusher commented Nov 28, 2018

@sushantdhiman Just tell me if you think anything else is missing. I don't see any other improvement that i can do, but better bulk implementation (mariadb connector has a dedicated bath API that use a MariaDB specific protocol, but that would be another PR)

.travis.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
changing travis docker
lib/dialects/mariadb/query-generator.js Outdated Show resolved Hide resolved
let startWithDot = true;

// Convert .number. to [number].
path = path.replace(/\.(\d+)\./g, '[$1].');

This comment has been minimized.

@SimonSchick

SimonSchick Nov 29, 2018
Member

I think you only need to bottom one?

This comment has been minimized.

@rusher

rusher Nov 30, 2018
Author Contributor

it depends if you want to support JSON dot notation or not.

SET @json = '{"tt": [1, 2, [3, 4]], "tt2" : 3}';
SELECT JSON_EXTRACT(@json, '$.tt.2'); // return null;
SELECT JSON_EXTRACT(@json, '$.tt[2]'); // return [3,4];
SELECT JSON_EXTRACT(@json, '$.tt[2].1'); // return null;
SELECT JSON_EXTRACT(@json, '$.tt[2][1]'); // return 4;

so supporting something like whereItemQuery(Sequelize.json('profile.id.0.1'), '1')
profile.id.0.1 needs to be changed to json_unquote(json_extract(profile,'$.id[0][1]')) = '1'

lib/dialects/mariadb/query-generator.js Outdated Show resolved Hide resolved
resolve(results);
})
.catch(err => {
debug(

This comment has been minimized.

@SimonSchick

SimonSchick Nov 29, 2018
Member

Is it really useful to debug log and regular log?

This comment has been minimized.

@rusher

rusher Nov 30, 2018
Author Contributor

hmm, a better thing may be

debug( `executed(${this.connection.uuid || 'default'}) with error : ${this.sql}\n${err}`);

This comment has been minimized.

@sushantdhiman

sushantdhiman Dec 1, 2018
Contributor

No, keep this format

rusher added 4 commits Nov 30, 2018
improve comparison removing useless regex
Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

LGTM @rusher Let me know if there is any thing pending. Will merge this on Monday

@rusher
Copy link
Contributor Author

@rusher rusher commented Dec 3, 2018

@sushantdhiman No other changes unless new reviews

@sushantdhiman sushantdhiman merged commit c6d7333 into sequelize:master Dec 3, 2018
2 of 4 checks passed
2 of 4 checks passed
codecov/patch 92.97% of diff hit (target 96.34%)
Details
codecov/project 96.28% (-0.07%) compared to f2cafcc
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Dec 3, 2018

Thanks @rusher 👍 (& @SimonSchick for additonal reviews) :)

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

Successfully merging this pull request may close these issues.

None yet

3 participants