MS SQL syntax for LIMIT/OFFSET

[Sylvestre67]

Resolves https://github.com/observablehq/projects/issues/269.

SQL server syntax for LIMIT/OFFSET type query is

ORDER BY <col> <direction>
OFFSET 0 ROWS
FETCH NEXT 1000 ROWS ONLY

The table cell is currently generating the standard SQL syntax only.

LIMIT 1000
OFFSET 0

If the source is of dialect mssql it should use the right LIMIT/OFFSET syntax.

MS SQL documentation.

[mbostock]

Do you mind running Prettier on the parts you’ve changed? I see some formatting nits here and I’d rather not comment on them. Thanks!

[mbostock]

I think we’ll want the escaped column name here, to match the SELECT.

Suggested change

	appendOrderBy({column: select.columns[0], direction: 'ASC'}, args);
	appendOrderBy({column: columns[0], direction: 'ASC'}, args);

Also, this will crash if select.columns is null, as it is with SELECT *. In practice this should never happen for a MS SQL data source because we would always materialize the selected columns in a table cell, but we should either throw an error here with a human-readable message or perhaps we can make it work with something like ORDER BY NULL.

[Sylvestre67]

Seems like ORDER BY NULL won't work, mssql is not happy with it. So I added a catch and throwing an error with a human-readable message as you suggested.

Regarding the escaping, it seems that columns[0] has the alias of the table (t) prepended to it (L245). The table alias is also added in the appendOrderBy method (L286), so we end up with something like ORDER BY t.t."ModifiedDate" ASC. I decided to apply the escaper to the select.columns[0].

Also after looking at it for the other operations (sort and filter) I realized these are not escaped either. So I added the escape logic for both of these as well + some test.

[Sylvestre67]

Do you mind running Prettier on the parts you’ve changed? I see some formatting nits here and I’d rather not comment on them. Thanks!

I applied the styling. prettier wants to slice any line longer than the default printWidth value of 80 char . So I ignored these changes. Hopefully this is good now.

Should we change the printWidth in the .prettierrc to something like 120 ?

[mbostock]

Good find on the need for more escaping! Thanks for adding tests, too. 🙏

[mbostock]

Sent #314 for your consideration. 🙏

[Sylvestre67]

Sent #314 for your consideration. 🙏

Merged. Thanks a lot !

[mbostock]

Almost there…

[mbostock]

👍👍