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.

Sign up for GitHub

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

Jump to bottom

Support a restricted SQL subset for querying timeseries #4475

Merged
merged 2 commits into from
Dec 12, 2023
Merged

Support a restricted SQL subset for querying timeseries #4475

merged 2 commits into from
Dec 12, 2023

Conversation

bnaecker
Copy link
Collaborator

@bnaecker bnaecker commented Nov 9, 2023

  • Add methods for querying oximeter timeseries using a limited subset of SQL. The raw string is heavily validated, and only simple SELECT queries are currently supported, with a limited subset of ClickHouse functions. Still, this allows running many different kinds of queries, including aggregations, window functions, and joins.
  • Adds a few types for reading out tabular data from the query result, and parsing basic metadata for understanding resource usage of the queries.
  • Add a barebones SQL shell for running oximeter SQL queries, using the new Client::query method.
  • Include a bunch of tests for the restricted SQL subset as well as correctness of the actual returned queries against the DB.

@bnaecker bnaecker marked this pull request as draft November 9, 2023 00:28
@bnaecker bnaecker force-pushed the oxdb-sql branch 9 times, most recently from 16ef0c5 to 085b376 Compare November 10, 2023 21:55
@bnaecker bnaecker marked this pull request as ready for review November 10, 2023 21:57
@bnaecker bnaecker requested a review from ahl November 13, 2023 23:08
@bnaecker bnaecker assigned davepacheco and unassigned davepacheco Nov 13, 2023
@bnaecker bnaecker force-pushed the oxdb-sql branch 2 times, most recently from 3511e0b to 4d644f7 Compare December 11, 2023 20:49
@bnaecker
Copy link
Collaborator Author

This one has been hanging around for a while, so I'm not sure if folks have any feedback about it. On one hand, there is basically zero risk right now. SQL support is only available through the internal oxdb development tool, not the public API. I've tried to ensure that all queries must be read-only, and there are a number of other guardrails in place (e.g., a function allow-list). On the other hand, if this isn't a good direction, we may not want to even start down it.

@bnaecker
Support a restricted SQL subset for querying timeseries
07a2023 
- Add methods for querying oximeter timeseries using a limited subset of
  SQL. The raw string is heavily validated, and only simple SELECT
  queries are currently supported, with a limited subset of ClickHouse
  functions. Still, this allows running many different kinds of queries,
  including aggregations, window functions, and joins.
- Adds a few types for reading out tabular data from the query result,
  and parsing basic metadata for understanding resource usage of the
  queries.
- Add a barebones SQL shell for running oximeter SQL queries, using the
  new `Client::query` method.
- Include a bunch of tests for the restricted SQL subset as well as
  correctness of the actual returned queries against the DB.
- Add a small README for the SQL shell
david-crespo
david-crespo reviewed Dec 11, 2023
View reviewed changes
oximeter/db/src/sql/mod.rs
}
let SetExpr::Select(select) = &mut *query.body else {
return unsupported!("Only SELECT queries are currently supported");
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was not aware of let/else. that is handy!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, might be my favorite syntax enhancement from the last few Rust releases!

david-crespo
david-crespo approved these changes Dec 11, 2023
View reviewed changes
Copy link
Contributor

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, been busy! I think this looks great, and it will be very helpful to exercise it as a dev tool to inform the much harder decision about whether to incorporate it into the API somehow.

@bnaecker bnaecker merged commit 2d95aac into main Dec 12, 2023
22 checks passed
@bnaecker bnaecker deleted the oxdb-sql branch December 12, 2023 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-crespo david-crespo david-crespo approved these changes

@ahl ahl Awaiting requested review from ahl

@davepacheco davepacheco Awaiting requested review from davepacheco

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bnaecker @david-crespo @davepacheco