-
-
Notifications
You must be signed in to change notification settings - Fork 115
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
#180 fix for destinationTable not returned #187
base: main
Are you sure you want to change the base?
Conversation
@@ -1485,7 +1485,7 @@ func (h *jobsInsertHandler) Handle(ctx context.Context, r *jobsInsertRequest) (* | |||
func (h *jobsInsertHandler) addQueryResultToDynamicDestinationTable(ctx context.Context, tx *connection.Tx, r *jobsInsertRequest, response *internaltypes.QueryResponse) error { | |||
projectID := r.project.ID | |||
jobID := r.job.JobReference.JobId | |||
datasetID := jobID | |||
datasetID := "ds_" + jobID |
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.
why this is needed?
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.
datasetId
must be unique although tableId
does not need to. That's why we needed.
reference:
https://cloud.google.com/ruby/docs/reference/google-cloud-bigquery/latest/Google-Cloud-Bigquery-Project#Google__Cloud__Bigquery__Project_create_dataset_instance_
https://cloud.google.com/ruby/docs/reference/google-cloud-bigquery/latest/Google-Cloud-Bigquery-Dataset#Google__Cloud__Bigquery__Dataset_create_table_instance_
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 think the jobID is already unique for each request. So if you just want to make datasetID unique, jobID seems fine.
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.
It seems fine, but actually, after deleting this ds_
I got an error below when using bigquery ruby client.
Do you know what causes this erorr? @goccy
2023-09-21 21:49:45 bq-emulator-test-bq-1 | 2023-09-21T12:49:45.555Z ERROR server/handler.go:2355 internalError {"error": "internalError: failed to query SELECT * FROM `job_JSsjGMduSIBf5L_poPWkkL2vZdkd`: no such table: test_job_JSsjGMduSIBf5L_poPWkkL2vZdkd"}
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.
@kromiii Sorry, I don't known by this information. I'd like to have reproducible test code. Go is preferred, but other languages are also acceptable. Also, please attach the logs obtained at that time.
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 created a repo to test this. https://github.com/kromiii/bq-emulator-test
This issue depends on the ruby client, so I attached a simple ruby script for testing bigquery-emulator.
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.
@prabhav007 Let us know the reason of ds_
if you are available.
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.
In case of jobs with anonymous tables, when the dataset name and the table name are the same, an error response is received. When they are different, it works fine. PFB the error response I got locally.
{"error":{"errors":[{"reason":"internalError","location":"","debugInfo":"","message":"failed to query SELECT * FROM 0ffd9b42-7541-4317-ad16-fad5ec3beb55
: no such table: demo-0d168064_0ffd9b42-7541-4317-ad16-fad5ec3beb55"}],"code":500,"message":"failed to query SELECT * FROM 0ffd9b42-7541-4317-ad16-fad5ec3beb55
: no such table: demo-0d168064_0ffd9b42-7541-4317-ad16-fad5ec3beb55"}}
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.
@prabhav007 Thank you. I could reproduce the error. However, the quetion is that it works well in the python clinent even if the dataset name and the table name is the same. What makes this difference?
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 am not sure, I was using the java client for spring.
I have the same problem. How about I take over if there is no progress? |
I applied these changes to my branch, and it resolved the error. |
Yes your changes and your image solved a similar problem when I issue a query with a specific JobId. I hope we can have this fix merged in to goccy project and image. |
@yaronneuman it seems your question has been answered 3 weeks ago by someone else. Also others mention the code works well... is there anything that's still keeping you from getting this merged? Getting this fixed can also help to resolve an issue being the discussed in the DBT community: @goccy can you help perhaps? |
@benvdh-incentro |
@goccy I see @prabhav007 and @kromiii have delivered some test code and further rationale behind the current PR, anything else that is still needed to get this merged? |
The reason I cannot merge this PR is that there is no test code in this repository that corresponds to the modification. If there is no test code, I need to determine the validity of the modification from the diffs, which is currently difficult. |
I can confirm this breaks Ruby client. 🤔 Any idea how to move this forward? Even with this patch only query like bigquery-emulator/server/handler.go Line 1427 in 316038b
I have checked BQ response and it returns destination table even for empty result. |
@kromiii I can try to help with writing test, but seems destination table is not tested currently at all. |
Any update on this? The Go client doesn't work because the job status destination table is not populated. |
FYI this also seems to break Scala client on this line. |
If this cannot be merged soon, any chance you can rebase your branch and release a fresh version @kromiii? |
Ok, I rebuild the package applying this patch. @turb I hope it will help. |
closes #180