-
Notifications
You must be signed in to change notification settings - Fork 21
Introduce QueryHook so users can process custom queries with own logic. #204
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
Introduce QueryHook so users can process custom queries with own logic. #204
Conversation
@sunng87 I made a pr as discussed. I'm unsure what kind of Statement arguement to accept. datafusion::sql::Statement is a wrapper enum with a variant Statement::Statement(Boxdatafusion::logical_expr::sqlparser::ast::Statement) |
Hello @tonyalaribe , Thank you so much for the quick update! It should be the Statement from parser. I will leave some comments inline. |
datafusion-postgres/src/handlers.rs
Outdated
} | ||
|
||
// Call query hook with the parsed statement | ||
if let Some(hook) = &self.query_hook { |
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.
Loop over query_hooks and make early return if one of them returns non-empty response
datafusion-postgres/src/handlers.rs
Outdated
statement: &Statement, | ||
session_context: &SessionContext, | ||
client: &dyn ClientInfo, | ||
) -> Option<PgWireResult<Vec<Response>>>; |
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.
Because the input is a single Statement
, the return value should be at most one Response
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.
Sounds good. I've updated the code to return a single Response.
datafusion-postgres/src/handlers.rs
Outdated
|
||
// Call query hooks with the parsed statement | ||
for hook in &self.query_hooks { | ||
let wrapped_statement = Statement::Statement(Box::new(statement.clone())); |
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.
Do we still need wrap here? Can we feed statement
directly to hook.handle_query
?
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 was simply felt we would want to expose the datafusion Statement and types to users instead of the sqlparser's. But not wrapping might also be cleaner. I've updated the code.
… need to wrap in a datafusion::Statement
…ponse is expected.
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.
There is some minor issue with early return. But overall it looks good to me and let me pick up the work to update. Thank you so much @tonyalaribe ! This is a major step for extensibility of datafusion-postgres.
Introduce QueryHook so users can process custom queries with own logic.
This will allow users to implement other behaviors like SET queries,
s other sql query kinds which might not be supported by datafusion-postgres.
The source conversation is from this thread: #201