Refactor Query Generator Limit/Offset

[mkaufmaner]

What are you doing?

Currently the following code in the abstract query generator should be refactored to conform to eslint rules. However, this brings up the question about the implementation and limitations of limit and offset. I dislike the arbitrary implementation of a static count when the offset is passed without a limit. What are the valid values between the support dialects and their versions?

    /* eslint-disable */
    if (options.offset != null && options.limit == null) {
      fragment += ' LIMIT ' + this.escape(options.offset) + ', ' + 10000000000000;
    } else if (options.limit != null) {
      if (options.offset != null) {
        fragment += ' LIMIT ' + this.escape(options.offset) + ', ' + this.escape(options.limit);
      } else {
        fragment += ' LIMIT ' + this.escape(options.limit);
      }
    }
    /* eslint-enable */

Related:

• feat(query): bind parameters #10284 (comment)
• fix limit=0 issue #5632
• Limit = 0 (zero) #3914

Dialect: any
Dialect version: any
Database version: any
Sequelize version: 5
Tested with latest release: N/A

[javiertury]

In a another issue I argued against limit = 0. I was wrong, all dialects interpret limit = 0 uniformly and return 0 rows. Looking at related issues, some users need the limit = 0 feature. You proposed options.limit !== null && options.limit !== undefined and I think it is adequate.

Perhaps documentation in Pagination / Limiting should explain the difference between limit = 0 and limit = null.

Regarding offset, I think we can safely use if (options.offset). Offset indicates the number of rows to skip, and offset = 0 is equivalent to omitting the offset clause (as mentioned in some docs).

Docs: postgres, mssql, sqlite and mysql