fix(core): fix the uppercase table name of the model tableReference#1173
fix(core): fix the uppercase table name of the model tableReference#1173douenergy merged 3 commits intoCanner:mainfrom
Conversation
WalkthroughThis update introduces enhanced handling for SQL identifiers and table references within the codebase. A new utility module provides robust parsing, normalization, and quoting of SQL identifiers, leveraging the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Case
participant ModelBuilder as ModelBuilder
participant Utils as Identifier Utils
participant Manifest as Manifest (Serde)
participant SQLTransformer as SQL Transformer
Test->>ModelBuilder: Create Model with quoted/Unicode/uppercase table_reference
ModelBuilder->>Manifest: Serialize Model to JSON
Manifest->>Utils: Normalize and quote identifiers
Manifest->>Manifest: Deserialize Model from JSON
Manifest->>Utils: Parse identifiers with case/quote sensitivity
Test->>SQLTransformer: Analyze and transform SQL query
SQLTransformer->>Utils: Quote identifiers for SQL generation
SQLTransformer->>Test: Return transformed SQL with proper quoting/casing
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
core
dependencies
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wren-core-base/src/mdl/utils.rs (1)
31-41: Potential error handling improvement opportunityThe function silently converts parsing errors to empty vectors via
unwrap_or_default(). This works for the current use case but could hide actual parsing issues.Consider propagating errors to callers in cases where they need to know about parsing failures:
-pub(crate) fn parse_identifiers_normalized(s: &str, ignore_case: bool) -> Vec<String> { +pub(crate) fn parse_identifiers_normalized(s: &str, ignore_case: bool) -> Result<Vec<String>, sqlparser::parser::ParserError> { - parse_identifiers(s) - .unwrap_or_default() + parse_identifiers(s).map(|idents| { + idents .into_iter() .map(|id| match id.quote_style { Some(_) => id.value, None if ignore_case => id.value, _ => id.value.to_ascii_lowercase(), }) .collect::<Vec<_>>() + }) }Or add a comment explaining the reasoning behind using
unwrap_or_default().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
wren-core-py/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
wren-core-base/Cargo.toml(1 hunks)wren-core-base/src/mdl/builder.rs(1 hunks)wren-core-base/src/mdl/manifest.rs(4 hunks)wren-core-base/src/mdl/mod.rs(1 hunks)wren-core-base/src/mdl/utils.rs(1 hunks)wren-core/core/src/mdl/mod.rs(1 hunks)
🔇 Additional comments (12)
wren-core-base/Cargo.toml (1)
16-16: Appropriate addition of the sqlparser dependencyThe
sqlparsercrate with the "visitor" feature will provide robust SQL identifier parsing capabilities, which aligns with the PR objective of fixing case sensitivity in table references.wren-core-base/src/mdl/utils.rs (3)
24-29: Well-structured SQL identifier parsing functionThis function correctly uses sqlparser to parse multipart identifiers from a string input, leveraging the GenericDialect for broad SQL compatibility.
43-49: Efficient use of Cow for zero-copy optimizationsGreat use of
Cow<str>to avoid unnecessary allocations when quotes aren't needed, and proper escaping of internal quotes by doubling them.
52-63: Accurate SQL identifier quoting logicThe function correctly determines when identifiers need quotes based on SQL standard rules for the first character.
wren-core-base/src/mdl/mod.rs (1)
24-24: Appropriate module declarationCorrectly adds the new utilities module to the mdl namespace.
wren-core-base/src/mdl/builder.rs (1)
485-492: Good test case for quoted table referencesThis test case effectively verifies that a model with a quoted table reference (
"Wren"."Public"."Source") can be properly serialized and deserialized while preserving case sensitivity.wren-core-base/src/mdl/manifest.rs (4)
116-117: Good addition of utility imports for SQL identifier handling.The new imports from the
utilsmodule provide essential functions for properly handling SQL identifiers, which is critical for the case-sensitivity fix.
138-141: Improved deserializer now properly quotes identifiers.The deserializer now correctly handles each component of the table reference by:
- Filtering out empty strings
- Properly quoting each identifier using the
quote_identifierfunctionThis ensures that table references maintain proper case sensitivity during deserialization.
154-154: Robust parsing of identifiers during serialization.Replacing the naive string splitting with
parse_identifiers_normalizedprovides proper handling of quoted identifiers and case sensitivity during serialization. This is a key improvement that fixes the issue with uppercase table names.
323-333: Good test for case-sensitive identifiers.This test verifies that quoted identifiers with mixed case are correctly serialized, preserving the original casing. It's an essential addition that validates the fix for the case-sensitivity issue.
wren-core/core/src/mdl/mod.rs (2)
1442-1483: Excellent test for uppercase table references.This test verifies that models with uppercase table names are correctly transformed into SQL queries with proper quoting. It confirms that the changes to the serialization and deserialization process work correctly with uppercase identifiers.
The test correctly validates that:
- The manifest with an uppercase table name can be parsed
- The SQL transformation correctly quotes the uppercase table name
This is a crucial test for the fix being implemented.
1485-1526: Great test coverage for Unicode table references.This test extends the validation to include Unicode characters in table and catalog names, ensuring that:
- Unicode identifiers are properly handled during serialization/deserialization
- The SQL transformation correctly quotes Unicode identifiers
This provides additional confidence in the robustness of the fix for different character sets.
2 participants
Description
Previously, the table reference in a manifest JSON was case-insensitive. However, it's not expected behavior. This PR fixes the serialization and deserialization behaviors for the table reference.
Summary by CodeRabbit