You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/06/16 04:00:10 UTC
[GitHub] [calcite-avatica-go] brz233 opened a new pull request #50: Replace ExecuteRequest with ExecuteBatchRequest
brz233 opened a new pull request #50:
URL: https://github.com/apache/calcite-avatica-go/pull/50
Where writing a large amount of data, ExecuteRequest need to send multiple http requests. We hope that ExecuteBatchRequest can be used instead of ExecuteRequest to reduce the number of data transmissions as much as possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica-go] F21 commented on pull request #50: Replace ExecuteRequest with ExecuteBatchRequest
Posted by GitBox <gi...@apache.org>.
F21 commented on pull request #50:
URL: https://github.com/apache/calcite-avatica-go/pull/50#issuecomment-645109044
Can you squash the commits?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica-go] brz233 commented on pull request #50: Replace ExecuteRequest with ExecuteBatchRequest
Posted by GitBox <gi...@apache.org>.
brz233 commented on pull request #50:
URL: https://github.com/apache/calcite-avatica-go/pull/50#issuecomment-644576719
I forget to remove docker-compose file when I pushed😅。Adding batching as a configuration flag is a good idea! I also confused about what you mention " if we try to update some data and query it within a transaction, it will not work as expected". I add sync.Mutex on connection when batching=true, I'm not sure if it's a good idea.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica-go] F21 merged pull request #50: Replace ExecuteRequest with ExecuteBatchRequest
Posted by GitBox <gi...@apache.org>.
F21 merged pull request #50:
URL: https://github.com/apache/calcite-avatica-go/pull/50
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica-go] F21 commented on pull request #50: Replace ExecuteRequest with ExecuteBatchRequest
Posted by GitBox <gi...@apache.org>.
F21 commented on pull request #50:
URL: https://github.com/apache/calcite-avatica-go/pull/50#issuecomment-645125324
That's perfect! Thanks for your contribution 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica-go] F21 commented on pull request #50: Replace ExecuteRequest with ExecuteBatchRequest
Posted by GitBox <gi...@apache.org>.
F21 commented on pull request #50:
URL: https://github.com/apache/calcite-avatica-go/pull/50#issuecomment-644588919
Thanks for making those changes! I'll review them very shortly.
I think the mutex for safety is a good addition. What I meant is that if we use batching the result will be unexpected if we query in a transaction:
```
--> begin transaction
--> exec to update data
--> query for changes // data does not appear to be updated
--> exec to update some more data
--> commit // changes are actually sent to the server
```
So, with batching enabled, any query response would not be up to date as the batched changes has not been sent to the server.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica-go] F21 commented on pull request #50: Replace ExecuteRequest with ExecuteBatchRequest
Posted by GitBox <gi...@apache.org>.
F21 commented on pull request #50:
URL: https://github.com/apache/calcite-avatica-go/pull/50#issuecomment-644639393
Looks great!
Just a few small things:
- Protect `batchUpdates` in statement with a mutex. This is because a statement should be safe to be used by multiple goroutines: https://golang.org/pkg/database/sql/#Stmt
- Document the new parameter to turn on batching here: https://github.com/apache/calcite-avatica-go/blob/master/site/_docs/go_client_reference.md#dsn-data-source-name
- Add a test to test batching and make sure it works as expected. Please add one for Phoenix and one for HSQLDB.
Once that's done:
- Open a JIRA issue for this change at https://issues.apache.org/jira/projects/CALCITE/issues
- Squash the commits and change the commit message to: `[CALCITE-XXXX] Your description here (your name here)`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica-go] F21 commented on pull request #50: Replace ExecuteRequest with ExecuteBatchRequest
Posted by GitBox <gi...@apache.org>.
F21 commented on pull request #50:
URL: https://github.com/apache/calcite-avatica-go/pull/50#issuecomment-644520679
Hey thanks for implementing this.
I have a question regarding the implementation. From what I can see this enables batching within a transaction, meaning that all changes within a transaction will not be executed on the server until a commit is issued.
In this case, if we try to update some data and query it within a transaction, it will not work as expected. If this is the case I think batching should be added as a configuration flag and only explicitly enabled by the user.
I don't think we need to commit the `docker-compose-cn.yml` file here as the alpine linux cdn should be accessible within china and it creates more work when it comes to updating the version numbers in the docker-compose file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica-go] F21 edited a comment on pull request #50: Replace ExecuteRequest with ExecuteBatchRequest
Posted by GitBox <gi...@apache.org>.
F21 edited a comment on pull request #50:
URL: https://github.com/apache/calcite-avatica-go/pull/50#issuecomment-645044733
Thank you! That looks great!
Just 2 more small things:
- In the [documentation](https://github.com/apache/calcite-avatica-go/blob/master/site/_docs/go_client_reference.md#dsn-data-source-name), you should add that the statement will only be executed when `Close()` is called.
- There's a small typo in your latest commit message. It should be `[CALCITE-4067] Add support for ExecuteBatchRequest in prepared statement (chenhualin)`
Other than that, squash your commits and I'll merge this :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica-go] brz233 commented on pull request #50: Replace ExecuteRequest with ExecuteBatchRequest
Posted by GitBox <gi...@apache.org>.
brz233 commented on pull request #50:
URL: https://github.com/apache/calcite-avatica-go/pull/50#issuecomment-644623337
I think I fix it? The method send ExecuteBatchRequest on statement Close() now, but exec(ctx context.Context, args []namedValue) would not return the really affectedRows.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica-go] F21 commented on pull request #50: Replace ExecuteRequest with ExecuteBatchRequest
Posted by GitBox <gi...@apache.org>.
F21 commented on pull request #50:
URL: https://github.com/apache/calcite-avatica-go/pull/50#issuecomment-645044733
Thank you! That looks great!
Just 2 more small things:
- In the [documentation](https://github.com/apache/calcite-avatica-go/blob/master/site/_docs/go_client_reference.md#dsn-data-source-name), you should add that the statement will only be executed when `Close()` is called.
- There's a small typo in your latest commit message. It should be `[CALCITE-4067] Add support for ExecuteBatchRequest in prepared statement (chenhualin)`
Other then that, squash your commits and I'll merge this :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica-go] brz233 commented on pull request #50: Replace ExecuteRequest with ExecuteBatchRequest
Posted by GitBox <gi...@apache.org>.
brz233 commented on pull request #50:
URL: https://github.com/apache/calcite-avatica-go/pull/50#issuecomment-645124400
I am not familiar with git, just like that?😅
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org