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(dialect): snowflake dialect support #13406
feat(dialect): snowflake dialect support #13406
Conversation
Pipeline is green.... Let me know if any other actions needed...thx @Keimeno |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't looked at everything, but here's some feedback for now.
Edit: also - it looks like you copied and pasted the query generator from mysql. A lot of the functions seem untouched, maybe you should just extend the mysql query generator instead of the abstract one.
@allawesome497 Thx! Yes it is fork from mySQL Dialect and most of use cases we are using are adopted (there are some practices more close to pgsql, not mysql) and tested in snowflake integration test.... I will fix most of the missing name changing and ping u back. Also regarding to Thx again! |
@allawesome497 All comments addressed. Could u please review it again and let me know anything still needs update? Thx! Regarding the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still haven't looked through everything, but here's some more feedback.
Ideally this would be broken up into multiple prs - e.g. connection manager
-> queryGenerator
-> queryInterface
.
Thx will try to catch it up today or tomorow. Regarding to the PR split, it might be good to promote something working (with the real server) as 1st commit...sorry for the heavy load of review. I could take responsibility for adding more cases and issue fixes. |
Agreed. There should probably be a process for adding dialects added to the contributing docs. Ideally this would be broken up into multiple prs but since there isn't any established guidelines for this it's probably fine as is. |
11702c0
to
ce72c6b
Compare
@allawesome497 All addressed - thanks for the review! Let me know anything needs to be update and fixed. |
191c708
to
56a0957
Compare
56a0957
to
1109f58
Compare
@Keimeno @allawesome497 Hi could we have another review and proceed this PR? Thanks... |
Haven't taken the time to look at sequelize prs since there isn't any active core maintainers- so even if its approved, I don't see it getting merged any time soon. I'll let you know if the situation changes. |
c585fe3
to
7fec7cc
Compare
7fec7cc
to
1ef2c36
Compare
@jesse23 Could you please ping me on slack? |
We are going to have our monthly sequelize sync tomorrow and will discuss the matter of how to deal with dialect support requests there: https://www.meetup.com/eBay-Europe-Technology/events/282125063/ Feel free to drop by and participate in the discussion. |
Thanks - yes will attend. FYI @cincyelite22 |
* | ||
* @private | ||
*/ | ||
const snowflakeReservedWords = 'account,all,alter,and,any,as,between,by,case,cast,check,column,connect,connections,constraint,create,cross,current,current_date,current_time,current_timestamp,current_user,database,delete,distinct,drop,else,exists,false,following,for,from,full,grant,group,gscluster,having,ilike,in,increment,inner,insert,intersect,into,is,issue,join,lateral,left,like,localtime,localtimestamp,minus,natural,not,null,of,on,or,order,organization,qualify,regexp,revoke,right,rlike,row,rows,sample,schema,select,set,some,start,table,tablesample,then,to,trigger,true,try_cast,union,unique,update,using,values,view,when,whenever,where,with'.split(','); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be really cool if this could be moved over to the snowflake specific dialect files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdepold This is currently following the same practice as what postgres is doing for reserved words. Do you have a recommendation on where this can neatly be injected without refactoring all of the calls to this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sdepold I will follow up in the dialect separation work with postgres together.
@cincyelite22 Please follow up. Thx. |
.github/workflows/ci.yml
Outdated
strategy: | ||
fail-fast: false | ||
matrix: | ||
node-version: [10, 12] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change the highest tested node version from 12 to 16, we recently changed this
* fix(dialect): fix Snowflake query syntax * fix(dialect): fix offset-limit test * fix(dialect): fix offset-limit test Co-authored-by: Maria Lee <maria.lee@dynatrace.com>
69210c9
to
7eb548a
Compare
Docs seems to be broken now. But we are getting closer :) |
@sdepold Should be ready now for another review. |
I think you'll have to update with master one more time |
* feat(typescript): create alpha release with ts * ci: add other ts versions to ci (#13686) * fix: wrong interface used within mixin (#13685) * fix(increment): fix key value broken query (#12985) Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com> * fix(upsert): fall back to DO NOTHING if no update key values provided (#13594) * fix(types): add instance member declaration (#13684) * fix(types): add instance member declaration * test(types): add static/instance members test cases Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com> * Create aws-lambda.md (#12642) * fix(docs): add aws-lamda route (#13693) * fix(types): allow override json function with custom return type (#13694) * fix(types): allow override to json function with custom return type * fix(types): remove automatic linter changes Co-authored-by: sander-mol <SMol@thepeoplegroup.nl> * ci(docs): add doc generation to checks (#13704) * docs: add logo (#13700) * docs: add logo * fix(docs): logo not show up (#13699) * fix(build): markdownlint * docs(readme): use internal link * docs(index.md): use internal link * docs(index): update logo rendering in docs * Center logo and headline Co-authored-by: Sascha Depold <sascha@depold.com> Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com> * test: fix mocha (#13707) Co-authored-by: Rik Smale <WikiRik@users.noreply.github.com> Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com> * test: fix failing stack trace test (#13708) * test: fix failing tests (#13709) * test: fix failing tests due to minification Removes esbuild minification which was causing tests to fail and some changed behavior. * Revert "fix(upsert): fall back to DO NOTHING if no update key values provided (#13594)" This reverts commit 4071378. * fix(upsert): fall back to DO NOTHING if no update key values provided (#13711) * fix(upsert): fall back to DO NOTHING if no update key values provided (#13594) * fix: remove erroneous .length in _.isEmpty * refactor: use includes instead of or operators (#13706) * docs(contributing): add section on adding/updating deps (#13715) * docs(contributing): add Node versions Fixes #13714 * docs(contributing): add section on adding/updating deps * fix(types): add Col to where Ops (#13717) * fix(types): add Col to where Ops * fix(types): tests * fix(data-types): unnecessary warning when getting data with DATE dataTypes (#13712) * fix(data-types): unnecessary error when getting data with DATE dataTypes * fix(data-types): date stringify mariadb * fix(types): add missing schema field to sequelize options Fixes #12606 Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com> * fix(example): fix coordinates format as per GeoJson (#13718) * fix(example): fix coordinates format as per GeoJson * Update data-types.js Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com> * ci(node): use Node 16 instead of 12 (#13703) * ci(node): add Node 14 and 16 to DB tests * ci(node): move linting, test typing and release to Node 16 * ci(docs): use Node 16 for docs * ci(node): run tests on Node 10 and 16 Co-authored-by: Rik Smale <WikiRik@users.noreply.github.com> * refactor(postgres): move `clientMinMessages` from general to pg options (#13720) * refactor(postgres): move `clientMinMessages` from general to pg options * refactor(postgres): address review comments * refactor(postgres): address review comments * refactor(postgres): fix pipeline * fix(dialect): try to fix flaky test * fix(dialect): try to fix flaky test Co-authored-by: Jesse Peng <jesse.peng@dynatrace.com> * refactor(build): use rm instead of rmdir for Node 14 and up (#13702) * fix(build): refactor rmdir to rm * refactor(build): only use rm in Node 14 and up * refactor(build): move consts inside function * refactor(build): fix definition Co-authored-by: Rik Smale <WikiRik@users.noreply.github.com> Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com> * docs(postgres): warn about deprecated clientMinMessage option (#13727) * docs(postgres): warn about deprecated clientMinMessage option * docs(deprecation-warning): fix typo in deprecation warning * docs(deprecation-warning): fix typo in deprecation warning * meta(deps): upgrade (dev)deps (#13729) Co-authored-by: Rik Smale <WikiRik@users.noreply.github.com> * test(integration): mark and fix flaky test (#13735) * feat(dialect): snowflake dialect support (#13406) Co-authored-by: Mohamed El Mahallawy <mmahalwy@gmail.com> Co-authored-by: bparan <bparan@softserveinc.com> Co-authored-by: Matthew Blasius <matthew.blasius@expel.io> Co-authored-by: WeRDyin <heyin223@gmail.com> Co-authored-by: Marco Gonzalez <marcogrcr@gmail.com> Co-authored-by: Constantin Metz <constantin@metzworld.com> Co-authored-by: Sander Mol <Sandermol95@hotmail.com> Co-authored-by: sander-mol <SMol@thepeoplegroup.nl> Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com> Co-authored-by: Fauzan <fncolon@pm.me> Co-authored-by: Rik Smale <WikiRik@users.noreply.github.com> Co-authored-by: AllAwesome497 <47748690+AllAwesome497@users.noreply.github.com> Co-authored-by: manish kakoti <madguy02@users.noreply.github.com> Co-authored-by: Marco Kerwitz <marco@kerwitz.com> Co-authored-by: Jesse Peng <vijcp@outlook.com> Co-authored-by: Jesse Peng <jesse.peng@dynatrace.com>
🥳 |
🎉 This PR is included in version 6.12.0-beta.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 6.12.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Pull Request check-list
npm run test
ornpm run test-DIALECT
pass with this change (including linting)?Description of change
snowflake dialect support.
Note
How to run integration test
Link with existing defect:
#11979