Skip to content

Comments

ParameterStatus on SET statements#293

Open
TwistingTwists wants to merge 10 commits intodatafusion-contrib:masterfrom
TwistingTwists:feat/parameter-status-on-set
Open

ParameterStatus on SET statements#293
TwistingTwists wants to merge 10 commits intodatafusion-contrib:masterfrom
TwistingTwists:feat/parameter-status-on-set

Conversation

@TwistingTwists
Copy link

Attempt to fix: #242

Merge unit tests for parameter_status_key_for_set into single
table-driven test. Add handler-level tests that call do_query()
and assert ParameterStatus messages appear in MockClient's
sent_messages, closing the gap where the actual feed() path
was untested. Remove standalone timezone SingleAssignment case
from parameter_status_key_for_set (SET TIME ZONE uses SetTimeZone).
@TwistingTwists
Copy link
Author

I fixed the cargo fmt issue. Can you trigger the CI please @sunng87 ?

Copy link
Member

@sunng87 sunng87 left a comment

Choose a reason for hiding this comment

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

Thank you @TwistingTwists ! Please check my comments about moving the logic into our QueryHook completely.

{
if result.is_ok() {
if let Some((name, value)) =
set_show::parameter_status_key_for_set(&statement, client)
Copy link
Member

Choose a reason for hiding this comment

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

I haven't checked the order of returned packets for SET statements, I wonder if it's possible to add this logic to set_show's try_respond_set_statements. We can modify the definition of client in QueryHook if needed.

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.

Send ParameterStatus on SET statements

2 participants