joellubi commented on PR #38385:
URL: https://github.com/apache/arrow/pull/38385#issuecomment-1814346224

   @emkornfield @lidavidm Does this capture the structure/behavior you would 
expect?
   
   ```proto
   message CommandStatementIngest {
     option (experimental) = true;
   
     enum TableDoesNotExistOptions {
        CREATE = 0;
        FAIL = 1;
     }
   
     enum TableExistsOptions {
        FAIL = 0;
        APPEND = 1;
        REPLACE = 2;
     }
   
     TableDoesNotExistOptions if_table_not_exist = 1; // Suggestions for naming?
     TableExistsOptions if_table_exists = 2;
     string table = 3;
     optional string schema = 4;
     optional string catalog = 5;
     optional bool temporary = 6;
     optional bytes transaction_id = 7;
     map<string, string> options = 1000;
   }
   ```
   
   I believe this would imply the following correspondence between the old and 
new structures:
   ```proto
   // Default TableDoesNotExistOptions.CREATE, TableExistsOptions.FAIL
   CommandStatementIngest(mode=CREATE) -> CommandStatementIngest() 
   
   CommandStatementIngest(mode=APPEND) -> 
CommandStatementIngest(if_table_not_exist=FAIL, if_table_exists=APPEND) 
   
   // Default TableDoesNotExistOptions.CREATE
   CommandStatementIngest(mode=REPLACE) -> 
CommandStatementIngest(if_table_exists=REPLACE) 
   
   // Default TableDoesNotExistOptions.CREATE
   CommandStatementIngest(mode=CREATE_APPEND) -> 
CommandStatementIngest(if_table_exists=APPEND) 
   ```
   
   This new structure does also add two new cases, though only one is 
meaningful:
   ```proto
   CommandStatementIngest(if_table_not_exist=FAIL, if_table_exists=FAIL) // 
Degenerate case
   CommandStatementIngest(if_table_not_exist=FAIL, if_table_exists=REPLACE) // 
Must replace existing
   ```
   
   The only thing that `temporary` would change would be the `catalog` and/or 
`schema` based on the backend-specific handling of temporary tables.
   
   Does this all align with your understanding and what you intended? Also, 
should we remove `optional` from `temporary`? That way `temporary` is 
automatically interpreted as `false` when unset instead of handling it in the 
code.


-- 
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-unsubscr...@arrow.apache.org

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

Reply via email to