pan3793 commented on PR #7748: URL: https://github.com/apache/kyuubi/pull/7748#issuecomment-6443586671
@attilapiros thanks for working on this, and sorry for the slow response. Following my [comment](https://github.com/apache/kyuubi/issues/3845#issuecomment-5728798849) on the issue, I think the required privileges should depend on how the connector actually replaces the table: | Replace semantics | Required privileges on the target table | |---|---| | Atomic replace (`StagingTableCatalog#stageReplace` / `stageCreateOrReplace`); keeps table history, grants, etc. | `CREATE` + `ALTER` + `UPDATE` | | Non-atomic replace (`DROP` then `CREATE`) | `CREATE` + `DROP` | | RTAS input tables | `SELECT` | Reasoning: 1. An atomic replace is closer to Unity Catalog's `MODIFY` than to `DROP`. It changes schema and partitioning (`ALTER`) and rewrites the data (`UPDATE`, the same as `INSERT OVERWRITE` and `TRUNCATE`), and the old snapshots stay reachable. 2. `CREATE` is still needed because `CREATE OR REPLACE` may create a new table, and `{OWNER}` policies can't match a table that doesn't exist yet. 3. A non-atomic replace is exactly `DROP TABLE` + `CTAS`, so it should require the same privileges as running those two statements. The hard part is telling the two cases apart. `catalog.isInstanceOf[StagingTableCatalog]` is not enough. In production, the session-catalog wrappers are the most common setup, and they fall back to drop-then-create when the provider isn't their own format: 1. Iceberg `SparkSessionCatalog#stageReplace` / `stageCreateOrReplace` calls `dropTable` and then `createTable` for non-Iceberg providers, or when the underlying Iceberg catalog doesn't support staging. 2. Delta `DeltaCatalog#stageReplace` / `stageCreateOrReplace` calls `dropTable` and then `createCatalogTable` (via `BestEffortStagedTable`) for non-Delta providers. So a check on the catalog type alone would let a user who has `ALTER` + `UPDATE` permanently drop a parquet or Hive table through `spark_catalog`. Approaches I can think of, each with a downside: 1. Treat `StagingTableCatalog` that is not a `CatalogExtension` as atomic. This is simple, but it doesn't help the session-catalog setup, which is where most users are. 2. Decide by catalog plus provider, e.g. `SparkSessionCatalog` + `iceberg`, `DeltaCatalog` + `delta`. This is accurate for known connectors, but authz then hard-codes connector internals, which have to be kept in sync with upstream changes. Unknown connectors would need to default to `CREATE` + `DROP`. 3. Always require `CREATE` + `DROP`, which is the current PR. This is safe, but stricter than needed for Iceberg, Delta and other lakehouse formats, and those are the main users of `REPLACE`. Feel free to suggest a better option. Whichever way we go, the operation type for `ReplaceTable` / `ReplaceTableAsSelect` can't stay a fixed string in `table_command_spec.json`. Both plans carry `ResolvedIdentifier(catalog, ident)`, so it could come from an extractor at analysis time, as @bowenliang123 suggested earlier on the issue. Other comments: 1. The reproducer in #3845 is `CREATE OR REPLACE VIEW`, and it still requires only `CREATE` after this PR. Please either cover views here or open a separate issue for tables, so that merging this PR doesn't close #3845. 2. Please add Ranger-level tests for `CREATE OR REPLACE TABLE ... AS SELECT`, for `CREATE OR REPLACE` on a table that doesn't exist yet, and for a non-admin user who has exactly the required privileges and succeeds. The current tests only cover plain `REPLACE TABLE` on an existing table, with only admin as the passing case. 3. Nits: 1. `getAccessTypes` -> `requiredAccessTypes`. 2. Keep `OperationType` sorted alphabetically: `REPLACETABLE*` should come after `RELOADFUNCTION`. 3. The PR description says the new types map to `CREATE` and `DROP`, but RTAS inputs map to `SELECT`. Related gaps that existed before this PR, which can go to follow-up issues: 1. V1 `df.write.mode("overwrite").saveAsTable(...)` drops the existing table without a `DROP` check. 1. On Spark 3.5 and 4.0, `DataFrameWriter` calls `dropTable` directly, and authz only sees the following CTAS, which requires `CREATE`. 2. On Spark 4.1+, `SaveAsV1TableCommand` (SPARK-52451) is not in `table_command_spec.json`, so it isn't checked at all. Once replace on V2 tables requires `DROP`, V1 and V2 overwrite will behave inconsistently. 2. `CREATE OR REPLACE FUNCTION` requires only `CREATE`. 3. Spark 4.2+ V2 `CreateView` (`org.apache.spark.sql.catalyst.plans.logical.CreateView`, which includes `OR REPLACE`) is not in `table_command_spec.json`, so it falls back to `QUERY` with no privilege objects. 4. Delta `CREATE OR REPLACE TABLE ... CLONE` (`CloneTableStatement`) is not in `table_command_spec.json`. -- 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]
