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

chore: Send query text when executing query over RPC #2179

Merged
merged 1 commit into from
Dec 1, 2023

Conversation

vrongmeal
Copy link
Contributor

{
  "database_id": "00000000-0000-0000-0000-000000000000",
  "connection_id": "00000000-0000-0000-0000-000000000000",
  "query_text": "SELECT * FROM my_pg.public.lineitem WHERE l_shipdate <= DATE '1998-12-01' - INTERVAL '90' LIMIT 5",
  "telemetry_tag": "unknown",
  "execution_status": "success",
  "error_message": null,
  "elapsed_compute_ns": 11627950,
  "output_rows": 5,
  "bytes_read": 15466360,
  "bytes_written": 0
}

@vrongmeal
Copy link
Contributor Author

I didn't make many changes to incorporate an operation_id right now since there are going to be many changes as we're working on distributed exec but getting the query text in was quick and easy enough.

@tychoish
Copy link
Contributor

this is fine in it's current form, getting the operation ID is definitely needed for distributed execution, and I expect that we'll want to pass around an operation context object that will have all of this request-related metadata rather than passing each one in.

@vrongmeal vrongmeal force-pushed the vrongmeal/query-text-rpc branch from 240f4ef to ff856da Compare November 30, 2023 10:42
@vrongmeal vrongmeal marked this pull request as ready for review November 30, 2023 10:44
Comment on lines 497 to 498
plan: LogicalPlan,
query_text: &str,
Copy link
Contributor

Choose a reason for hiding this comment

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

this logicalplan is already a wrapper around the datafusion type, so it could become a tuple of the query-text (or empty string) and the LogicalPlan and that way the function signatures don't need to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried doing that initially. I can't remember what made me change my mind and do this instead. Anyways, will update!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While trying to do the same, I realised these two combine and disassociate quickly. They are required together here only. Combining them makes the code elsewhere hard to work with. Yes, this function signature feels tacky, but we can fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

```json
{
  "database_id": "00000000-0000-0000-0000-000000000000",
  "connection_id": "00000000-0000-0000-0000-000000000000",
  "query_text": "SELECT * FROM my_pg.public.lineitem WHERE l_shipdate <= DATE '1998-12-01' - INTERVAL '90' LIMIT 5",
  "telemetry_tag": "unknown",
  "execution_status": "success",
  "error_message": null,
  "elapsed_compute_ns": 11627950,
  "output_rows": 5,
  "bytes_read": 15466360,
  "bytes_written": 0
}
```

Signed-off-by: Vaibhav <[email protected]>
@vrongmeal vrongmeal force-pushed the vrongmeal/query-text-rpc branch from ff856da to 0c21b66 Compare December 1, 2023 08:16
@vrongmeal vrongmeal merged commit 035fe55 into main Dec 1, 2023
9 checks passed
@vrongmeal vrongmeal deleted the vrongmeal/query-text-rpc branch December 1, 2023 09:37
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.

2 participants