cshuo commented on PR #20329: URL: https://github.com/apache/hudi/pull/20329#issuecomment-6486476074
> Re-reviewed at [b42f641](https://github.com/apache/hudi/commit/b42f641632b2914ac1e2b21e201482caa2ba4976). The table-function gap is closed, and the approach is the right one: an allow-list of table functions instead of a growing deny-list. > > **What I checked.** I ran the PR's `guardrails.py` against sqlglot 30.25. All 239 tests in `test_guardrails.py` pass. I also probed it with bypass attempts beyond the new tests. All of these are rejected: > > * Hudi table functions given a path, or a computed first argument such as `concat('s3a:', '//b/t')`. > * A backtick-quoted function name, `` `hudi_table_changes`('s3a://…') ``. > * A Hudi table function inside `LATERAL`, a `JOIN`, or an `IN (SELECT …)` subquery. > * `hudi_filesystem_view` with a subpath. > * `IDENTIFIER(...)` in any form, `read_files(...)`, and qualified table functions such as `db.range(10)`. > * `reflect` nested inside `stack()`. > > Legitimate queries still pass: `hudi_table_changes('db.t', …)`, `hudi_query_timeline`, `hudi_metadata`, `range`, `LATERAL VIEW explode`, `VALUES`, `TABLESAMPLE`. Two-part names are still forced through the catalog. The new `_cap_limit` handling of `FETCH FIRST`, `LIMIT ALL`, negative limits and `WITH TIES` / `PERCENT` also behaves correctly. > > **💬 SUGGESTION (non-blocking).** The guardrails now depend on specific sqlglot AST shapes: `exp.QueryTransform`, `exp.Lateral`, the `Fetch` `count` and `limit_options` args, and how function names are normalized. But `pyproject.toml` still allows `sqlglot>=25.0`, and CI installs the latest version, so the lower bound is never tested. If an older sqlglot parses one of these constructs differently, the check doesn't fire, and the query is _allowed_, not rejected. Consider raising the floor to the version CI tests with, or adding a small test that asserts the expected node types are produced. > > **Approving.** Separately, as tracked in the plan on #20234: keeping timezones and struct field names in gateway JSON results (M2.2) and confirming that Stop cancels the Spark job (M2.3) are still open for Phase 2. Neither needs to block this PR. The rebase onto #20243 can happen once it lands. Rebased the PR and raised the sqlglot minimum to the verified 30.21.0. CI now also runs the guardrails tests against the minimum version on Python 3.11 and 3.12. -- 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]
