You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "avantgardnerio (via GitHub)" <gi...@apache.org> on 2023/04/01 18:16:55 UTC

[GitHub] [arrow-datafusion] avantgardnerio opened a new pull request, #5827: Implement LogicalPlan support for transactions

avantgardnerio opened a new pull request, #5827:
URL: https://github.com/apache/arrow-datafusion/pull/5827

   # Which issue does this PR close?
   
   Closes #5826.
   
   # Rationale for this change
   
   To make it easier for folks to build execution engines on top of DataFusion's planner (see issue for more detail).
   
   # What changes are included in this PR?
   
   Two LogicalPlan nodes for starting and ending transactions.
   
   # Are these changes tested?
   
   Additional tests were added to the planner.
   
   # Are there any user-facing changes?
   
   When users try to run begin and end transaction statements, they will now fail in physical instead of logical planning.


-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5827: Implement LogicalPlan support for transactions

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5827:
URL: https://github.com/apache/arrow-datafusion/pull/5827#discussion_r1155363480


##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1057,6 +1069,20 @@ impl LogicalPlan {
                     }) => {
                         write!(f, "DropTable: {name:?} if not exist:={if_exists}")
                     }
+                    LogicalPlan::TransactionStart(TransactionStartNode {

Review Comment:
   The other variants seem to use the name name for the variant and the structure they contain. I wonder if we should call this `TransactionStart` rather than `TransactionStartNode` 🤔  -- like
   
   ```suggestion
                       LogicalPlan::TransactionStart(TransactionStart {
   ```



-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] avantgardnerio merged pull request #5827: Implement LogicalPlan support for transactions

Posted by "avantgardnerio (via GitHub)" <gi...@apache.org>.
avantgardnerio merged PR #5827:
URL: https://github.com/apache/arrow-datafusion/pull/5827


-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5827: Implement LogicalPlan support for transactions

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5827:
URL: https://github.com/apache/arrow-datafusion/pull/5827#discussion_r1155363480


##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1057,6 +1069,20 @@ impl LogicalPlan {
                     }) => {
                         write!(f, "DropTable: {name:?} if not exist:={if_exists}")
                     }
+                    LogicalPlan::TransactionStart(TransactionStartNode {

Review Comment:
   The other variants seem to use the name name for the variant and the structure they contain. I wonder if we should call this `TransactionStart` rather than TransactionStartNode` 🤔 
   
   ```suggestion
                       LogicalPlan::TransactionStart(TransactionStart {
   ```



-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] alamb commented on pull request #5827: Implement LogicalPlan support for transactions

Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5827:
URL: https://github.com/apache/arrow-datafusion/pull/5827#issuecomment-1494129960

   Here is a proposal to move these statements into their own variant: https://github.com/apache/arrow-datafusion/pull/5842


-- 
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.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org