plusplusjiajia commented on PR #758: URL: https://github.com/apache/paimon-rust/pull/758#issuecomment-5810391218
> Reviewed head `3c7b1694` for the full REST scan/read path and alternate read entry points. This has end-to-end value: a REST user with an unrestricted `query-auth` response can now plan and read real rows, while restricted responses fail closed. > > **[P2] Literal REST table names containing `$` become unreadable.** `RESTCatalog::get_table` and `load_table` now call `identifier.reject_decorated()` unconditionally (`crates/paimon/src/catalog/rest/rest_catalog.rs:277,289`). `create_table` still accepts a literal name such as `orders$files` (the identifier validator permits `$`, and the server stores the submitted name). I reproduced the create-then-read path in a temporary REST integration test: creation succeeded, but `get_table` failed with `Unsupported: 'default.orders$files' is a decorated name`. This affects ordinary tables with query auth disabled too. Please make create/load semantics consistent and keep the protected-view refusal without breaking previously readable literal names. > > The PR description also needs updating before merge. It says query-auth enablement is read live from the server for each entry point and advertises a new public `Table::ensure_read_authorized` method. The current code uses the option cached on the loaded handle and no longer exposes that method. This matters to the documented behavior of old handles after authorization changes. > > Validation: `cargo test -p paimon --test rest_catalog_test query_auth` (15 passed), `cargo test -p paimon --lib query_auth` (29 passed), and the REST-server decorated-object test (1 passed). The head CI is green. In `cargo test -p paimon-datafusion partition_count`, the two matching unit tests passed; a separate `read_tables` integration test then failed because its `default.partitioned_log_table` fixture was missing in this local checkout, so that run does not validate the DataFusion fallback end to end. The temporary regression test failed as described above and was removed after verification. @JingsongLi You're right. That rejection only served the live check, which is gone, so `reject_decorated` is dropped from `RESTCatalog::get_table`/`load_table` and `RESTEnv::build_table` — all three back to main's code, and a literal `orders$files` loads again. A query-auth table under such a name is still refused, but at planning: `authorize_read` treats a `$branch_x`/`$files` handle as reading a schema the server did not rule on. Test covers both: `plain$files` loads, `guarded$files` is refused when planned. Description updated. -- 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]
