refactor: rewrite sample/issue922.rb to use table adapter pattern#1119
refactor: rewrite sample/issue922.rb to use table adapter pattern#1119
Conversation
- Remove DuckDB::TableFunction and DuckDB::Connection monkey-patches - Add PolarsDataFrameTableAdapter class - Register adapter via DuckDB::TableFunction.add_table_adapter - Use con.expose_as_table instead of con.create_table_function Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughThis change introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
sample/issue922.rb (2)
45-54: Use block form for automatic resource cleanup instead of manualbegin/ensure.If any statement raises an exception between lines 47–51, cleanup is skipped. Use the block form for automatic cleanup:
DuckDB::Database.open do |db| db.connect do |con| con.query('SET threads=1') con.expose_as_table(df, 'polars_df') result = con.query('SELECT * FROM polars_df()').to_a p result puts result.first.first == '1' end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample/issue922.rb` around lines 45 - 54, Replace the manual open/connect/close pattern with the block form to ensure automatic cleanup: wrap the Database.open call with a block and inside it call db.connect with a block, then move the calls to con.query('SET threads=1'), con.expose_as_table(df, 'polars_df'), the SELECT query and printing logic into that inner block so resources are closed automatically instead of calling con.close and db.close; refer to DuckDB::Database.open, db.connect, con.query and con.expose_as_table to locate the relevant code to change.
31-32: Implement dtype-to-LogicalType mapping for Polars columns ininfer_columns.The current implementation coerces all columns to
VARCHAR, which loses native Polars type information and forces numeric values to strings (e.g.,1becomes'1'). Consider mapping Polars dtypes to DuckDB logical types—for example,Int64→DuckDB::LogicalType::BIGINT,Float64→DuckDB::LogicalType::DOUBLE,Boolean→DuckDB::LogicalType::BOOLEAN, andDate→DuckDB::LogicalType::DATE, withVARCHARas fallback for unmapped types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample/issue922.rb` around lines 31 - 32, The infer_columns method currently maps every Polars column to DuckDB::LogicalType::VARCHAR; update infer_columns to inspect each column's Polars dtype and map it to the appropriate DuckDB::LogicalType (e.g., Int64 -> DuckDB::LogicalType::BIGINT, Float64 -> DuckDB::LogicalType::DOUBLE, Boolean -> DuckDB::LogicalType::BOOLEAN, Date -> DuckDB::LogicalType::DATE) and fall back to DuckDB::LogicalType::VARCHAR for unknown dtypes; modify the code that builds data_frame.columns.to_h so it queries each column's dtype (via Polars column dtype accessor) and returns the mapped DuckDB::LogicalType for that header.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sample/issue922.rb`:
- Around line 45-54: Replace the manual open/connect/close pattern with the
block form to ensure automatic cleanup: wrap the Database.open call with a block
and inside it call db.connect with a block, then move the calls to
con.query('SET threads=1'), con.expose_as_table(df, 'polars_df'), the SELECT
query and printing logic into that inner block so resources are closed
automatically instead of calling con.close and db.close; refer to
DuckDB::Database.open, db.connect, con.query and con.expose_as_table to locate
the relevant code to change.
- Around line 31-32: The infer_columns method currently maps every Polars column
to DuckDB::LogicalType::VARCHAR; update infer_columns to inspect each column's
Polars dtype and map it to the appropriate DuckDB::LogicalType (e.g., Int64 ->
DuckDB::LogicalType::BIGINT, Float64 -> DuckDB::LogicalType::DOUBLE, Boolean ->
DuckDB::LogicalType::BOOLEAN, Date -> DuckDB::LogicalType::DATE) and fall back
to DuckDB::LogicalType::VARCHAR for unknown dtypes; modify the code that builds
data_frame.columns.to_h so it queries each column's dtype (via Polars column
dtype accessor) and returns the mapped DuckDB::LogicalType for that header.
Summary
Rewrites
sample/issue922.rbto use the table adapter pattern, consistent with the approach insample/issue930.rb.Changes
DuckDB::TableFunction,DuckDB::Connection, andDuckDB::Polars::DataFramePolarsDataFrameTableAdapterclass following the same structure asCSVTableAdapterDuckDB::TableFunction.add_table_adapter(Polars::DataFrame, ...)con.expose_as_tableinstead ofcon.create_table_functionSummary by CodeRabbit