Aggarwal-Raghav commented on PR #6701:
URL: https://github.com/apache/hive/pull/6701#issuecomment-5523163733
> @Aggarwal-Raghav , @deniskuzZ
>
> My concern with how we currently determine the writer implementation is
that we select it at compile time, but it can later be overridden in
`WriterBuilder`.
>
> I explored an alternative approach. Right now, the writer implementation
depends on two factors:
>
> * Write operation
> * Copy-On-Write mode (IIUC, this PR calculates COW inside `WriterBuilder`
using the statement operation)
>
> What if we add a new enum constant inside `Context.Operation` to merge the
two?
>
> ```
> public enum Operation {UPDATE, DELETE, MERGE, IOW, OTHER, COW}
> ```
>
> And move this logic
>
> ```
> boolean isCOW = IcebergTableUtil.isCopyOnWriteMode(operation,
table.properties()::getOrDefault);
> ```
>
> from `WriterBuilder` to `SemanticAnalyzer`
>
> ```
> private Context.Operation getWriteOperation(String destination, Table
destinatonTable) {
> if (destinatonTable != null &&
> destinatonTable.getStorageHandler() != null &&
>
destinatonTable.getStorageHandler().shouldOverwrite(destinatonTable,
ctx.getOperation())) {
> return Context.Operation.COW;
> }
>
> return deleting(destination) ? Context.Operation.DELETE :
> updating(destination) ? Context.Operation.UPDATE :
> merging(destination) ? Context.Operation.MERGE :
Context.Operation.OTHER;
> }
> ```
>
> We can check whether the operation is `Operation.COW` in the
`WriterBuilder` and in `HiveIcebergSerDe`.
>
> WDYT?
I'm not in favour of this. Here is my reasoning:
1. `Context.Operation` is used at other places for example below, if we add
COW as enum Constant, it will break the default case.
https://github.com/apache/hive/blob/3ad1d0fbb9363d20160de3074b0bddc8f0aa246a/iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergStorageHandler.java#L1683-L1696
2. write.*.mode can be mixed. The core issue in this PR, if table has mixed
property and SemanticAnalyzer set the operation to COW, then will fail
`shouldOverwrite(table, COW)` , it falls into the default switch case, returns
false, and crashes/fails/whatnot.
https://github.com/apache/hive/blob/c1588f1d5fcb183057f1508df9e26ef192248345/iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/IcebergTableUtil.java#L430-L441
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
