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.

Sign up for GitHub

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

OFFSET should be allowed to be 0 #1937

Merged
merged 3 commits into from
Mar 13, 2015
Merged

OFFSET should be allowed to be 0 #1937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eda74a2
allow offset 0
corylanou Mar 13, 2015
bdfb9bf
update changelog
corylanou Mar 13, 2015
08b15d9
zeros are ok after all
corylanou Mar 13, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- [#1930](https://github.com/influxdb/influxdb/pull/1930): Auto create database for graphite if not specified.
- [#1908](https://github.com/influxdb/influxdb/pull/1908): Cosmetic CLI output fixes.
- [#1931](https://github.com/influxdb/influxdb/pull/1931): Add default column to SHOW RETENTION POLICIES.
- [#1937](https://github.com/influxdb/influxdb/pull/1937): OFFSET should be allowed to be 0.

### Features
- [#1902](https://github.com/influxdb/influxdb/pull/1902): Enforce retention policies to have a minimum duration.
Expand Down
4 changes: 2 additions & 2 deletions influxql/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1435,8 +1435,8 @@ func (p *Parser) parseOptionalTokenAndInt(t Token) (int, error) {
// Parse number.
n, _ := strconv.ParseInt(lit, 10, 64)

if n < 1 {
msg := fmt.Sprintf("%s must be > 0", t.String())
if n < 0 {
Copy link
Member

Choose a reason for hiding this comment

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

any reason to validate this as all? Wouldn't it be better to just look at the error from ParseInt? I'm guessing that's why it wasn't allowing zero before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you get an error like this?

> show series limit -1
ERR: error parsing query: LIMIT must be > 0 at line 1, char 19

Copy link
Contributor

Choose a reason for hiding this comment

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

That was a deliberate design decision we came to a while back (search Slack).

If my recollection is correct we decided that LIMIT 0 didn't make much sense, since it should be interpreted literally as "give me no records". SQLite, MySQL (and I believe Postgres) all return 0 records if you ask for LIMIT 0. Since there was still arguments about this we simply prohibited LIMIT 0 in code. Obviously OFFSET 0 is a different issue, but OFFSET did not exist at the time of the discussions.

Copy link
Member

Choose a reason for hiding this comment

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

I was just thinking because it was parseOptionalTokenAndInt, not necessarily LIMIT. But sure, this makes sense. Merging it!

msg := fmt.Sprintf("%s must be >= 0", t.String())
return 0, &ParseError{Message: msg, Pos: pos}
}

Expand Down
14 changes: 12 additions & 2 deletions influxql/parser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,18 @@ func TestParser_ParseStatement(t *testing.T) {
stmt: &influxql.ShowSeriesStatement{},
},

// SHOW SERIES with OFFSET 0
{
s: `SHOW SERIES OFFSET 0`,
stmt: &influxql.ShowSeriesStatement{Offset: 0},
},

// SHOW SERIES with LIMIT 2 OFFSET 0
{
s: `SHOW SERIES LIMIT 2 OFFSET 0`,
stmt: &influxql.ShowSeriesStatement{Offset: 0, Limit: 2},
},

// SHOW SERIES WHERE with ORDER BY and LIMIT
{
s: `SHOW SERIES WHERE region = 'uswest' ORDER BY ASC, field1, field2 DESC LIMIT 10`,
Expand Down Expand Up @@ -766,10 +778,8 @@ func TestParser_ParseStatement(t *testing.T) {
{s: `SELECT field1 FROM myseries GROUP`, err: `found EOF, expected BY at line 1, char 35`},
{s: `SELECT field1 FROM myseries LIMIT`, err: `found EOF, expected number at line 1, char 35`},
{s: `SELECT field1 FROM myseries LIMIT 10.5`, err: `fractional parts not allowed in LIMIT at line 1, char 35`},
{s: `SELECT field1 FROM myseries LIMIT 0`, err: `LIMIT must be > 0 at line 1, char 35`},
{s: `SELECT field1 FROM myseries OFFSET`, err: `found EOF, expected number at line 1, char 36`},
{s: `SELECT field1 FROM myseries OFFSET 10.5`, err: `fractional parts not allowed in OFFSET at line 1, char 36`},
{s: `SELECT field1 FROM myseries OFFSET 0`, err: `OFFSET must be > 0 at line 1, char 36`},
{s: `SELECT field1 FROM myseries ORDER`, err: `found EOF, expected BY at line 1, char 35`},
{s: `SELECT field1 FROM myseries ORDER BY /`, err: `found /, expected identifier, ASC, or DESC at line 1, char 38`},
{s: `SELECT field1 FROM myseries ORDER BY 1`, err: `found 1, expected identifier, ASC, or DESC at line 1, char 38`},
Expand Down
8 changes: 8 additions & 0 deletions server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1609,6 +1609,14 @@ func TestServer_ShowSeriesLimitOffset(t *testing.T) {
t.Fatalf("unexpected row count: %d", len(res.Series))
}

// Select data from the server.
results = s.ExecuteQuery(MustParseQuery(`SHOW SERIES LIMIT 4 OFFSET 0`), "foo", nil)
if res := results.Results[0]; res.Err != nil {
t.Fatalf("unexpected error: %s", res.Err)
} else if len(res.Series) != 2 {
t.Fatalf("unexpected row count: %d", len(res.Series))
}

// Select data from the server.
results = s.ExecuteQuery(MustParseQuery(`SHOW SERIES LIMIT 20`), "foo", nil)
if res := results.Results[0]; res.Err != nil {
Expand Down