Prevent LeftJoinIterators with no required conditions from being built - #3159
Conversation
|
I've noticed this as well and was tempted to write similar code. I am concerned about the panic, but I'm not sure there's a better way. wdyt? |
I think it's a fair approach. It's a programming bug, not an error that the program can handle at runtime: it's not transient, it's not due to bad input—ie. it can't be fixed except for fixing the code that generates it. The other option we considered was returning an error when creating the iterator or when calling |
|
My concern is that, with this change, there may be a way to write a query that panics every ingester. As our query path continues to get more complex I continue to think we should wrap queries in defer/recover. |
Returning and error from a constructor when it is called with invalid arguments is a perfectly good solution and semantically correct in my opinion |
|
After discussion, I've changed it to return an error instead of panicking. Thanks for the comments |
…ilt (grafana#3159) * Prevent LeftJoin iterators with no required conditions from being built * chlog * Change panic for err return
3 participants
What this PR does:
No query should ever result in a left-join with no required iterators. If that happens, it's a bug in the iter building code, so it returns an error.
LeftJoinIteratoris not designed to handle this case and will loop forever.Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]