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

Prevent LeftJoinIterators with no required conditions from being built #3159

Merged
merged 3 commits into from Nov 23, 2023

Conversation

mapno
Copy link
Member

@mapno mapno commented Nov 21, 2023

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.

LeftJoinIterator is not designed to handle this case and will loop forever.

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@joe-elliott
Copy link
Member

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?

@mapno
Copy link
Member Author

mapno commented Nov 21, 2023

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 Next(), but IMO they're semantically wrong.

@joe-elliott
Copy link
Member

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.

@stoewer
Copy link
Collaborator

stoewer commented Nov 21, 2023

The other option we considered was returning an error when creating the iterator ... but IMO they're semantically wrong

Returning and error from a constructor when it is called with invalid arguments is a perfectly good solution and semantically correct in my opinion

@mapno
Copy link
Member Author

mapno commented Nov 22, 2023

After discussion, I've changed it to return an error instead of panicking. Thanks for the comments

@mapno mapno merged commit f9100fb into grafana:main Nov 23, 2023
14 checks passed
@mapno mapno deleted the left-join-panic branch November 23, 2023 10:04
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.

None yet

3 participants