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 driver to support for Sql Server #60

Closed
wants to merge 8 commits into from

Conversation

tjodalv
Copy link

@tjodalv tjodalv commented Mar 18, 2024

I've created driver for the SQL server and updated readme file to document driver support.

@tjodalv tjodalv mentioned this pull request Mar 18, 2024
@tjodalv
Copy link
Author

tjodalv commented Mar 18, 2024

I just notice that there is another pull request that adds support for SQL server. Still, I think this one is better because this one is using CONVERT SQLserver function to format date column and the other one is using FORMAT function which is slower and locale aware. Also the other pull request unnecessary refactor code in Trend service IMO.

@ryanmortier
Copy link

ryanmortier commented Mar 19, 2024

I don't think this is working?

I tried your fork today and this is the SQL that was generated:

SELECT
  CONVERT(VARCHAR, created_at, 23) AS date,
  count(*) AS aggregate
FROM
  [ crm ].[ account_notes ]
WHERE
  [ created_at ] BETWEEN 2024 -03 -12 01: 59: 02.687
  AND 2024 -03 -19 01: 59: 02.687
GROUP BY
  [ CONVERT(VARCHAR, created_at, 23) ]
ORDER BY
  [ date ] ASC

Query 1 ERROR at Line 1: : Msg: 102, Line 7, State: 1, Level: 15
Incorrect syntax near '01'.

Manually fixed the query to be working:

SELECT
  CONVERT(VARCHAR, [created_at], 23) AS date,
  count(*) AS aggregate
FROM
  [crm].[account_notes]
WHERE
  [created_at] BETWEEN '2024-03-12 01:59:02.687'
  AND '2024-03-19 01:59:02.687'
GROUP BY
  CONVERT(VARCHAR, [created_at], 23)
ORDER BY
  [date] ASC

I removed spaces from inside the brackets, fixed the date strings, and removed the brackets from around the group by clause.

@tjodalv
Copy link
Author

tjodalv commented Mar 19, 2024

I've fixed the problem with groupBy clause when using SQL server driver and tested it against SQL Server 2022. It should work fine now.

@ryanmortier
Copy link

ryanmortier commented Mar 19, 2024

Working now, thanks!

SELECT
	CONVERT(VARCHAR, created_at, 23) AS date,
	count(*) AS aggregate
FROM
	[crm].[tickets]
WHERE
	[created_at] BETWEEN '2024-03-12 12:29:28.806'
	AND '2024-03-19 12:29:28.806'
GROUP BY
	CONVERT(VARCHAR, created_at, 23)
ORDER BY
	[date] ASC

Is there a way for you to wrap the column name in brackets in case it's a reserved word?

@tjodalv
Copy link
Author

tjodalv commented Mar 19, 2024

I've push latest commit that encloses date column name in square brackets.

@ryanmortier
Copy link

I've push latest commit that encloses date column name in square brackets.

Tested and working great, thanks!

SELECT
	LEFT(CONVERT(VARCHAR, [add_date], 23), 7) AS date,
	sum(scale_quantity) AS aggregate
FROM
	[crm].[tickets]
WHERE
	[commodity_id] = 'corn'
	AND [add_date] BETWEEN '2023-03-19 20:35:42.983'
	AND '2024-03-19 20:35:42.983'
GROUP BY
	LEFT(CONVERT(VARCHAR, [add_date], 23), 7)
ORDER BY
	[date] ASC

@Larsklopstra can we have this reviewed and merged whenever you have a chance?

@ryanmortier
Copy link

ryanmortier commented Mar 22, 2024

I got looking at this again today and I started wondering if this could potentially be vulnerable to SQL injection if the developer for some reason accepted input for the ->dateColumn() method?

@Larsklopstra
Copy link
Member

Sorry, I won't be support Sql Server OOTB. The new extensible version will allow you to add your own drivers

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.

3 participants