Skip to content

Conversation

@jakubskrz
Copy link
Contributor

Since MS SQL 2012 there is equivalent to LIMIT OFFSET clause in T-SQL.

But it requires to use ORDER BY clause in query, see official docs

To consider:

  • add default ORDER BY instead of exception
  • throw NotSupportedException when connected to older MSSQL Server version

@milo
Copy link
Member

milo commented Jun 12, 2015

What about older MSSQL? Sqlsrv driver supports MSSQL since version 2005.

@jakubskrz
Copy link
Contributor Author

On 2005 and 2008 versions this SQL syntax cause an error, so it would be good solution to check database version, like this:

$info = $this->connection->query('SELECT SERVERPROPERTY(\'ProductVersion\') AS version')->fetch();

if ($info->version < 11) { // 11 == SQL Server 2012
    throw new Nette\NotSupportedException('Offset is not supported by this database.');
}

But that doesn't work for me, I'm getting NULL.

@jakubskrz
Copy link
Contributor Author

Hm, OK, when selecting server version via sql query, it's needed to be manually cast to string.

So this one 43a7fc1 will preserve current behaviour on older MS SQL versions.

@dg dg force-pushed the master branch 4 times, most recently from 0fc7ac2 to 7194522 Compare June 21, 2015 16:17
Copy link
Contributor

Choose a reason for hiding this comment

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

How about checking version only once, like this:

    public function applyLimit(& $sql, $limit, $offset)
    {
        static $ver = null;
        if (is_null($ver)) {
            $info = $this->connection->query("SELECT CAST(SERVERPROPERTY('ProductVersion') AS varchar) AS [ver]")->fetch();
            $ver = (int) $info->ver;
        }
        if ($ver < 11) { // 11 == SQL Server 2012

I'd really like to see this merged. Perhaps squashing would help?

Copy link
Member

Choose a reason for hiding this comment

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

Is possible to use $connection->getPdo()->getAttribute(\PDO::ATTR_SERVER_VERSION) like in MySqlDriver?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it is.

    /** @var string */
    private $version;

    public function __construct(Nette\Database\Connection $connection, array $options)
    {
        $this->connection = $connection;
        $this->version = $connection->getPdo()->getAttribute(\PDO::ATTR_SERVER_VERSION);
    }
...
    public function applyLimit(& $sql, $limit, $offset)
    {
        if (version_compare($this->version, 11, '<')) { // < SQL Server 2012

@meridius
Copy link
Contributor

@jakubskrz Could you please incorporate all the mentioned changes and squash all to one commit so it could be merged?

@jakubskrz
Copy link
Contributor Author

Hi, sorry for delay, I hadn't much time last week.

I've updated my PR with suggested changes and also with changes from this commit b3b6e9b so there is no merge conflict with current head. Looking at #99, I've also removed checking for ORDER BY clause.

But I can't test it right now, as I can't access my dev SQL server (broken VPN :/).

@dg
Copy link
Member

dg commented Sep 21, 2015

Merged, thx 15ccd8b

@dg dg closed this Sep 21, 2015
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.

4 participants