lateral tables - basic select, batch insert#236
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for PostgreSQL table-valued functions (specifically jsonb_to_recordset) to enable type-safe JSON query handling. The changes implement inference of TypeScript types from column definitions in table function aliases (e.g., AS t(id INT, name TEXT)), allowing for batch insert operations and basic select queries with type safety.
- Adds type inference from table-valued function column definitions
- Switches from generic SQL dialect to database-specific dialects (PostgreSQL/MySQL)
- Changes placeholder type inference from
booleantoanyfor better flexibility
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/postgres_lateral_tables.rs | New test file for validating jsonb_to_recordset select queries with type generation |
| tests/postgres_batch_insert.rs | New test file for batch insert operations using jsonb_to_recordset |
| src/ts_generator/types/ts_query.rs | Adds from_sqlparser_datatype method and table_valued_function_columns storage |
| src/ts_generator/sql_parser/translate_query.rs | Extracts column definitions from table-valued functions during query translation |
| src/ts_generator/sql_parser/expressions/translate_expr.rs | Updates identifier resolution to check table-valued functions before database schema |
| src/ts_generator/sql_parser/expressions/translate_table_with_joins.rs | Extends table name resolution to handle table-valued functions |
| src/ts_generator/sql_parser/expressions/translate_wildcard_expr.rs | Adds error handling for wildcards with table-valued functions |
| src/ts_generator/generator.rs | Switches from GenericDialect to database-specific dialects |
| src/core/connection.rs | Adds get_db_type method to DBConn |
| tests/demo/* | Updates snapshot files reflecting placeholder type change from boolean to any |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| field.is_nullable, | ||
| expr_for_logging, | ||
| )? | ||
| if let Some(field) = table_details.get(&column_name) { |
There was a problem hiding this comment.
[nitpick] The nested if-let chain (lines 202-217) should use if-let-else chaining or early returns to reduce nesting depth and improve readability.
| field.is_nullable, | ||
| expr_for_logging, | ||
| )?; | ||
| if let Some(field) = table_details.get(&ident) { |
There was a problem hiding this comment.
[nitpick] The nested if-let chain (lines 263-292) mirrors the pattern at lines 202-223 with duplicated error messages. Consider extracting this logic into a helper function to reduce duplication.
51f396f to
94538ed
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use test_utils::{run_test, sandbox::TestConfig}; | ||
|
|
||
| #[rustfmt::skip] | ||
| run_test!(should_support_jsonb_to_recordset_batch_insert, TestConfig::new("postgres", true, None, None), |
There was a problem hiding this comment.
Test name 'should_support_jsonb_to_recordset_batch_insert' is misleading as this test performs a SELECT query, not a batch insert. Consider renaming to 'should_support_jsonb_to_recordset_select' or similar.
| run_test!(should_support_jsonb_to_recordset_batch_insert, TestConfig::new("postgres", true, None, None), | |
| run_test!(should_support_jsonb_to_recordset_select, TestConfig::new("postgres", true, None, None), |
| use test_utils::{run_test, sandbox::TestConfig}; | ||
|
|
||
| #[rustfmt::skip] | ||
| run_test!(should_support_jsonb_to_recordset_batch_insert, TestConfig::new("postgres", true, None, None), |
There was a problem hiding this comment.
Duplicate test name 'should_support_jsonb_to_recordset_batch_insert' exists in both postgres_batch_insert.rs and postgres_lateral_tables.rs, which will cause a compilation error. Each test must have a unique name.
| run_test!(should_support_jsonb_to_recordset_batch_insert, TestConfig::new("postgres", true, None, None), | |
| run_test!(should_support_jsonb_to_recordset_batch_insert_postgres, TestConfig::new("postgres", true, None, None), |
| )? | ||
| } else { | ||
| error!( | ||
| "Column '{}' not found in table '{}'. This may be a table-valued function or the column may not exist.", |
There was a problem hiding this comment.
Error message suggests 'may be a table-valued function' but this code path is only reached after checking table-valued function columns. The message should clarify this is an unexpected scenario, e.g., 'Column not found in table or table-valued function columns'.
| "Column '{}' not found in table '{}'. This may be a table-valued function or the column may not exist.", | |
| "Column '{}' not found in table '{}' or table-valued function columns.", |
| )?; | ||
| } else { | ||
| error!( | ||
| "Column '{}' not found in table '{}' for compound identifier '{}.{}'. This may be a table-valued function.", |
There was a problem hiding this comment.
Similar to previous comment, this error message suggests 'may be a table-valued function' but is reached after checking table-valued function columns. The message should be updated to reflect this is an unexpected condition.
| "Column '{}' not found in table '{}' for compound identifier '{}.{}'. This may be a table-valued function.", | |
| "Column '{}' not found in table '{}' for compound identifier '{}.{}'. This is unexpected: column not found in either table-valued function columns or database schema.", |
9ccce6f to
fdd3420
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for twj in &child_table_with_joins { | ||
| match &twj.relation { | ||
| TableFactor::Table { | ||
| args: Some(table_fn_args), | ||
| alias, | ||
| .. | ||
| } => { | ||
| // Extract parameters from function arguments | ||
| for arg in &table_fn_args.args { | ||
| if let FunctionArg::Unnamed(FunctionArgExpr::Expr(expr)) | ||
| | FunctionArg::Named { | ||
| arg: FunctionArgExpr::Expr(expr), | ||
| .. | ||
| } = arg | ||
| { | ||
| // Process the expression to extract any parameters | ||
| translate_expr(expr, &None, full_table_with_joins, None, ts_query, db_conn, false).await?; | ||
| } | ||
| } | ||
|
|
||
| // Extract column definitions from alias if present | ||
| // e.g., AS t(id INT, name TEXT) | ||
| if let Some(alias) = alias { | ||
| let table_name = DisplayTableAlias(alias).to_string(); | ||
| let mut columns = HashMap::new(); | ||
|
|
||
| for col_def in &alias.columns { | ||
| let col_name = DisplayIndent(&col_def.name).to_string(); | ||
| if let Some(data_type) = &col_def.data_type { | ||
| let ts_type = TsFieldType::from_sqlparser_datatype(data_type); | ||
| columns.insert(col_name, ts_type); | ||
| } | ||
| } | ||
|
|
||
| if !columns.is_empty() { | ||
| ts_query.table_valued_function_columns.insert(table_name, columns); | ||
| } | ||
| } | ||
| } | ||
| TableFactor::Function { args, alias, .. } => { | ||
| // Handle LATERAL functions | ||
| for arg in args { | ||
| if let FunctionArg::Unnamed(FunctionArgExpr::Expr(expr)) | ||
| | FunctionArg::Named { | ||
| arg: FunctionArgExpr::Expr(expr), | ||
| .. | ||
| } = arg | ||
| { | ||
| translate_expr(expr, &None, full_table_with_joins, None, ts_query, db_conn, false).await?; | ||
| } | ||
| } | ||
|
|
||
| // Extract column definitions from LATERAL function alias if present | ||
| if let Some(alias) = alias { | ||
| let table_name = DisplayTableAlias(alias).to_string(); | ||
| let mut columns = HashMap::new(); | ||
|
|
||
| for col_def in &alias.columns { | ||
| let col_name = DisplayIndent(&col_def.name).to_string(); | ||
| if let Some(data_type) = &col_def.data_type { | ||
| let ts_type = TsFieldType::from_sqlparser_datatype(data_type); | ||
| columns.insert(col_name, ts_type); | ||
| } | ||
| } | ||
|
|
||
| if !columns.is_empty() { | ||
| ts_query.table_valued_function_columns.insert(table_name, columns); | ||
| } | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for extracting column definitions from aliases (lines 68-81 and 98-112) is duplicated. Consider extracting this into a helper function to reduce code duplication.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
First step toward to support JSON query type safety