18740: fix: update schema's data type for LogicalPlan::Values after placeholder substitution

[martin-augment]

18740: To review by AI

[coderabbitai]

Walkthrough

The pull request modifies the schema update path in the logical planner by replacing a recompute_schema() call with a new update_schema_data_type() helper method. This private method differentiates handling between Values plans, which are reconstructed using LogicalPlanBuilder, and other plans, which fall back to recompute_schema(). Additionally, a test case is added to the prepared statement test suite, verifying the execute-deallocate cycle for an untyped PREPARE statement using inline VALUES with parameters.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch pr-18740-2025-11-16-12-47-38

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

datafusion/expr/src/logical_plan/plan.rs (2)

1480-1482: Consider enhancing the documentation.

The comment could be more explicit about what this method does differently from recompute_schema(). For example:

/// Recomputes schema fields' data type after replacing params, ensuring fields data type can be
/// updated according to the new parameters. Unlike `recompute_schema()`, this method rebuilds
/// VALUES plans entirely to properly infer types from literal values after placeholder substitution.

---

1484-1488: Consider tracking the FIXME as technical debt.

The FIXME comment indicates that special-casing VALUES is a workaround. The root cause is that recompute_schema() doesn't handle VALUES plans (see lines 635-638). Consider:

1. Creating an issue to track fixing recompute_schema() to properly handle VALUES plans
2. Linking the FIXME comment to that issue
3. Evaluating whether the inconsistency between VALUES and other plan types could cause issues in the future

Additionally, when rebuilding the VALUES plan via LogicalPlanBuilder::values(values)?.build(), any schema-level metadata from the original Values.schema is discarded since values() calls infer_data() which creates a fresh schema. If schema metadata preservation is important for VALUES plans in prepared statements, this should be addressed.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 86ebaab and 364d5f7.

📒 Files selected for processing (2)

• datafusion/expr/src/logical_plan/plan.rs (1 hunks)
• datafusion/sqllogictest/test_files/prepare.slt (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

datafusion/expr/src/logical_plan/plan.rs (1)

datafusion/expr/src/logical_plan/builder.rs (2)

• plan (160-162)
• values (215-236)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: claude-review
• GitHub Check: Analyze (rust)

🔇 Additional comments (1)

datafusion/sqllogictest/test_files/prepare.slt (1)

366-375: LGTM! Good test coverage for untyped VALUES with parameters.

This test case validates the schema update logic when placeholders in a VALUES clause are replaced with concrete values, without explicit type declarations in the PREPARE statement. This complements the existing test at line 102 which uses explicit type declarations.

[claude]

Pull Request Review: Fix schema data type update for LogicalPlan::Values after placeholder substitution

Summary

This PR addresses issue apache#18740 by ensuring that the schema's data types are properly updated for LogicalPlan::Values after placeholder substitution during prepared statement execution.

---

Code Quality and Best Practices

Positive aspects:

• The fix is focused and minimal, addressing the specific issue without unnecessary changes
• Good use of pattern matching to handle the special case
• The new method update_schema_data_type() has a clear, descriptive name
• Documentation comment explains the purpose and rationale

Recommendations:

1. FIXME comment (datafusion/expr/src/logical_plan/plan.rs:1484-1485)

   • The FIXME comment indicates this is a workaround rather than a proper fix
   • Consider creating a follow-up issue to address why recompute_schema() doesn't handle LogicalPlan::Values properly
   • The root cause appears to be in the recompute_schema() method at line 635-638, which has a TODO comment explaining the same issue
   • Long-term: these two TODOs should be consolidated into a proper fix that makes recompute_schema() work correctly for Values nodes

2. Documentation consistency

   • The doc comment at line 1480 could be more detailed about when this method should be used vs. recompute_schema()
   • Consider adding a note explaining why Values requires special handling

---

Potential Bugs or Issues

Minor concern:

• Rebuilding via LogicalPlanBuilder (line 1487):
  • Using LogicalPlanBuilder::values(values)?.build() rebuilds the entire Values node from scratch
  • This approach infers the schema from the data, which should now have concrete types instead of placeholders
  • While this should work correctly, it's worth verifying that any other metadata or properties of the original Values node aren't being lost
  • The code looks safe since Values only has schema and values fields (as seen in plan.rs:2145-2150)

Validation:

• The existing test at datafusion/sqllogictest/test_files/prepare.slt:367-375 correctly tests the exact scenario being fixed
• This is a good regression test for the issue

---

Performance Considerations

Minor overhead:

• Rebuilding the Values node via LogicalPlanBuilder::values() has some overhead compared to just updating the schema in-place
• However, this only happens during prepared statement parameter substitution, not during normal query execution
• The overhead is negligible compared to the overall query planning cost
• Verdict: Performance impact is acceptable for correctness

Optimization opportunity (future):

• If recompute_schema() were fixed to properly handle Values nodes, this special case could be eliminated entirely
• This would be cleaner and potentially slightly more efficient

---

Security Concerns

No security issues identified. The change:

• Doesn't introduce any new attack surfaces
• Properly validates data through existing LogicalPlanBuilder validation
• Maintains type safety throughout the transformation

---

Test Coverage

Good:

• New sqllogictest added at datafusion/sqllogictest/test_files/prepare.slt:366-375
• Tests the exact scenario: PREPARE my_plan AS SELECT a, b FROM (VALUES ($1, $2)) AS t(a, b);
• Verifies execution with concrete values: EXECUTE my_plan(1, 2)

Recommendations:

1. Additional test cases to consider:

   • VALUES with mixed types: VALUES ($1, 'string', $2) where placeholders have different types
   • Multiple rows: VALUES ($1, $2), ($3, $4)
   • VALUES in subqueries with placeholders
   • Null values mixed with placeholders: VALUES ($1, NULL, $2)

2. Unit tests:

   • Consider adding a unit test in datafusion/sql/tests/cases/params.rs following the existing pattern
   • This would provide more detailed verification of the schema transformation

---

Overall Assessment

Verdict: Approve with minor suggestions

This is a solid fix for the reported issue. The implementation is straightforward and correct, though it's clearly a workaround for a deeper issue in recompute_schema(). The test coverage is adequate for the specific bug being fixed.

Recommendations summary:

1. Approve and merge this PR - it fixes a real bug
2. Create a follow-up issue to properly fix recompute_schema() to handle Values nodes
3. Consider adding the suggested unit test to params.rs for more comprehensive coverage
4. Link the FIXME comment to the follow-up issue once created

Great work on identifying and fixing this issue!

[augmentcode]

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

[martin-augment]

• FIXME comment (datafusion/expr/src/logical_plan/plan.rs:1484-1485)

  • The FIXME comment indicates this is a workaround rather than a proper fix
  • Consider creating a follow-up issue to address why recompute_schema() doesn't handle LogicalPlan::Values properly
  • The root cause appears to be in the recompute_schema() method at line 635-638, which has a TODO comment explaining the same issue
  • Long-term: these two TODOs should be consolidated into a proper fix that makes recompute_schema() work correctly for Values nodes

value:useful; good-to-have:documentation; feedback:The Claude AI reviewer is correct. The "FIXME" suggests that the newly introduced code is broken and needs to be fixed. It would be better to note instead that this is a temporary hack/workaround that needs to be improved later by filling an issue. Prevents confusion in the next developer who reads this code comment.

[martin-augment]

1484-1488: Consider tracking the FIXME as technical debt.

The FIXME comment indicates that special-casing VALUES is a workaround. The root cause is that recompute_schema() doesn't handle VALUES plans (see lines 635-638). Consider:

1. Creating an issue to track fixing recompute_schema() to properly handle VALUES plans
2. Linking the FIXME comment to that issue
3. Evaluating whether the inconsistency between VALUES and other plan types could cause issues in the future

Additionally, when rebuilding the VALUES plan via LogicalPlanBuilder::values(values)?.build(), any schema-level metadata from the original Values.schema is discarded since values() calls infer_data() which creates a fresh schema. If schema metadata preservation is important for VALUES plans in prepared statements, this should be addressed.

value:useful; good-to-have:documentation; feedback:The CodeRabbit AI reviewer is correct. The "FIXME" suggests that the newly introduced code is broken and needs to be fixed. It would be better to note instead that this is a temporary hack/workaround that needs to be improved later by filling an issue. Prevents confusion in the next developer who reads this code comment.

[martin-augment]

2. Documentation consistency

* The doc comment at line 1480 could be more detailed about when this method should be used vs. `recompute_schema()`
* Consider adding a note explaining why Values requires special handling

value:good-to-have; category:documentation; feedback: The Claude AI reviewer is correct that the method documentation could be improved to explain why the new method is needed. It is obvious from the method body implementation what it does but not why it does it.

[martin-augment]

1480-1482: Consider enhancing the documentation.

The comment could be more explicit about what this method does differently from recompute_schema(). For example:

/// Recomputes schema fields' data type after replacing params, ensuring fields data type can be
/// updated according to the new parameters. Unlike `recompute_schema()`, this method rebuilds
/// VALUES plans entirely to properly infer types from literal values after placeholder substitution.

value:good-to-have; category:documentation; feedback: The CodeRabbit AI reviewer is correct that the method documentation could be improved to explain why the new method is needed. It is obvious from the method body implementation what it does but not why it does it.