pepijnve commented on issue #26997: URL: https://github.com/apache/datafusion/issues/26997#issuecomment-6564774615
I experimented with a number of different approaches, but I haven't landed on anything satisfactory yet. Here's a summary of where I've gotten, questions for anyone wishing to participate at the bottom. The SQL planner turns `CREATE VIEW` into a `CreateView` plan node. It rebuilds the whole statement from the AST and stores its text as `definition`. That string ends up in `ViewTable` and is returned by `TableProvider::get_table_definition()`. Two different code paths use `get_table_definition`: - `information_schema.views.definition` use this value as is, but should show only the query (`SELECT * FROM abc` iso `CREATE VIEW xyz AS SELECT * FROM abc`). - `SHOW CREATE TABLE` should show the full statement, and does today. The trait documents `get_table_definition()` as "the create statement used to create this table". Listing tables also return their `CREATE EXTERNAL TABLE` statement through it. So the current value is correct for `SHOW CREATE TABLE`; the information schema needs a second string or a substring somehow. ### A. Store only the query The planner stores `query.to_string()` as `definition` value. `SHOW CREATE TABLE` rebuilds `CREATE VIEW <qualified name> AS <query>` for tables with view type. - Pro: about 10 lines changed, no new API. - Con: column aliases are lost in both places, so `SHOW CREATE TABLE` output can recreate a view with different column names. - Con: changes what `get_table_definition()` returns for views, contrary to its docs. ### B. Fold aliases into the query Like A, but with aliases present the definition becomes `SELECT * FROM (<query>) AS v (a, b)`, close to how PostgreSQL rewrites the query. - Pro: the definition runs on its own and yields the right column names. One code path handles `SELECT *`, `UNION` and `VALUES`. - Con: the definition is no longer the text the user wrote. - Con: keeps A's `get_table_definition()` change. MySQL simply drops the aliases, so this may be more than needed. ### C. Store aliases separately, downcast in `SHOW CREATE TABLE` Add `column_aliases: Option<Vec<String>>` to `CreateView`, and `ViewTable`. `SHOW CREATE TABLE` downcasts to `ViewTable` to print `(a, b)` only when declared. - Pro: the alias list appears exactly when it was declared. - Con: the downcast excludes other view implementations. - Con: breaks code that builds `CreateView` directly, and changes `get_table_definition()` as in A. ### D. Keep the statement, strip it by text search Leave everything as it is and cut the statement after `AS SELECT` in the information schema. - Pro: no API change. - Con: fragile. It misses `AS VALUES`, `AS WITH ...` and `AS (SELECT ...)`, and matches inside quoted names such as `CREATE VIEW "x AS SELECT"`. ### E. Keep the statement, re-parse it The information schema parses the stored statement with sqlparser and prints its query. - Pro: works for any provider that returns a `CREATE VIEW` statement, with no API change. - Con: adds a sqlparser dependency to `datafusion-catalog`. - Con: re-parses every view whenever `information_schema.views` is read. - Con: the session's dialect is not known there, so dialect-specific syntax may fail to parse. ### F. `ViewTable` assembles the statement - `ViewTable::new_with_query(plan, name, column_aliases, query)` builds `CREATE VIEW name [(aliases)] AS query` and remembers where the query starts. - A new default method `TableProvider::get_logical_plan_definition()` returns the SQL for what `get_logical_plan()` returns; for a view that is the query, sliced from the full `get_table_definition` string. - `information_schema.views` uses it and falls back to `get_table_definition()`. `SHOW CREATE TABLE` is unchanged. - Pro: no downcast, any provider can implement the method, and those that don't keep today's output. - Pro: `get_table_definition()` keeps its meaning, and `ViewTable::new` keeps its signature. - Pro: The alias list appears only when declared. - Con: `CreateView.definition` now holds the query, and `CreateView` gains `column_aliases`. That breaks code building `CreateView` directly. - Con: Old serialized `CreateViewNode`s decode with the full statement taken as the query, giving `CREATE VIEW v AS CREATE VIEW v AS ...`. - Con: The statement is rebuilt from normalized parts, not the user's exact text: `OR REPLACE` is gone and the name and aliases are normalized. ## Questions for others 1. Is F the right direction, or is E's parsing acceptable in `datafusion-catalog`? 2. For F: should `CreateView.definition` keep the full statement, with a separate `query` field? That avoids the API and proto break but adds a field. 3. Is `get_logical_plan_definition()` a good name and contract for the new trait method? 4. Must `SHOW CREATE TABLE` reproduce the user's exact text (`OR REPLACE`, original casing), or is a normalized statement enough? 5. Do serialized plans need to stay compatible across versions here? -- 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]
