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

Delay parsing of date/time strings until needed #6730

Merged
merged 1 commit into from
May 27, 2016
Merged

Conversation

joelegasse
Copy link
Contributor

The current code would compare every string literal it crossed and tried
to coerce them to time literals if the looked like date/time strings.

The only time the TimeLiteral was used is when comparing to the the
'time' value in a where clause. This change moves the string parsing
code until we attempt to compare 'time' to a string, at which point we
know we need/want a TimeLiteral, and not just an ordinary string.

Fixes #6727

\cc @beckettsean @benbjohnson @jsternberg

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated

@e-dard
Copy link
Contributor

e-dard commented May 27, 2016

LGTM 👍

The current code would compare every string literal it crossed and tried
to coerce them to time literals if the _looked_ like date/time strings.

The only time the TimeLiteral was used is when comparing to the the
'time' value in a where clause. This change moves the string parsing
code until we attempt to compare 'time' to a string, at which point we
know we need/want a TimeLiteral, and not just an ordinary string.

Fixes #6727
@benbjohnson
Copy link
Contributor

The code generally lgtm. 👍 I'm surprised it didn't take a little more code to change.

There seems to be a breaking test in the CLI though:

Unknown precision "". Please use rfc3339, h, m, s, ms, u or ns.
Visit https://enterprise.influxdata.com to register for updates, InfluxDB server management, and monitoring.
Connected to http://127.0.0.1:32988 version x.x
InfluxDB shell version: y.y
> panic: close of closed channel

goroutine 51 [running]:
panic(0x7d2b60, 0xc8201380e0)
    /usr/local/go/src/runtime/panic.go:481 +0x3e6
github.com/influxdata/influxdb/cmd/influx/cli_test.TestRunCLI.func1(0xc820108000)
    /root/go/src/github.com/influxdata/influxdb/cmd/influx/cli/cli_test.go:51 +0x28
created by github.com/influxdata/influxdb/cmd/influx/cli_test.TestRunCLI
    /root/go/src/github.com/influxdata/influxdb/cmd/influx/cli/cli_test.go:52 +0x257

@joelegasse
Copy link
Contributor Author

I was also really surprised that the change was that small. I started by removing the regex matching, and making all strings StringLiterals, and then only one thing broke... so I put the conversion code there. :-)

The CLI test passed on rebuild, I'm guessing there's a race-encouraging test in there somewhere.

@joelegasse joelegasse merged commit 8c3ef8d into master May 27, 2016
@joelegasse joelegasse deleted the jl-date-fix branch May 27, 2016 14:41
@beckettsean
Copy link
Contributor

Thanks for the very quick fix!

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