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

Clean up sql server meta data #246

Closed

Conversation

RachelAmbler
Copy link
Contributor

Fixes #243 (now even passes testing!)

@wokket
Copy link
Collaborator

wokket commented Sep 14, 2022

Thanks for looking into this mate, some clean up is welcome!

I do have a couple of questions/concerns that I'll raise here without really knowing whether they're showstoppers from @erikbra 's point of PoV or not.

  • By moving to 'modern' field types like datetime2 we are losing support for old versions of Sql Server (2005?). MS clearly don't support that any more, and I wouldn't want to make changes just to support version that old, but I'm not sure we should be deliberately breaking support either. That said grate was meant to be the 'modern' RoundhousE....
  • By excluding (say) read-only databases you've highlighted an existing bug (we try to write to a read-only database) and it's changed to a different bug (we try and create a duplicate of the read-only database). What was the scenario you hit that brought about this change?

Sorry it's taking some time to review all this stuff, time is precious for me atm :(

@RachelAmbler
Copy link
Contributor Author

RachelAmbler commented Sep 15, 2022

  1. Personally I'd say that 2005 is too old to be supported - it's not unreasonable to say that 1.40 is the last version to support 2005.
  2. It didn't make sense to me to try and consider read-only databases. Which issue is this other bug in - I could take a look at it and see if I can fix that as well.

I'm on a bit of a tear - having settled on grate for our new deployment process I'm adding in functionality that grate doesn't have that I need. First off with the Dependency management (I raised a discussion on that - I also Cc'd you on it as well so you can chime in). Next up is having grate able to handle executables as well as Sql Scripts.

Right now I'm going to be deploying on SQL Server, but after the new year I may well find myself needing to target Greenplum (a PostgreSql variant) - and I need both these two new features I'm working on. It's up to y'all if you're interested in consuming my upstream changes!

@wokket
Copy link
Collaborator

wokket commented Sep 15, 2022

Sorry there's no existing bug logged, it's not something I'd considered either until I was was thinking about the impacts of your change.

@RachelAmbler
Copy link
Contributor Author

@wokket So the issue is just ensuring we don't try to recreate an existing database?

If so, then that should be an easy fix. Let me take a butcher's at that in the AM tomorrow and see if I can clean that up with a pre-check.

If I nail that, how you feel about the PR then (@erikbra aside)?

public string TableWithSchema(string schemaName, string tableName) => $"[{schemaName}].{tableName}";
public string ReturnId => ";Select Scope_Identity()";
public string TimestampType => "DateTime2(0)";
Copy link
Owner

@erikbra erikbra Sep 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer more than one-second precision here, is there any reason not to use Datetime2(7) ?

@erikbra
Copy link
Owner

erikbra commented Sep 17, 2022

Thanks a lot for you contribution, @RachelAmbler ! I appreciate more people becoming active in the development of grate!!

I don't have any problems replacing ntext with nvarchar(max). I know ntext is obsoleted, even if it still works. But, maybe better to change it, that't the point of obsoletion, isn't it, to give people (us) time to move away from it :)

I have a small question regarding the change in casing on the SQL server commands. Is there any special reason you have changed the keywords from CAPITAL to PascalCase? I tried to find some style guides for T-SQL here, but I couldn't find any by googling. Are you aware of any? Technically, it doesn't matter. I just wonder if there was a particular reason for the change.

Good catch on the " instead of the brackets around reserved names. The former required QUOTED_IDENTIFIER` to be set to ON if it should work, I hadn't seen that one.

@erikbra
Copy link
Owner

erikbra commented Apr 18, 2024

This has been laying here a long time, and not been merged. The source code is restructured quite a lot since this PR, and #501 solved much of this, so I'll close this for now. Please open new issues if there are things left that needs updating/fixing, @RachelAmbler

@erikbra erikbra closed this Apr 18, 2024
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.

Various updates to SqlServerSyntax are needed
3 participants