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
fix(influxql): don't expand large regex char sets #30
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the max should be lower, but the code is solid so whichever number it is this can be approved.
ast.go
Outdated
// this is exceeded, no expansion will be done. This allows reasonable | ||
// optimizations of regex by expansion to literals but prevents cases | ||
// where that expansion would result in a large number of literals. | ||
const maxLiterals = 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 100 is a better value. 1000 is likely too high.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 fixed
InfluxQL optimizes some queries by expanding regular expressions in the WHERE clause to literal expressions. This causes a problem when the expression expands to a large number of literals. This change caps it at 100 literals. If the expression would expand to more, it is not optimized at all (i.e., no partial optimization).
be022f0
to
3af2fe0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is right approach, but we need to change a couple of things here:
- we can't introduce a change like this that could break existing users' queries without a form of control. We need to propogate
maxLiterals
all the way back to a configuration file option in InfluxDB. The default of 100 seems reasonable. - when I tested this I just got a normal
info
message saying the query executed, but no error or indication the query failed. We need to be able to inform the user that their query failed, if indeed it did.
Further, when the PR to InfluxDB is made, we need to add then config option to the demo config with some description of what it does. We also need to let the docs team know about this change.
|
@dgnorton ah OK I misunderstood the function's purpose. This makes a lot of sense! We should still document in the InfluxDB release notes (top of the changelog) that the optimisation has now been limited to XYZ expressions. |
fixes influxdata/influxdb#13929
InfluxQL optimizes some queries by expanding regular expressions in the
WHERE clause to literal expressions. This causes a problem when the
expression expands to a large number of literals. This change caps it at
100 literals. If the expression would expand to more, it is not
optimized at all (i.e., no partial optimization).