fix: Fix panic in filter predicate#7126
Conversation
github-actions
bot
added
core
| if !is_operator_supported(binary.op()) || columns.is_empty() { | ||
| return Statistics::default(); | ||
| } | ||
| if !check_support(predicate) { |
There was a problem hiding this comment.
There is an existing function that does the correctly recursive check
There was a problem hiding this comment.
Do we need to keep columns.is_empty() check?
There was a problem hiding this comment.
I don't know its purpose -- given that all the tests still pass I think the answer is no, but it would be good to get a second opinion on that
There was a problem hiding this comment.
I guess it means if the predicate doesn't involve any column, we cannot do selectivity check so just return default?
alamb
changed the title
|
@berkaysynnada do you perhaps have time to review this PR? |
LGTM, thank you for the fix. Actually, it's good that you've removed the default return in the case that there is no column in the predicate. We don't need to provide a default return for those with 0 selective deterministic filters. Now, the calculations will yield a 0 selectivity result for these cases, instead of returning a default (as in the instance of 1>2). |
3 participants
Which issue does this PR close?
Closes #7125
Rationale for this change
Panic in query isn't good
What changes are included in this PR?
Fix the check if a predicate is supported for range analysis
Are these changes tested?
yes
I also verified this PR passes the IOx tests in https://github.com/influxdata/influxdb_iox/pull/8355
Are there any user-facing changes?