Skip to content

Conversation

iamlinjunhong
Copy link
Contributor

@iamlinjunhong iamlinjunhong commented Jun 23, 2025

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #16471

What this PR does / why we need it:

ast string with quote identifier


PR Type

Enhancement


Description

  • Add quote identifier formatting option for AST string output

  • Implement backtick quoting for table names and identifiers

  • Add test case for partition table with quoted identifiers


Changes walkthrough 📝

Relevant files
Tests
mysql_sql_test.go
Add quote identifier test case                                                     

pkg/sql/parsers/dialect/mysql/mysql_sql_test.go

  • Add new test case TestQuoteIdentifer for partition table SQL
  • Test quote identifier functionality with backticks
  • Include expected output with quoted column reference in partition
    clause
  • +25/-0   
    Enhancement
    format.go
    Add quote identifier formatting option                                     

    pkg/sql/parsers/tree/format.go

  • Add quoteIdentifier boolean field to FmtCtx struct
  • Implement WithQuoteIdentifer() option function
  • +7/-0     
    identifier.go
    Implement identifier quoting in UnresolvedName                     

    pkg/sql/parsers/tree/identifier.go

  • Modify UnresolvedName.Format() to support quote identifier option
  • Add backtick wrapping when quoteIdentifier is enabled
  • +13/-4   
    table_name.go
    Implement identifier quoting in TableName                               

    pkg/sql/parsers/tree/table_name.go

  • Update TableName.Format() to support quote identifier option
  • Add backtick wrapping for catalog, schema, and object names
  • +20/-8   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Typo

    Function name has a typo - should be WithQuoteIdentifier instead of WithQuoteIdentifer (missing 'i' in 'Identifier')

    func WithQuoteIdentifer() FmtCtxOption {
    	return FmtCtxOption(func(ctx *FmtCtx) {
    Logic Issue

    The else block formatting logic appears incomplete - the closing brace is missing proper indentation and structure

    } else {
    	for i := node.NumParts - 1; i >= 0; i-- {
    		ctx.WriteString(node.CStrParts[i].Origin())
    		if i > 0 {
    			ctx.WriteByte('.')
    		}
    	}
    }

    Copy link

    You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Security
    Escape backticks in identifiers

    The backtick concatenation could fail if node.CStrParts[i].Origin() contains
    backticks. Consider escaping existing backticks by doubling them to prevent SQL
    injection or parsing errors.

    pkg/sql/parsers/tree/identifier.go [110-116]

     if ctx.quoteIdentifier {
     	for i := node.NumParts - 1; i >= 0; i-- {
    -		ctx.WriteString("`" + node.CStrParts[i].Origin() + "`")
    +		origin := strings.ReplaceAll(node.CStrParts[i].Origin(), "`", "``")
    +		ctx.WriteString("`" + origin + "`")
     		if i > 0 {
     			ctx.WriteByte('.')
     		}
     	}
     }

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly points out a potential issue where an identifier containing a backtick would not be properly escaped, leading to invalid SQL. This is a valid correctness and security improvement.

    Medium
    Escape backticks in table names

    Similar to the identifier issue, backticks in catalog, schema, or object names
    should be escaped by doubling them to prevent SQL parsing errors.

    pkg/sql/parsers/tree/table_name.go [24-34]

     if ctx.quoteIdentifier {
     	if tn.ExplicitCatalog {
    -		ctx.WriteString("`" + string(tn.CatalogName) + "`")
    +		catalog := strings.ReplaceAll(string(tn.CatalogName), "`", "``")
    +		ctx.WriteString("`" + catalog + "`")
     		ctx.WriteByte('.')
     	}
     	if tn.ExplicitSchema {
    -		ctx.WriteString("`" + string(tn.SchemaName) + "`")
    +		schema := strings.ReplaceAll(string(tn.SchemaName), "`", "``")
    +		ctx.WriteString("`" + schema + "`")
     		ctx.WriteByte('.')
     	}
    -	ctx.WriteString("`" + string(tn.ObjectName) + "`")
    +	object := strings.ReplaceAll(string(tn.ObjectName), "`", "``")
    +	ctx.WriteString("`" + object + "`")
     }

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that table, schema, or catalog names containing backticks are not escaped, which could lead to invalid SQL generation. This is a valid correctness and security improvement.

    Medium
    General
    Fix function name typo

    The function name has a typo - "Identifer" should be "Identifier". This
    misspelling could cause confusion and inconsistency in the API.

    pkg/sql/parsers/tree/format.go [70-74]

    -func WithQuoteIdentifer() FmtCtxOption {
    +func WithQuoteIdentifier() FmtCtxOption {
     	return FmtCtxOption(func(ctx *FmtCtx) {
     		ctx.quoteIdentifier = true
     	})
     }
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly identifies a typo in the function name WithQuoteIdentifer. Fixing this improves API consistency and code quality. This change would also require updating the call site in the test file.

    Low
    • More

    @mergify mergify bot merged commit 5c4f615 into matrixorigin:main Jun 23, 2025
    19 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Review effort 2/5 size/S Denotes a PR that changes [10,99] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants