Support REST catalog views and SQL functions#494
Conversation
JingsongLi
force-pushed
the
codex/rest-catalog-views-sql-functions
branch
from
cca29e5 to
30a283b
Compare
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Thanks, the overall structure is clear, but I found several blocking correctness and compatibility issues. I reproduced the CTE and HTTP 501 cases with focused local regression tests, and the Python integration CI currently exposes the no-catalog regression. Please address the inline comments and add coverage for these cases.
| "REST SQL function expansion exceeded the reference limit of {MAX_FUNCTION_REFERENCES}" | ||
| ))); | ||
| } | ||
| let catalog = catalogs.get(&reference.0).ok_or_else(|| { |
There was a problem hiding this comment.
This lookup regresses SQLContext usages that have no registered Paimon catalog. Every function call is treated as a REST-function candidate, current_catalog is then DataFusion's default datafusion, and this branch returns Unknown catalog 'datafusion' before registered/built-in UDF resolution. The current Python integration CI fails video_snapshot and vector_from_json tests for exactly this reason. Please make expansion a no-op/fallback when the current catalog is not in the Paimon catalog map (while retaining an appropriate error for explicitly invalid qualification), and add a no-catalog built-in regression test.
| Ok(function) => Some(function), | ||
| Err(paimon::Error::FunctionNotExist { .. }) | ||
| | Err(paimon::Error::Unsupported { .. }) => None, | ||
| Err(error) => return Err(crate::to_datafusion_error(error)), |
There was a problem hiding this comment.
Please preserve compatibility with REST servers that do not implement the new function endpoint. This code probes get_function for every scalar/aggregate call, including built-ins such as abs and count; RESTCatalog::get_function currently leaves HTTP 501 as Error::RestApi(NotImplemented), so SELECT abs(-1) fails instead of falling through to DataFusion. I reproduced this with a catalog returning RestError::NotImplemented. Please map 501 to Unsupported at the REST catalog boundary or handle it as a non-persistent function here, and add a regression test. The same compatibility consideration applies to the new view endpoint.
| } | ||
|
|
||
| let mut identifiers = Vec::new(); | ||
| let _: std::ops::ControlFlow<()> = visit_relations(&statements, |relation| { |
There was a problem hiding this comment.
visit_relations also visits references to local CTE names, but this code treats every unqualified relation as a catalog object. For example, a persistent view named cte_view with WITH cte_view AS (SELECT CAST(42 AS BIGINT) AS answer) SELECT * FROM cte_view is rejected as default.cte_view -> default.cte_view recursion; I reproduced this locally. Please exclude CTE aliases according to their SQL scope (including nested CTEs) before catalog lookup, and add a regression test.
| match expand_call(function, call, &reference.0, &functions) { | ||
| Ok(expanded) => { | ||
| *expr = expanded; | ||
| expanded_count += 1; |
There was a problem hiding this comment.
The current limits bound nesting depth and the number of distinct function definitions, but not the number of expanded call sites. A chain such as f0(x) = f1(x) + f1(x), ..., f31(x) = x creates roughly 2^31 subexpressions while using only 32 references and 32 rounds, so the process can exhaust memory before the depth guard fires. Please add a total expanded-call/AST-size budget checked before replacement, with a branching-chain regression test using a small configured limit.
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Thanks, the four issues from the previous review are addressed, and the focused regressions for CTE scoping, unsupported endpoints, no-catalog function resolution, and bounded expansion now pass. On this second pass I found additional source-compatibility and SQL-resolution gaps described inline. I also ran the rest_ DataFusion tests (18 passed), REST object model tests (6 passed), the HTTP 501 catalog fallback test, and cargo fmt --all -- --check.
| } | ||
|
|
||
| pub(crate) fn with_dynamic_options( | ||
| pub fn new( |
There was a problem hiding this comment.
Please preserve the existing public PaimonCatalogProvider::new(Arc<dyn Catalog>) API. This changes it to a five-argument constructor, so every downstream crate using the provider directly stops compiling; I confirmed the old call now fails with E0061. The extra session state is only needed for the SQLContext integration, so keeping new(catalog) and adding a crate-private/session-aware constructor would avoid an unrelated source-breaking change. PaimonSchemaProvider::new is changed similarly.
There was a problem hiding this comment.
Thanks for calling this out. We intentionally do not preserve source compatibility for these provider constructors: PaimonCatalogProvider and PaimonSchemaProvider are implementation details of the DataFusion integration, while SQLContext is the supported public API. The constructors therefore keep the session-aware parameters required for persistent REST view planning. I have removed the compatibility wrappers.
| } | ||
| } | ||
|
|
||
| fn sql_identifier_key(identifier: &Ident) -> SqlIdentifierKey { |
There was a problem hiding this comment.
Please match DataFusion's identifier normalization rather than treating quote_style as part of the CTE identity. With default normalization, WITH cte_view AS (...) SELECT * FROM "cte_view" references the same CTE, but these keys differ (None versus Some('\"')). If the persistent view itself is named cte_view, dependency validation reports a false self-cycle. I reproduced this locally: recursive REST catalog view dependency detected: default.cte_view -> default.cte_view. The same normalization should be applied when relation parts are converted to catalog/database/object names.
| .name | ||
| .0 | ||
| .iter() | ||
| .map(|part| part.as_ident().map(|ident| ident.value.clone())) |
There was a problem hiding this comment.
Please apply DataFusion identifier normalization before REST function lookup and parameter substitution. A catalog function stored as plus_one is not expanded for SELECT PLUS_ONE(41) because the raw Ident.value is used; DataFusion lowercases unquoted identifiers by default, so the query should resolve the same function. I reproduced the current behavior as Invalid function 'plus_one'. Catalog/database components and unquoted parameter identifiers inside stored definitions need the same treatment.
| ) | ||
| .await | ||
| } | ||
| Statement::Query(_) => { |
There was a problem hiding this comment.
Please ensure REST function expansion is used by the other query paths as well. In particular, sql() returns through handle_time_travel_query before reaching this arm, and that handler calls the raw SessionContext, so a query such as SELECT plus_one(id) FROM t VERSION AS OF 1 bypasses expansion and fails function resolution. EXPLAIN SELECT plus_one(1) also does not enter the Statement::Query arm. The PR states that queries executed through SQLContext can call these functions, so these existing query forms should not silently lose the feature (or the limitation should be explicit and intentional).
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Thanks, the CTE/relation normalization, function call normalization, and EXPLAIN/time-travel expansion issues are addressed, and the new focused tests pass locally. I also accept the provider-constructor break as an intentional API decision based on the explanation in the thread. One parameter-binding normalization edge remains, described inline.
| let replacements: HashMap<String, SqlExpr> = input_params | ||
| .iter() | ||
| .zip(values) | ||
| .map(|(field, value)| (normalize_identifier(&Ident::new(field.name())), value)) |
There was a problem hiding this comment.
Please preserve the exact metadata parameter name when building this binding map. DataField::name() is already the catalog name, not an unquoted SQL token; wrapping an uppercase name such as X in Ident::new normalizes it to x. A valid definition using the exact quoted identifier ("X" + 1) then fails with references undeclared identifier 'X'. I reproduced this with a focused test. This also collapses distinct parameters such as x and X. Keeping field.name().to_string() as the key while normalizing only identifiers parsed from the SQL definition preserves both unquoted lookup semantics and quoted exact names.
leaves12138
left a comment
leaves12138
left a comment
There was a problem hiding this comment.
Thanks, all previously reported issues are addressed. I rechecked the REST view CTE scoping, unsupported-endpoint fallback, function expansion budget, DataFusion identifier normalization, EXPLAIN/time-travel paths, and quoted metadata parameter binding. The focused local regressions pass, and all CI checks are green.
2 participants
What changed
Why
paimon-datafusionpreviously treated persistent REST Catalog views as missing tables and could not resolve SQL functions stored in the catalog. This adds read-and-execute support for existing objects without introducing view or function DDL.User impact
Queries executed through
SQLContextcan now read persistent REST views and call existing SQL scalar functions as eitherfunction(...)in the current namespace orcatalog.database.function(...). Unsupported definitions and unsafe shapes fail during planning with explicit errors.Validation
cargo test -p paimon-datafusion --lib rest_— 16 passedcargo test -p paimon --test rest_object_models_test— 6 passedcargo test -p paimon --test rest_api_test— 21 passedcargo clippy -p paimon -p paimon-datafusion --lib --tests -- -D warningscargo fmt --all -- --checkmkdocs build --strictThe full
paimon-datafusionlibrary test run also reached 222 passing tests; seven existing scan tests could not load their expected/tmp/paimon-warehousefixture metadata in this local environment.